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

[REGRESSION]: Difference between Ember 1.11.0 and Ember 1.11.0-beta.5 rendering/run loop #10880

Closed
williamsbdev opened this issue Apr 15, 2015 · 20 comments

Comments

@williamsbdev
Copy link

We have an app that is relying on the way that Ember is rendering/using the run loop in testing. We are trying to have the templates render a "Loading..." state before a promise returning the array of items has been resolved. In our test we are doing the following and this passes in Ember 1.11.0-beta.5 but fails in Ember 1.11.X:

test("show loading state", function(assert) {
    var returnArray = [{id: 1, name: "foo"}, {id: 2, name: "bar"});
    stubRequest("/todos", returnArray);
    visit("/");
    Ember.run(function() {
        // Promise has not resolved but template has rendered in Ember 1.11.0-beta.5
        // Template has not rendered in Ember 1.11.X
        assert.equal(find("#loading-details").is(":visible"), true);
        assert.equal(find("#loading-details").text(), "Loading...");
    });
    andThen(function() {
        // Promise has resolved
        assert.equal(find("#loading-details").is(":hidden"), true);
        assert.equal(find(".todo").length, 2);
    });
});

We are returning an ArrayProxy that gets populated by the promise and we have a check in the template whether the ArrayProxy "isLoaded" as we are setting the "isLoaded" property on the ArrayProxy when the promise resolves.

The app works perfectly when running ember server when I have the same stubbed request and a response time of 2 seconds. I get the "Loading..." state in the template and then the array populates when the promise finally resolves and the data is shown.

@toranb
Copy link
Contributor

toranb commented Apr 15, 2015

I have a full ember-cli app to show this in action

https://github.com/toranb/kanban-board-without-ember-data/blob/allAsyncLoading/tests/acceptance/assignemnt-test.js#L96-L120

The tests I link to above show that previously (in 1.11-beta 5) I could run the loop and see the content loading before a promise was resolved

