Skip to content

Commit 1c1ed26

Browse files
sbesh91clauderschristian
committed
Fix test suite compatibility with Navigation API router (#135)
* fix: Handle traversal navigation when canIntercept is false For back/forward traversals, canIntercept may be false when the destination entry was created via history.replaceState, but the URL changes regardless. Only gate on canIntercept for push/replace navigations. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * test: Remove non-configurable window.location stub sinon cannot stub window.location in modern browsers as it is non-configurable and non-writable. The stub was a leftover from before the Navigation API refactor — location.assign/replace are no longer used in the router or tests. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * test: Fix _top/_parent navigation crashing WTR in CI WTR runs tests inside an iframe. Clicking links with target="_top" or target="_parent" targets the WTR runner frame (canIntercept=false), so calling e.intercept() unconditionally throws and lets the navigation escape — crashing the entire test session. Fix by: - Checking canIntercept before calling e.intercept() in test handlers - Skipping _top/_parent tests (like _blank) since they can't be meaningfully tested in an iframe context Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * test: Remove unused sinon afterEach cleanup sinon.restore() in afterEach was only needed for the window.location stub which was removed. The remaining sinon.spy (scrollTo) is restored manually. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * revert: Undo traversal/canIntercept handling that broke tests Reverts the isTraversal guard and conditional e.intercept() from router.js, and restores the original test handler behavior. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix: Gate navigation intercept on canIntercept check Move canIntercept to the top of the guard conditions so navigations that cannot be intercepted (e.g. back/forward to history.replaceState entries) are skipped cleanly without calling e.intercept(). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * test: Revert scope handler to unconditional intercept Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * test: Skip navigation tests, add a big ol' TODO --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com> Co-authored-by: Ryan Christian <rchristian@ryanchristian.dev>
1 parent b5431e0 commit 1c1ed26

File tree

1 file changed

+9
-12
lines changed

1 file changed

+9
-12
lines changed

test/router.test.js

Lines changed: 9 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,6 @@ describe('Router', () => {
4141
history.replaceState(null, null, '/');
4242
});
4343

44-
4544
it('should throw a clear error if the LocationProvider is missing', () => {
4645
const Home = () => <h1>Home</h1>;
4746

@@ -611,16 +610,14 @@ describe('Router', () => {
611610
});
612611
}
613612

614-
// TODO: These tests are rather flakey, for some reason the additional handler
615-
// occasionally isn't running prior browser actually navigates away.
613+
// TODO: Need a better way to test these. We're currently racing the browser and trying to observe
614+
// the navigation event after is has been ignored by the router, but before any navigation occurs,
615+
// which has been incredibly flakey. Not to mention, wtr runs in an iframe so `_top` and `_parent`
616+
// wouldn't act as expected either (cheers to @sbesh91 for that).
617+
//
618+
// For now, we can skip, though it'd be good to get a better option in place.
616619
for (const target of shouldNavigate) {
617-
it(`should allow default browser navigation for links with ${getName(target)}`, async () => {
618-
// Currently cross-window navigations, (e.g., `target="_blank"`), do not trigger a
619-
// `navigate` event, which makes this difficult to observe. Per the spec, however, this
620-
// might be a bug in Chrome's implementation:
621-
// https://github.com/WICG/navigation-api?tab=readme-ov-file#restrictions-on-firing-canceling-and-responding
622-
if (target === '_blank' || target === '_BLANK' || target === 'custom') return;
623-
620+
it.skip(`should allow default browser navigation for links with ${getName(target)}`, async () => {
624621
scratch.querySelector(`#${createId(target)}`).click();
625622
await sleep(1);
626623
expect(triedToNavigate).to.be.true;
@@ -989,12 +986,12 @@ describe('Router', () => {
989986

990987
expect(loc).to.deep.include({ url: '/foo', path: '/foo', searchParams: {} });
991988

992-
navigation.back();
989+
await navigation.back().finished;
993990
await sleep(10);
994991

995992
expect(loc).to.deep.include({ url: '/', path: '/', searchParams: {} });
996993

997-
navigation.forward();
994+
await navigation.forward().finished;
998995
await sleep(10);
999996

1000997
expect(loc).to.deep.include({ url: '/foo', path: '/foo', searchParams: {} });

0 commit comments

Comments
 (0)