[BUGFIX release] Fix for URL problem using currentURL() acceptance test helper #11201
Conversation
elskwid
force-pushed
the
fix-acceptance-test-urls
branch
from
f663387
to
18bcf9f
Compare
| @@ -57,6 +57,8 @@ function focus(el) { | |||
|
|
|||
| function visit(app, url) { | |||
| var router = app.__container__.lookup('router:main'); | |||
| router._setupLocation(); | |||
There was a problem hiding this comment.
We should try to avoid manually calling _setupLocation here (as it forces the location to be ready before the actual application is fully booted). However, this is precisely why the call to router.location.setURL was moved down in 8e130e5 (because the location isn't ready yet).
Can you instead do something like the following (just after grabbing the router:main here):
app.boot().then(function() {
router.location.setURL(url);
});Then remove the duplicate router.location.setURL call in the else block.
There was a problem hiding this comment.
@rwjblue - Yes, like I said, I figured it wasn't the correct way to fix the problem just a place to discuss the fix. 😀
Aha! I see, so boot() gets the application up and running, so to speak, but the important thing for our needs is the advanceReadiness call which means we get the router + location set up.
|
Thanks for tackling this! Your research was very helpful in understanding what happened (and why). I'm labeling this for the 1.12.1 milestone, so please prefix your commit (once you have a chance to try my suggestion) with |
|
@rwjblue your suggestion worked perfectly. Would you like this squashed down into one commit? |
|
Yes, please and prefixed with |
elskwid
force-pushed
the
fix-acceptance-test-urls
branch
from
18bcf9f
to
b290afd
Compare
Problem: There are reported cases of the currentURL helper returning an empty string after the release of 1.12. Tests: The acceptance tests are not currently checking the currentURL after using the visit helper. This adds an assertion for the current url after each currentRoute assertion - the currentRouteName and currentPath helpers are functioning normally and, in these tests, the currentRoute is a var set on transition to a new route so there is a need to specifically check the url. Fix: It turns out that this isn't so much a problem with currentURL as it is the visit helper. When the lazy router location work was completed in 8e130e5 the call to `router.location.setURL()` was moved down in the else case after checking for readiness deferrals. The problem seems to be a timing issue with the call to setupRouter after the application is marked ready. The initialURL, while set, doesn't give us a reliable URL during tests. The call to `app.boot()` ensures the application is booted and ready, the important part for this fix is the call to `advanceReadiness` in boot - it is there that the router and location are set up. After the app is booted the call to `setURL` works as expected. Thanks to @rwjblue for working out the code needed to correct the issue.
elskwid
force-pushed
the
fix-acceptance-test-urls
branch
from
b290afd
to
1771aec
Compare
|
@rwjblue - Done. Thank you for your help and prompt response. Way to kick off a week! |
[BUGFIX release] Fix for URL problem using currentURL() acceptance test helper
|
Thank you! |
|
Also, pulled the fix into beta and canary branches. |
Hello there,
We attempted an upgrade to Ember 1.12 today and encountered an issue with using
currentURL()in our acceptance tests. This problem, or something similar, has been reported in #10850, #10784, and most recently in #11175.Digging in showed that it isn't so much that
currentURL()isn't returning the correct url but thatvisit()isn't setting the url.I've added failing tests in
ember-testing/tests/acceptance_test.js(and added some missingexpects). The actual fix, as I mention in my commit messages, is based on my limited understanding of the router, application, and application instance lifecycle. From the commits:I'm happy to revise, edit, cleanup, or whatever is needed. Please ask if you have any questions.
Thanks for a fantastic project and I hope this helps.