test/apps/basics is non-deterministic

[GrygrFlzr]

Describe the bug
I've encountered this twice now modifying unrelated files on two different branches.

Logs

CI logs

> test-basics@0.0.2 test /home/runner/work/kit/kit/test/apps/basics
> node test

dev
 dev  [snowpack] ! building dependencies...
[snowpack] ✔ dependencies ready! [1.09s]
  http://localhost:3001 • http://10.1.0.4:3001
  Server started.


• ✘ • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • •   (101 / 102)

   FAIL  dev  "announces client-side navigation [js]"
    Expected values to be deeply equal:  (equal)

        ++Navigated·to·b    (Expected)
        --                  (Actual)
          ^^^^^^^^^^^^^^
    at assert (file:///home/runner/work/kit/kit/node_modules/.pnpm/uvu@0.6.0-next.1/node_modules/uvu/assert/index.mjs:31:8)
    at Module.equal (file:///home/runner/work/kit/kit/node_modules/.pnpm/uvu@0.6.0-next.1/node_modules/uvu/assert/index.mjs:43:2)
    at file:///home/runner/work/kit/kit/test/apps/basics/src/routes/accessibility/__tests__.js:41:11
    at async Object.handler (file:///home/runner/work/kit/kit/test/runner.js:113:7)
    at async Number.runner (file:///home/runner/work/kit/kit/node_modules/.pnpm/uvu@0.6.0-next.1/node_modules/uvu/dist/index.mjs:78:5)
    at async Module.exec (file:///home/runner/work/kit/kit/node_modules/.pnpm/uvu@0.6.0-next.1/node_modules/uvu/dist/index.mjs:132:33)
    at async runner (file:///home/runner/work/kit/kit/test/runner.js:180:2)

To Reproduce
Unknown. It's some sort of race condition in the basics test.

Expected behavior
Deterministic behavior.

Additional context
First encountered in #402, then in #403. It's most likely related to the fact that the test involves an await sleep(50):

kit/test/apps/basics/src/routes/accessibility/__tests__.js

Lines 39 to 41 in 67ddeea

	await click('[href="/accessibility/b"]');
	await sleep(50);
	assert.equal(await html('[aria-live]'), 'Navigated to b'); // TODO i18n

Strongly related to #213.

[Rich-Harris]

We replaced all the

page.click('[href="xyz"]')

calls with

Promise.all([page.waitForNavigation(), page.click('[href="xyz"]')])

and I think that was probably a mistake. SvelteKit changes the URL in the URL bar as soon as you click, it doesn't wait for the page to actually update. (Whether this is correct is debatable, but I personally prefer this behaviour — it provides instant feedback, and means that if the user Cmd-Rs because navigation is taking too long they'll end up where they wanted to be.) Sure enough, we're still seeing flakiness in those tests.

We probably need something more bespoke — an SvelteKit-namespaced event that fires on window once navigation has happened, maybe, or some globally accessible promise that is created when navigation starts and resolves when it ends. (This could be done in userland via the page store, but it would have to live in all the test apps; maybe it belongs in Kit itself?) If we had such a thing, we could create a click helper that lived in the test context and wrapped page.click.

[Conduitry]

Cypress automatically retries assertions until they're true, with some sensible timeout. This makes a lot of these types of concerns go away, and without any framework-specific code. I'm not saying switch to Cypress, but I am saying I'd be a bit happier with solving this by retrying in the tests than in having something else in the app that's exposed only for tests.

[Conduitry]

https://playwright.dev/docs/api/class-page#pagewaitforselectorselector-options sounds like it lets you wait for a selector to match something. The default timeout is 30 seconds, which sounds like a great way for your tests to take a very long time before they fail, but this is configurable.

[Rich-Harris]

Yeah I definitely don't think we should switch to Cypress — 'solving' indeterminism by trying again until you succeed might be okay for testing an app (though I don't love the overhead that it presumably introduces) but seems like a dangerous precedent for a framework's tests.

I too would prefer that we not have things that exist solely for our own tests. What about other people's tests though? If someone else were to build an app with SvelteKit and they wanted to test it with Playwright instead of Cypress, it might be useful if they also had a way to (for example) ensure that a navigation had taken effect before making assertions. It probably warrants more discussion than just this issue thread, but it feels like something worth tossing around?

I don't love waitForSelector — the selector that you wait for a) needs to exist on the destination page but not the source page, and b) will be unique for each test, so you can't just blindly do await click('[href="whatever"]'). IOW it adds significant complexity

[Conduitry]

While we can create something to hook into for page navigation (since Kit will know when load is done and when the new page component has been created with those props), but there are going to be a lot of other async interactions that are impossible for the framework to know about. I guess your position is that it's better to solve what we can and have an uneven experience than it is to throw up our hands and make people do the annoying thing all the time?

[benmccann]

Before / after hooks for page navigation is something we need for the framework as a user-facing feature anyway: #552

Although we may already have some support using the page store: https://gist.github.com/Jayphen/dc2db7591cd0735cefa586b2f5facacf

[Rich-Harris]

I guess your position is that it's better to solve what we can and have an uneven experience than it is to throw up our hands and make people do the annoying thing all the time?

Basically yeah. If something is specific to an app, then it needs to be solved in userland, but things like router init and navigation start/end feel like things that can be handled in a consistent way.

It could be as simple as

window.dispatchEvent(new CustomEvent('sveltekit:start'));
window.dispatchEvent(new CustomEvent('sveltekit:navigationstart'));
window.dispatchEvent(new CustomEvent('sveltekit:navigationend'));

Before / after hooks for page navigation is something we need for the framework as a user-facing feature anyway: #552

holy shit that discussion got long

[Rich-Harris]

i'm not going to wade back into that hellthread, but it strikes me that we could solve that problem by exposing a lifecycle hook from `$app/navigation':

<script>
  import { onMount } from 'svelte';
  import { onNavigate } from '$app/navigation';

  onMount(() => {
    // runs once when the page mounts
  });

  onNavigate(() => {
    // runs when the page mounts, and also whenever SvelteKit
    // navigates to a new URL but stays on this component
  });
</script>

[Conduitry]

Would we want to allow onNavigate on layout pages as well? Or on other non-page components? Is that practical?

[Rich-Harris]

I'm imagining it would be implemented like this:

export function onNavigate(fn) {
  let mounted = false;

  const unsubscribe = getStores().page.subscribe(() => {
    if (mounted) fn();
  });

  onMount(() => {
    mounted = true;
    fn();

    return () => {
      unsubscribe();
      mounted = false;
    }
  });
}

There might be some nuance around ensuring that the callback doesn't run immediately before the component is unmounted, but you get the gist. This would work in any component (layout or otherwise) like a normal lifecycle function, including components created after the page was initially created (e.g. lazily-loaded stuff).

[Conduitry]

And the tests were never flaky again, the end

[GrygrFlzr]

Farewell, angry red cross, I hope I never see you again.