Here is the basic template exercised in the test above if you are still looking for more context around how this did work/etc.

      {{#if controller.content.isLoaded}}
        {{#if controller.content}}
          <div class="found">show data here</div>
        {{else}}
          <div class="no-data-found"><h2>No data available</h2></div>
        {{/if}}
        {{else}}
          <div class="loading-gif"><h2>Loading...</h2></div>
      {{/if}}

@teddyzeenny
Copy link
Contributor

That's because there was a bug where some async helpers were triggered synchronously, and you were relying on this (accidental) behavior in your tests.

One possible solution is to replace visit with a custom synchronous helper instead:

Ember.Test.registerHelper('syncVisit', function(app, url) {
  // .. code to visit a route goes here ..

  return app.testHelpers.wait();
});

The long term solution is to use ES7 async/await and make all test helpers sync. This will resolve the async helpers issue. See ember-cli/ember-cli#3529.

@rwjblue
Copy link
Member

rwjblue commented Apr 15, 2015

I have good news and bad news.

Good News

This isn't a regression! 😂

Bad News

You are kinda screwed. 😞


Seriously, though here is the lowdown on why it was working and now why it is not:

  • In 1.11.0-beta5 and lower

The first async helper was always being executed synchronously (in your case visit). This is generally bad based on the principal that if a given API can be async then it should always be async (otherwise it becomes a nightmare to reason about).

In your case here, the visit was executed so that the route has been transitioned and the initial model hook has fired, but since it runs synchronously you could check the loading state results.

  • 1.11.0 and above

In #10463 the bug causing the first helper to be ran synchronously was fixed (it really was a bug). Now all async helpers are always async.

Unfortunately, this means that there is no way (that I know of) at the moment to test the loading states/templates without visiting them directly.

@rwjblue
Copy link
Member

rwjblue commented Apr 15, 2015

@teddyzeenny - Thanks for explaining the way forward on this...

@toranb
Copy link
Contributor

toranb commented Apr 15, 2015

@rwjblue correct me if I'm wrong but it looks like the syncVisit helper @teddyzeenny showed above would provide the ability to inspect the template for loading state/etc

@rwjblue
Copy link
Member

rwjblue commented Apr 15, 2015

@toranb - Yep, it works but I don't really consider that a "good" solution for the long term (but it is great for now until the async/await stuff is ready for primetime). As an Ember app developer you generally shouldn't be forced to copy (and maintain as it changes) code from built-in test helpers.

I'm mostly just saying that we need to do better here, and @teddyzeenny + @stefanpenner have been paving the way on the better long term solution (via the async/await stuff).

@williamsbdev
Copy link
Author

Thank you for this great explanation!

@toranb
Copy link
Contributor

toranb commented Apr 15, 2015

@williamsbdev can you close the issue as it's something we can "work around" and not a regression as we originally thought.

@teddyzeenny one question about "how to" implement that sync based visit helper. Looking at the visit source it appears to "wait" (using 1.11.2 -looking at ember.debug.js from bower)

function visit(app, url) {
    var router = app.__container__.lookup('router:main');
    router.location.setURL(url);

    if (app._readinessDeferrals > 0) {
      router['initialURL'] = url;
      run['default'](app, 'advanceReadiness');
      delete router['initialURL'];
    } else {
      run['default'](app.__deprecatedInstance__, 'handleURL', url);
    }

    return app.testHelpers.wait();
  }

I originally took your comment to mean "re implement visit but wait" so now I'm unsure of what this helper looks like (that would be different than visit above).

Also, even when I added this test helper using the syntax you listed above - I get syncVisit is not defined when the tests run. Is an additional step required to "officially register" the helper so I can use it as easily as I did visit?

@teddyzeenny
Copy link
Contributor

@toranb it would be exactly the same as the original visit helper, except registered using registerHelper instead of registerAsyncHelper. It's not about the wait returned from the visit, but about the implicit wait in async helpers before visit starts executing. So with the function you pasted above, you can just add Ember.Test.registerHelper('syncVisit', visit);.

I get syncVisit is not defined when the tests run

You'll want to register it before calling startApp().

@toranb
Copy link
Contributor

toranb commented Apr 15, 2015

First up -adding visitSync to my .jshintrc (for tests) eliminated the visitSync not found issue I mentioned above. (sorry for the newb like panic but it didn't mention jshint in the breakdown)

Next - just hacking around in the ember source I added this line (as you suggested) and it solved the problem. Now I can do a simple Ember.run as I did in beta 5 :)

 asyncHelper('visit', visit);
 helper('visitSync', visit);

@toranb
Copy link
Contributor

toranb commented Apr 15, 2015

@teddyzeenny the very last question is - now that I'm trying to register this before startApp I need something that works and today the below isn't working to register this correctly. I assume it's because visit isn't a "thing" in beforeEach like it is inside the QUnit test block. What is the best way to register this before startApp (but in a way I can tap into visit to reduce pure duplication if that's an option)

module('Acceptance: My Test Suite', {
  beforeEach: function() {
    Ember.Test.registerHelper('visitSync', visit);
    application = startApp();
  }
});

@teddyzeenny
Copy link
Contributor

@toranb unfortunately you have to re-define the visit function (probably call it visitSync). That's why @rwjblue mentioned it wasn't a good solution (since the visit function contains private api, you will need to keep it in sync with the core one until a better long term solution is present).

Note that you don't need to call registerHelper in beforeEach, just once before starting the tests, for example in the start-app.js file next to the startApp function definition, or just before the module('Acceptance: My Test Suite') call.

@toranb
Copy link
Contributor

toranb commented Apr 15, 2015

@teddyzeenny you be up to help solve this some night in the next 2 weeks for an hourly bill rate? I've tried a few different approaches and cannot get around this jshint error saying "redefinition of visitSync" that I can't seem to find (did a complete fresh npm install / bower install - blew away tmp/dist/etc)

If you are free for some consulting work ping me offline :) toranb at gmail

Here is what I've tried (in the startApp helper) with no luck so far

var visitSync = function(app, url) {
    var router = app.__container__.lookup('router:main');
    router.location.setURL(url);

    if (app._readinessDeferrals > 0) {
      router['initialURL'] = url;
      Ember.run(app, 'advanceReadiness');
      delete router['initialURL'];
    } else {
      Ember.run(app.__deprecatedInstance__, 'handleURL', url);
    }

    return app.testHelpers.wait();
};

Ember.Test.registerHelper("visitSync", visitSync);

@teddyzeenny
Copy link
Contributor

@toranb that's because you have (correctly) configured visitSync as a global in .jshintrc. The error will go away by not defining a visitSync variable:

Ember.Test.registerHelper("visitSync", function(app, url) {
    var router = app.__container__.lookup('router:main');
    router.location.setURL(url);

    if (app._readinessDeferrals > 0) {
      router['initialURL'] = url;
      Ember.run(app, 'advanceReadiness');
      delete router['initialURL'];
    } else {
      Ember.run(app.__deprecatedInstance__, 'handleURL', url);
    }

    return app.testHelpers.wait();
});

This should solve the issue :)

@stefanpenner
Copy link
Member

Ember.run(function() {
// Promise has not resolved but template has rendered in Ember 1.11.0-beta.5
// Template has not rendered in Ember 1.11.X
assert.equal(find("#loading-details").is(":visible"), true);
assert.equal(find("#loading-details").text(), "Loading...");
});

these is no point in this run

@stefanpenner
Copy link
Member

This should solve the issue :)

this should be considered a short-term solution. Please update your tests to not rely on this being sync, as it is a pretty dubious thing to rely on.

@toranb
Copy link
Contributor

toranb commented Apr 15, 2015

@teddyzeenny haha that did the trick :) **now where to mail the check for all your help today

@stefanpenner In my example app the Ember.run allowed me to verify the conditional in my hbs templates for loading state/empty state/loaded state. Without this (and the visit sync helper) I cannot do automated testing around the pre-loaded state **unless you can prove me wrong :)

@stefanpenner
Copy link
Member

That run doesn't appear to be wrapping any run loop code. So it appears like a no opt. Or am I missing something

@williamsbdev
Copy link
Author

I'm closing this issue as this is not a real bug and I believe we have a way to do what we need. Thank you everyone @teddyzeenny @rwjblue @stefanpenner @toranb!

@benkiefer
Copy link

benkiefer commented Sep 3, 2016

Sorry to bring this back up, but is there any chance that there is better support for this in the 2.x series of Ember. I can open a new issue if needed.

Our goal is to functionally test that we correctly configured the loading route. @stefanpenner @rwjblue @teddyzeenny

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

No branches or pull requests

6 participants