fixed tests, now they are broken #150

bekzod · 2017-06-04T10:51:25Z

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 · 2017-06-04T17:19:03Z

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 · 2017-06-04T17:24:59Z

Yes of course, i wrote some therefore :P, I think only one failing other one i think just jshint error

…

On 4 Jun 2017, at 22:19, Krati Ahuja ***@***.***> wrote: 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. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.

kratiahuja · 2017-06-04T17:27:59Z

There are two failing. One is a jshint.

bekzod · 2017-06-04T18:12:16Z

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

stefanpenner · 2017-06-04T18:22:55Z

test/fastboot-test.js

@@ -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) {


why done?

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

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

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

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.

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

will cleanup with next PR, using it :)

bekzod · 2017-06-04T19:25:23Z

hurray tests fixed 🎊

kratiahuja · 2017-06-04T19:48:24Z

test/fastboot-test.js

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


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.

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

stefanpenner · 2017-06-05T15:21:16Z

src/ember-app.js

        result.instance = instance;
-        registerFastBootInfo(fastbootInfo, instance);
+        return new Promise(function(resolve, reject) {


stefanpenner · 2017-06-05T15:21:27Z

src/ember-app.js

+          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(()=> {


funky whitespace between () and =>

stefanpenner · 2017-06-05T15:22:23Z

test/fastboot-test.js

@@ -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) {


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

bekzod · 2017-06-09T19:38:21Z

good to merge ? :P

stefanpenner · 2017-06-09T22:23:03Z

test/fastboot-test.js

      .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');


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

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 ?

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

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

bekzod · 2017-06-11T17:15:01Z

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

kratiahuja · 2017-06-11T18:45:35Z

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

stefanpenner reviewed Jun 4, 2017

View reviewed changes

bekzod force-pushed the broken-tests branch from 0570b97 to 9914c1a Compare June 4, 2017 19:29

kratiahuja reviewed Jun 4, 2017

View reviewed changes

bekzod added 2 commits June 5, 2017 10:37

fix jshint error

6308c5f

bekzod force-pushed the broken-tests branch from 9914c1a to 3efcdb0 Compare June 5, 2017 05:38

fixed forcefully destroy the app instance test

5455fa1

bekzod force-pushed the broken-tests branch from 3efcdb0 to 5455fa1 Compare June 5, 2017 05:51

bekzod added 2 commits June 5, 2017 14:15

avoid calling timed destroy too early before app instance created

5a8fe13

refactor how timed app instance destroy works

b20e775

stefanpenner requested changes Jun 5, 2017

View reviewed changes

updated error message

ad6f109

bekzod force-pushed the broken-tests branch from 8d99029 to ad6f109 Compare June 5, 2017 15:56

stefanpenner requested changes Jun 9, 2017

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fixed tests, now they are broken #150

fixed tests, now they are broken #150

bekzod commented Jun 4, 2017

kratiahuja commented Jun 4, 2017

bekzod commented Jun 4, 2017 via email

kratiahuja commented Jun 4, 2017

bekzod commented Jun 4, 2017 •

edited

stefanpenner Jun 4, 2017

bekzod Jun 4, 2017

kratiahuja Jun 4, 2017

kratiahuja Jun 4, 2017

bekzod Jun 4, 2017

stefanpenner Jun 5, 2017

bekzod Jun 5, 2017

bekzod commented Jun 4, 2017

kratiahuja Jun 4, 2017

bekzod Jun 5, 2017

stefanpenner Jun 5, 2017

stefanpenner Jun 5, 2017

stefanpenner Jun 5, 2017

bekzod commented Jun 9, 2017

stefanpenner Jun 9, 2017

bekzod Jun 10, 2017

kratiahuja Jun 10, 2017

bekzod Jun 10, 2017 •

edited

bekzod commented Jun 11, 2017

kratiahuja commented Jun 11, 2017

fixed tests, now they are broken #150

Are you sure you want to change the base?

fixed tests, now they are broken #150

Conversation

bekzod commented Jun 4, 2017

kratiahuja commented Jun 4, 2017

bekzod commented Jun 4, 2017 via email

kratiahuja commented Jun 4, 2017

bekzod commented Jun 4, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bekzod commented Jun 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bekzod commented Jun 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bekzod Jun 10, 2017 • edited

Choose a reason for hiding this comment

bekzod commented Jun 11, 2017

kratiahuja commented Jun 11, 2017

bekzod commented Jun 4, 2017 •

edited

bekzod Jun 10, 2017 •

edited