Skip to content

Lazy router setup in non-application tests#17557

Closed
xg-wang wants to merge 2 commits intoemberjs:masterfrom
xg-wang:non-application-router-test
Closed

Lazy router setup in non-application tests#17557
xg-wang wants to merge 2 commits intoemberjs:masterfrom
xg-wang:non-application-router-test

Conversation

@xg-wang
Copy link
Contributor

@xg-wang xg-wang commented Feb 4, 2019

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. Calling 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™.

Link tested on xg-wang/__router-test@df28714. It fails on travis-ci.

Fixes #16831

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.
@rwjblue
Copy link
Member

rwjblue commented Feb 4, 2019

I’m not sure this is the right direction. IMHO, tests that need the router (e.g. those testing generated href’s or things using the router service) should call this.owner.setupRouter() explicitly in the test.

@xg-wang xg-wang force-pushed the non-application-router-test branch from 463e8fd to 6d3f888 Compare February 4, 2019 06:00
@xg-wang
Copy link
Contributor Author

xg-wang commented Feb 4, 2019

Can you explain the reason behind this? I think if using router service in a component is not discouraged the test experience should also be out-of-box.

@xg-wang xg-wang closed this Jul 28, 2019
@xg-wang xg-wang deleted the non-application-router-test branch July 28, 2019 22:52
@rwjblue
Copy link
Member

rwjblue commented Aug 4, 2020

Hmm, @xg-wang I think I've made a mistake here, sorry for the double speak (between #16831 (comment) and my comments here). What do you think about reviving these changes?

@nlfurniss
Copy link
Contributor

nlfurniss commented Aug 4, 2020

Is it better to handle this here or to call this.owner.setupRouter() in setupRenderingTest?

@rwjblue
Copy link
Member

rwjblue commented Aug 4, 2020

Is it better to handle this here or to call this.owner.setupRouter()insetupRenderingTest`?

IMHO, most rendering tests aren't going to need the router. So eagerly setting it up in all tests is a bit wasteful.

@xg-wang xg-wang restored the non-application-router-test branch August 5, 2020 14:07
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.

RouterService.urlFor causes error in non-application tests.

3 participants