Lazily setup the router in non-application tests #19080
Conversation
xg-wang
force-pushed
the
non-application-router-test
branch
3 times, most recently
from
41b87a7
to
5843603
Compare
|
@rwjblue (friendly ping) can you take another look? |
xg-wang
force-pushed
the
non-application-router-test
branch
from
5843603
to
c6226d0
Compare
xg-wang
force-pushed
the
non-application-router-test
branch
from
7571af3
to
038471d
Compare
packages/ember/tests/routing/router_service_test/non_application_test_test.js
Outdated
Show resolved
Hide resolved
| if (instance) { | ||
| instance.didCreateRootView(this._toplevelView); | ||
| } |
There was a problem hiding this comment.
On a cursory review, this seems somewhat unrelated to the other changes. What circumstances would have forced _setOutlets to be called without an -application-instance:main (IOW why is this change needed)?
There was a problem hiding this comment.
hmmm RenderingTestCase does not have -application-instance:main registered. I guess it's just how ember internal test setup, not having any real impact on user's test. Should we use another base test case class?
There was a problem hiding this comment.
No, I think its fine but I'd like to make an explicit test case so we don't accidentally make rendering test case have one and remove the guard. Specifically, I think tests that use a custom resolver (e.g. ember-engines's engineResolverFor) will have this exact issue.
There was a problem hiding this comment.
Created a RouterNonApplicationTestCase
|
Thanks for pushing this forward @xg-wang! |
xg-wang
force-pushed
the
non-application-router-test
branch
from
24deea5
to
bf8311e
Compare
During non setupApplicationTests type tests, the Router being injected into service:router and service:routing does not setup automatically, during which it initializes its _routerMicrolib, etc. Public API on service:router will throw in those tests. This commit will trigger setupRouter when accessing EmberRouter via router service to avoid those errors.
xg-wang
force-pushed
the
non-application-router-test
branch
from
bf8311e
to
cba48c5
Compare
|
@rwjblue the tests are passing (they failed because timeout issue before) can you take a look? |
rwjblue
changed the title
|
Thanks @xg-wang! |
|
looks like documentation, mentioned |
|
@lifeart Good catch! I'm taking a look at how to update that page |
PR emberjs#19080 fixed the issue where accessing public router service throwing error, however, the PR did not support LinkTo component which uses the internal -routing service. This PR adds similar lazy setup to -routing.router.
This assertion is no longer valid because emberjs/ember.js#19080 changed the behavior in Ember.js so that rendering tests can have routing enabled too
This assertion is no longer valid because emberjs/ember.js#19080 changed the behavior in Ember.js so that rendering tests can have routing enabled too
This assertion is no longer valid because emberjs/ember.js#19080 changed the behavior in Ember.js so that rendering tests can have routing enabled too
PR emberjs#19080 fixed the issue where accessing public router service throwing error, however, the PR did not support LinkTo component which uses the internal -routing service. This PR adds similar lazy setup to -routing.router.
During non setupApplicationTests type tests, the Router being injected
into service:router and service:routing does not start routing, in which
it initializes its _routerMicrolib. Public API on service:router will
throw in those tests.
This commit will trigger startRouting on router, in non application
tests the router service should just work.
Original PR: #17557
I cannot reopen bc force pushed the branch...
This PR is created with rebase and few changes:
setupRouter(the semantics are a bit different,startRoutingwill do an initial transition which we don't necessarily want)non_application_test.jstonon_application_test_test.js(because testing usages in tests 馃樃)