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

Failing test case for lazily loaded engines with loading states #19266

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

rwjblue
Copy link
Member

@rwjblue rwjblue commented Nov 12, 2020

Creates a separate set of tests that leverage async loaded routes (roughly akin to what ember-engines does) to ensure model loading and loading states work correctly in that scenario.

Currently, this emits the following error:

TypeError: Cannot read property '_stashNames' of undefined
    at stashParamNames (http://localhost:7021/tests/ember.js:23926:13)
    at Class.setup (http://localhost:7021/tests/ember.js:20365:37)
    at _routeEnteredOrUpdated (http://localhost:7021/tests/ember.js:62215:17)
    at PrivateRouter.routeEnteredOrUpdated (http://localhost:7021/tests/ember.js:62230:9)
    at PrivateRouter.setupContexts (http://localhost:7021/tests/ember.js:62159:16)
    at PrivateRouter.getTransitionByIntent (http://localhost:7021/tests/ember.js:61969:14)
    at PrivateRouter.transitionByIntent (http://localhost:7021/tests/ember.js:61905:21)
    at PrivateRouter.doTransition (http://localhost:7021/tests/ember.js:62040:19)
    at PrivateRouter.intermediateTransitionTo (http://localhost:7021/tests/ember.js:62522:19)
    at Class.intermediateTransitionTo (http://localhost:7021/tests/ember.js:22389:28)

This was introduced by fixing another issue (that the original state was not preserved when doing intermediated transitions) over in:

Those changes don't directly cause the issue, but the issue was masked due to the bug that they fixed.


This affects Ember 3.22.1 (and current 3.23 betas).

@rwjblue rwjblue added the Bug label Nov 12, 2020
@rwjblue rwjblue force-pushed the failing-test-case-for-engines branch from 163d69e to 022375c Compare November 12, 2020 22:27
@scalvert
Copy link
Contributor

@rwjblue any idea why that job is failing? I tried to parse the logs but they're making my eyes bleed...

@sly7-7
Copy link
Contributor

sly7-7 commented Mar 5, 2021

@rwjblue I've forked this branch and rebase against master. https://github.com/sly7-7/ember.js/tree/failing-test-case-for-engines

I turns out there are 2 failing tests. The first one is resolved by adding an undefined check in the router microlib
Capture d’écran 2021-03-05 à 11 10 41

I think it's legit, because after reading a bit the router code, I find out that when route are lazily loaded, they can be undefined at first (seems it's like this by design);

Concerning the second failing test, I have doubt, if either it's a brittle test, or if the bug is in the router.

If I understand correctly your goal when adding those tests, you would simulate routes lazy loading. In this case, I guess this involves some kind of asynchronous behavior. But seeing those three lines:

https://github.com/sly7-7/ember.js/blob/failing-test-case-for-engines/packages/ember/tests/routing/model_loading_test.js#L827

I think we should be waiting for the async process to finish before sending the second and third actions.
I've tried to use the await runLoopSettled();, but either it's not enough, or not the right 'waiter', or a bug in the router.

Capture d’écran 2021-03-05 à 11 24 21

As you can see, the first call to editPost triggers the lazy load of the edit route, but there is a missing 'waiter' somewhere, so I think the transition is aborted by the 'showPost' action. After that, the second call to edit post works, because this time, the route has been loaded.

So at this point, I'm just confused, because I don't know if it's a bug, or if the test should be adapted to take the async behavior in account.

@sly7-7
Copy link
Contributor

sly7-7 commented Mar 5, 2021

Ok, so there is definitely some kind of mess with asynchrony. The test now "passes", but probably for wrong reason, because I must call await runLoopSettled(); twice after the first call to editPost().
I've also replace the expectDeprecation()by expectDeprecationAsync which seems legit because of the route lazy loading.
You can see the diff here: sly7-7@c4b4d3b

@sly7-7
Copy link
Contributor

sly7-7 commented Mar 9, 2021

@rwjblue Unless I'm missing something, this is the thing landed in 3.25.3 right ?

@rwjblue
Copy link
Member Author

rwjblue commented Mar 9, 2021

I think we still need to add the guard in router_js setupContexts (from your screenshot).

Creates a separate set of tests that leverage async loaded routes
(roughly akin to what ember-engines does) to ensure model loading and
loading states work correctly in that scenario.

Currently, this emits the following error:

```
TypeError: Cannot read property '_stashNames' of undefined
    at stashParamNames (http://localhost:7021/tests/ember.js:23926:13)
    at Class.setup (http://localhost:7021/tests/ember.js:20365:37)
    at _routeEnteredOrUpdated (http://localhost:7021/tests/ember.js:62215:17)
    at PrivateRouter.routeEnteredOrUpdated (http://localhost:7021/tests/ember.js:62230:9)
    at PrivateRouter.setupContexts (http://localhost:7021/tests/ember.js:62159:16)
    at PrivateRouter.getTransitionByIntent (http://localhost:7021/tests/ember.js:61969:14)
    at PrivateRouter.transitionByIntent (http://localhost:7021/tests/ember.js:61905:21)
    at PrivateRouter.doTransition (http://localhost:7021/tests/ember.js:62040:19)
    at PrivateRouter.intermediateTransitionTo (http://localhost:7021/tests/ember.js:62522:19)
    at Class.intermediateTransitionTo (http://localhost:7021/tests/ember.js:22389:28)
```
@rwjblue rwjblue force-pushed the failing-test-case-for-engines branch from 022375c to 695b2bf Compare March 9, 2021 14:50
@rwjblue
Copy link
Member Author

rwjblue commented Mar 9, 2021

I've rebased against master (fixing up some merge conflict issues).

@rwjblue
Copy link
Member Author

rwjblue commented Mar 9, 2021

Also, FWIW, it is easiest to review without whitespace (https://github.com/emberjs/ember.js/pull/19266/files?diff=split&w=1) because I re-indented a bunch of the tests...

@rwjblue
Copy link
Member Author

rwjblue commented Mar 9, 2021

I think we still need to add the guard in router_js setupContexts (from your screenshot).

I think that will fix this test:

Test failed: Route - model loading (simulated within lazy engine):  Nested callbacks are not exited when moving to siblings", source:  (49)
[0309/145618.057284:INFO:CONSOLE(52)] "    Failed assertion: Promise rejected during " Nested callbacks are not exited when moving to siblings": Cannot convert undefined or null to object
TypeError: Cannot convert undefined or null to object
    at PrivateRouter.setupContexts (http://localhost:13141/tests/ember.js:64725:9)
    at PrivateRouter.finalizeTransition (http://localhost:13141/tests/ember.js:64653:14)
    at http://localhost:13141/tests/ember.js:64592:21
    at invokeCallback (http://localhost:13141/tests/ember.js:65801:17)

@sly7-7
Copy link
Contributor

sly7-7 commented Mar 9, 2021

@rwjblue Yeah, I'm into that right now, and sorry, I thought I had pushed a PR in the router.js.
Done here: tildeio/router.js#322

sly7-7 added a commit to sly7-7/ember.js that referenced this pull request Mar 10, 2021
sly7-7 added a commit to sly7-7/ember.js that referenced this pull request Mar 10, 2021
@kategengler
Copy link
Member

Is this still relevant? It is a draft and seems to have a successor PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants