Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fixed tests, now they are broken #150

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

bekzod
Copy link
Contributor

@bekzod bekzod commented Jun 4, 2017

mocha has behavior of setting test as passing if promise returned inside it(....) passing,
but if you have assertion in .catch it might skip assertion because returned promise resolved,
to avoid that you need finish assertion with done

because of it I think some of errors have been missed in master @kratiahuja @krisselden

@kratiahuja
Copy link
Contributor

I thought mocha has the same behavior with returning promise or using done: https://mochajs.org/#working-with-promises

Also not all tests are failing when using done in a catch.

@bekzod
Copy link
Contributor Author

bekzod commented Jun 4, 2017 via email

@kratiahuja
Copy link
Contributor

There are two failing. One is a jshint.

@bekzod
Copy link
Contributor Author

bekzod commented Jun 4, 2017

@kratiahuja I am look at those errors, will make PR if I fix them thanks

@@ -101,16 +101,15 @@ describe("FastBoot", function() {
});
});

it("can forcefully destroy the app instance using destroyAppInstanceInMs", function() {
it("can forcefully destroy the app instance using destroyAppInstanceInMs", function(done) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why done?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

because otherwise it is skipping assertion in catch, you can try yourself

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I a bit surprised with the mocha behavior if it is right. I thought returning promise or done was the same in mocha.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be fixed in mocha upstream as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think since you are returning resolved promise it is behaving correctly, it either needs rejections inside then or it needs to use done

I think this should be fixed in mocha upstream as well.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general I find https://github.com/domenic/chai-as-promised clears all this up nicely.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will cleanup with next PR, using it :)

@bekzod
Copy link
Contributor Author

bekzod commented Jun 4, 2017

hurray tests fixed 🎊

destroyAppInstanceInMs: 5
})
// delaying `visitRoute` to forcefully destroy app instance
let originalVisitRoute = fastboot._app.visitRoute;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of mutating the private API in the test to force a condition, please create a fixture that does a setTimeout in the model hook which implicitly will delay the visit resolution.

It seems basic-app does not do this currently.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added long-resolving-app with application route that has model which resolves in 10 seconds, that revealed 2 problems:

  • timed destroy could be fired too early before app instance created (so ended up not destroying app)
  • when app destroyed before instance.visit is called, it was throwing error for setting property for destroyed object (which is still thrown but it is superseded by App instance was forcefully destroyed ...).

both of this issues handled now

mocha has behavior of setting test as passing if `promise` returned inside `it(....)` passing,
but if you have assertion in `.catch` it might skip assertion because returned promise resolved,
to avoid that you need finish assertion with `done`

because of it I think some of errors have been missed in master
src/ember-app.js Outdated
result.instance = instance;
registerFastBootInfo(fastbootInfo, instance);
return new Promise(function(resolve, reject) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

=> ?

src/ember-app.js Outdated
if (destroyAppInstanceInMs > 0) {
// start a timer to destroy the appInstance forcefully in the given ms.
// This is a failure mechanism so that node process doesn't get wedged if the `visit` never completes.
timer = setTimeout(()=> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

funky whitespace between () and =>

@@ -101,16 +101,15 @@ describe("FastBoot", function() {
});
});

it("can forcefully destroy the app instance using destroyAppInstanceInMs", function() {
it("can forcefully destroy the app instance using destroyAppInstanceInMs", function(done) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general I find https://github.com/domenic/chai-as-promised clears all this up nicely.

@bekzod
Copy link
Contributor Author

bekzod commented Jun 9, 2017

good to merge ? :P

.catch((e) => {
expect(e.message).to.equal('App instance was forcefully destroyed in 5ms');
expect(e.message).to.equal('Fastboot forcefully destroyed App instance in 5ms');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if this expect fails, I believe an assertion is thrown preventing done from being invoked. This may result in annoying test issues on failure.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

test will just timeout, current timeout 1000ms I think, so it will still fail,
but if I return whole promsie return fastboot.visit('/', latest mocha will complain that
we are returning promise and using done at the same time, thoughts ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So why can't we just return the promise and move the assertion in the reject function?

Copy link
Contributor Author

@bekzod bekzod Jun 10, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kratiahuja because of this #150 (comment) , the whole point of this PR was not to do that, because of mentioned problems :)

@bekzod
Copy link
Contributor Author

bekzod commented Jun 11, 2017

this probably fixes ember-fastboot/ember-cli-fastboot#209 too

@kratiahuja
Copy link
Contributor

No those are different issues. The new config was introduced later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants