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

Lazy router setup in non-application tests #17557

Closed
wants to merge 2 commits into from

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