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

Lazily setup the router in non-application tests #19080

Merged
merged 6 commits into from
Oct 16, 2020

Conversation

xg-wang
Copy link
Contributor

@xg-wang xg-wang commented Aug 5, 2020

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:

  1. using setupRouter(the semantics are a bit different, startRouting will do an initial transition which we don't necessarily want)
  2. rename non_application_test.js to non_application_test_test.js (because testing usages in tests 😸)

@xg-wang xg-wang force-pushed the non-application-router-test branch 3 times, most recently from 41b87a7 to 5843603 Compare August 9, 2020 19:02
@xg-wang
Copy link
Contributor Author

xg-wang commented Aug 10, 2020

@rwjblue (friendly ping) can you take another look?

Comment on lines +479 to +489
if (instance) {
instance.didCreateRootView(this._toplevelView);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Created a RouterNonApplicationTestCase

packages/@ember/-internals/routing/lib/services/router.ts Outdated Show resolved Hide resolved
@rwjblue
Copy link
Member

rwjblue commented Sep 23, 2020

Thanks for pushing this forward @xg-wang!

@xg-wang xg-wang force-pushed the non-application-router-test branch from 24deea5 to bf8311e Compare October 2, 2020 23:48
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
Copy link
Contributor Author

xg-wang commented Oct 15, 2020

@rwjblue the tests are passing (they failed because timeout issue before) can you take a look?

@rwjblue rwjblue changed the title Lazy router setup in non-application tests Lazily setup the router in non-application tests Oct 16, 2020
@rwjblue rwjblue merged commit ab785b8 into emberjs:master Oct 16, 2020
@rwjblue
Copy link
Member

rwjblue commented Oct 16, 2020

Thanks @xg-wang!

@xg-wang xg-wang deleted the non-application-router-test branch October 19, 2020 18:58
@lifeart
Copy link
Contributor

lifeart commented Dec 29, 2020

looks like documentation, mentioned setupRouter should be updated too https://guides.emberjs.com/release/tutorial/part-2/route-params/

@xg-wang
Copy link
Contributor Author

xg-wang commented Dec 29, 2020

@lifeart Good catch! I'm taking a look at how to update that page

xg-wang added a commit to xg-wang/ember.js that referenced this pull request Dec 29, 2020
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.
Turbo87 added a commit to Turbo87/ember-link that referenced this pull request Jan 27, 2021
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
Turbo87 added a commit to Turbo87/ember-link that referenced this pull request Jan 27, 2021
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
Turbo87 added a commit to Turbo87/ember-link that referenced this pull request Jan 28, 2021
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
sly7-7 pushed a commit to sly7-7/ember.js that referenced this pull request Mar 10, 2021
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.
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.

3 participants