From 5ea1d834f26b287c63272d0523413077f2393f2e Mon Sep 17 00:00:00 2001 From: Robert Jackson Date: Wed, 9 Sep 2020 12:22:57 -0400 Subject: [PATCH 1/5] Failing test for `TransitionAborted` via `router.transitionTo` within async hook. --- tests/index.ts | 1 + tests/native-async-test.ts | 110 +++++++++++++++++++++++++++++++++++++ tests/test_helpers.ts | 2 +- 3 files changed, 112 insertions(+), 1 deletion(-) create mode 100644 tests/native-async-test.ts diff --git a/tests/index.ts b/tests/index.ts index 2a032d8e..731f704b 100644 --- a/tests/index.ts +++ b/tests/index.ts @@ -7,3 +7,4 @@ import './transition_intent_test'; import './transition_state_test'; import './unrecognized-url-error_test'; import './utils_test'; +import './native-async-test'; diff --git a/tests/native-async-test.ts b/tests/native-async-test.ts new file mode 100644 index 00000000..d0d69dc7 --- /dev/null +++ b/tests/native-async-test.ts @@ -0,0 +1,110 @@ +import Router, { Route, Transition } from 'router'; +import { Dict } from 'router/core'; +import { createHandler, TestRouter } from './test_helpers'; + +function sleep(ms: number) { + return new Promise((resolve) => setTimeout(resolve, ms)); +} + +QUnit.module('native async', function (hooks) { + let router: Router; + let url: string | undefined; + let routes: Dict; + + class LocalRouter extends TestRouter { + routeDidChange() {} + routeWillChange() {} + didTransition() {} + willTransition() {} + + getRoute(name: string) { + if (routes[name] === undefined) { + routes[name] = createHandler('empty'); + } + + return routes[name]; + } + + getSerializer(_name: string) { + return undefined; + } + + replaceURL(name: string) { + this.updateURL(name); + } + updateURL(newUrl: string) { + url = newUrl; + } + } + + hooks.beforeEach(() => { + router = new LocalRouter(); + }); + + QUnit.test('returning a transition does not reject with TransitionAborted', async function ( + assert + ) { + assert.expect(3); + + router.map(function (match) { + match('/').to('application', function (match) { + match('/').to('index'); + match('/about').to('about'); + }); + }); + + routes = { + index: createHandler('index', { + beforeModel(_params: Dict, _transition: Transition) { + assert.step('index beforeModel'); + + return router.transitionTo('/about'); + }, + }), + + about: createHandler('about', { + setup() { + assert.step('about setup'); + }, + }), + }; + + await router.handleURL('/'); + + assert.equal(url, '/about', 'ended on /about'); + + assert.verifySteps(['index beforeModel', 'about setup']); + }); + + QUnit.test( + 'returning a promise that resolves to a transition (which resolves) does not reject', + async function (assert) { + assert.expect(1); + + router.map(function (match) { + match('/').to('application', function (match) { + match('/').to('index'); + match('/about').to('about'); + }); + }); + + routes = { + index: createHandler('index', { + async beforeModel(_params: Dict, _transition: Transition) { + await sleep(5); + + return router.transitionTo('/about'); + }, + }), + + about: createHandler('about', { + setup: function () { + assert.ok(true, 'setup was entered'); + }, + }), + }; + + return router.handleURL('/'); + } + ); +}); diff --git a/tests/test_helpers.ts b/tests/test_helpers.ts index 2fdba80a..7abf522c 100644 --- a/tests/test_helpers.ts +++ b/tests/test_helpers.ts @@ -57,7 +57,7 @@ function transitionTo( path: string | { queryParams: Dict }, ...context: any[] ) { - let result = router.transitionTo.apply(router, [path, ...context]); + let result = router.transitionTo(path, ...context); flushBackburner(); return result; } From 512836cee6a6c056f4981ed446fb8bfa7ed53f41 Mon Sep 17 00:00:00 2001 From: Stefan Penner Date: Tue, 10 Nov 2020 13:04:43 -0700 Subject: [PATCH 2/5] tidy-up test --- tests/native-async-test.ts | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/tests/native-async-test.ts b/tests/native-async-test.ts index d0d69dc7..4f7bb74a 100644 --- a/tests/native-async-test.ts +++ b/tests/native-async-test.ts @@ -7,22 +7,23 @@ function sleep(ms: number) { } QUnit.module('native async', function (hooks) { - let router: Router; - let url: string | undefined; - let routes: Dict; + let router: LocalRouter; class LocalRouter extends TestRouter { + routes: Dict = Object.create(null); + url: string | undefined; + routeDidChange() {} routeWillChange() {} didTransition() {} willTransition() {} getRoute(name: string) { - if (routes[name] === undefined) { - routes[name] = createHandler('empty'); + if (this.routes[name] === undefined) { + this.routes[name] = createHandler('empty'); } - return routes[name]; + return this.routes[name]; } getSerializer(_name: string) { @@ -32,8 +33,9 @@ QUnit.module('native async', function (hooks) { replaceURL(name: string) { this.updateURL(name); } + updateURL(newUrl: string) { - url = newUrl; + this.url = newUrl; } } @@ -53,7 +55,7 @@ QUnit.module('native async', function (hooks) { }); }); - routes = { + router.routes = { index: createHandler('index', { beforeModel(_params: Dict, _transition: Transition) { assert.step('index beforeModel'); @@ -71,7 +73,7 @@ QUnit.module('native async', function (hooks) { await router.handleURL('/'); - assert.equal(url, '/about', 'ended on /about'); + assert.equal(router.url, '/about', 'ended on /about'); assert.verifySteps(['index beforeModel', 'about setup']); }); @@ -88,7 +90,7 @@ QUnit.module('native async', function (hooks) { }); }); - routes = { + router.routes = { index: createHandler('index', { async beforeModel(_params: Dict, _transition: Transition) { await sleep(5); From 1d5fdc3841fad5cc29e32522422a994d1b49f16c Mon Sep 17 00:00:00 2001 From: Stefan Penner Date: Tue, 10 Nov 2020 13:13:24 -0700 Subject: [PATCH 3/5] Only `async function` is required to replicate this issue, sleep + extra promise is not. --- tests/native-async-test.ts | 6 ------ 1 file changed, 6 deletions(-) diff --git a/tests/native-async-test.ts b/tests/native-async-test.ts index 4f7bb74a..2cbe0525 100644 --- a/tests/native-async-test.ts +++ b/tests/native-async-test.ts @@ -2,10 +2,6 @@ import Router, { Route, Transition } from 'router'; import { Dict } from 'router/core'; import { createHandler, TestRouter } from './test_helpers'; -function sleep(ms: number) { - return new Promise((resolve) => setTimeout(resolve, ms)); -} - QUnit.module('native async', function (hooks) { let router: LocalRouter; @@ -93,8 +89,6 @@ QUnit.module('native async', function (hooks) { router.routes = { index: createHandler('index', { async beforeModel(_params: Dict, _transition: Transition) { - await sleep(5); - return router.transitionTo('/about'); }, }), From 3ea8a785f9da19723d58814506821e0bc8250aa6 Mon Sep 17 00:00:00 2001 From: Stefan Penner Date: Tue, 10 Nov 2020 13:13:29 -0700 Subject: [PATCH 4/5] consistency --- tests/native-async-test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/native-async-test.ts b/tests/native-async-test.ts index 2cbe0525..686242b9 100644 --- a/tests/native-async-test.ts +++ b/tests/native-async-test.ts @@ -100,7 +100,7 @@ QUnit.module('native async', function (hooks) { }), }; - return router.handleURL('/'); + await router.handleURL('/'); } ); }); From 67a7868f12bdfd75360d637ff2fe093e628f8292 Mon Sep 17 00:00:00 2001 From: Stefan Penner Date: Tue, 10 Nov 2020 13:15:09 -0700 Subject: [PATCH 5/5] improve test name clarity --- tests/native-async-test.ts | 53 +++++++++++++++++--------------------- 1 file changed, 24 insertions(+), 29 deletions(-) diff --git a/tests/native-async-test.ts b/tests/native-async-test.ts index 686242b9..a905791b 100644 --- a/tests/native-async-test.ts +++ b/tests/native-async-test.ts @@ -39,9 +39,7 @@ QUnit.module('native async', function (hooks) { router = new LocalRouter(); }); - QUnit.test('returning a transition does not reject with TransitionAborted', async function ( - assert - ) { + QUnit.test('beforeModel with returning router.transitionTo', async function (assert) { assert.expect(3); router.map(function (match) { @@ -74,33 +72,30 @@ QUnit.module('native async', function (hooks) { assert.verifySteps(['index beforeModel', 'about setup']); }); - QUnit.test( - 'returning a promise that resolves to a transition (which resolves) does not reject', - async function (assert) { - assert.expect(1); + QUnit.test('async beforeModel with returning router.transitionTo', async function (assert) { + assert.expect(1); - router.map(function (match) { - match('/').to('application', function (match) { - match('/').to('index'); - match('/about').to('about'); - }); + router.map(function (match) { + match('/').to('application', function (match) { + match('/').to('index'); + match('/about').to('about'); }); + }); - router.routes = { - index: createHandler('index', { - async beforeModel(_params: Dict, _transition: Transition) { - return router.transitionTo('/about'); - }, - }), - - about: createHandler('about', { - setup: function () { - assert.ok(true, 'setup was entered'); - }, - }), - }; - - await router.handleURL('/'); - } - ); + router.routes = { + index: createHandler('index', { + async beforeModel(_params: Dict, _transition: Transition) { + return router.transitionTo('/about'); + }, + }), + + about: createHandler('about', { + setup: function () { + assert.ok(true, 'setup was entered'); + }, + }), + }; + + await router.handleURL('/'); + }); });