test: improve ci e2e setup to include multiple browsers, optimize trace capturing

[dominikg]

just an experiment for now.

Please don't delete this checklist! Before submitting the PR, please make sure you do the following:

• It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
• This message body should clearly illustrate what problems it solves.
• Ideally, include a test that fails without this PR but passes with it.

Tests

• Run the tests with pnpm test and lint the project with pnpm lint and pnpm check

Changesets

• If your PR makes a change that should be noted in one or more packages' changelogs, generate a changeset by running pnpm changeset and following the prompts. All changesets should be patch until SvelteKit 1.0

[changeset-bot]

🦋 Changeset detected

Latest commit: 3218239

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@sveltejs/kit	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[dominikg]

see #5057 too

[benmccann]

how expensive is tracing?

[dominikg]

how expensive is tracing?

it records screenshots, network log, actions log, code refs and packs it all up in a neat zip which is viewable in an extra trace viewer either locally or on the web eg https://trace.playwright.dev/?trace=https://demo.playwright.dev/reports/todomvc/data/cb0fa77ebd9487a5c899f3ae65a7ffdbac681182.zip

gh action runners are notoriously unstable so comparing single run results isn't much of an indicator but here's one run with tracing https://github.com/sveltejs/kit/actions/runs/2380502348 that was slower on ubuntu-16 (but on-par in ubuntu-18 and windows).

to check it locally you can do KIT_E2E_TRACE=true pnpm run test --force vs pnpm run test --force not tracing made the local tests more unstable for me, it may have uncovered some tests that relied on timings from traceing

edit: using retries locally, on-first-retry took 80s vs 93s with on

[benmccann]

I'm not a big fan of dropping the tracing info as it gives us less information to track down flaky tests. If tracing were super expensive maybe it'd be worth it, but it doesn't seem to me like it's worth getting rid of all the extra info at this point

[dominikg]

I'm not a big fan of this as it gives us less information to track down flaky tests. If tracing were super expensive maybe it'd be worth it, but it doesn't seem to me like it's worth getting rid of all the extra info at this point

the tracing is still done, but only on the first retry, see here https://github.com/sveltejs/kit/suites/6642353303/artifacts/250778029

[benmccann]

the tracing is still done, but only on the first retry

Yeah, but it means we'll get tracing info less often and it will be much harder to discover and diagnose flaky tests. I'd think fixing the tests so that they don't need retries would speed things up a lot more than getting rid of the tracing info - especially since no tracing info will need to be generated and uploaded if the tests don't flake

[dominikg]

the comments here confuse me a bit. back called page.goBack internally as well, so if it is about the behavior of back(), that wasn't working.
Or is it just refering to the fact that the first one goes to an SPA page and the second one has to do load because it's not part of the SPA?

[mrkishi]

Yeah, they both should be similar, there's no concept of native vs client-side back. Plus, back() is using window.navigated which is not there.

[benmccann]

This may have been a holdover. At some point we switched whether we updated the URL before/after updating the page contents. I think the point was to wait until the page contents were updated and then we'd have the window.navigated property, but maybe that was removed during the switch. Rich might be able to confirm or you could look up the PR that made the switch to confirm (can't remember what the title was, but I'm pretty sure it was authored by Rich)

[dominikg]

there is one fail on firefox that may be an actual error. resetting the selection doesn't seem to work. When i opened the test project in firefox manually after clicking on 'b', getSelection() still returned a rangeCount: 1.

I did change the fake return value to -1 to ensure it's distinguisable from an actual unexpected rangeCount

[dominikg]

windows doesn't seem to correctly archive test results, likely the find or tar working differently :/

[dominikg]

@timothycohen you already did some work regarding safari testing in #4847

Any idea why they fail here?

[timothycohen]

Tough to say, because I can’t reproduce the failed CI test locally (macOS 12.4) and couldn't see the uploaded test artifacts.

1) FAIL macOS-latest [safari-dev+js] › test/test.js:224:2 › Scrolling › scroll is restored after hitting the back button for an in-app cross-document navigation Expected: < 10 Received: 818

Received 818 is the diff between the top of page and a Playwright default 720p screen scrolled to the link. Either

1. y1 is being evaluated before the scroll takes place
2. y1 is the scrolled value and y2 is evaluated before the scroll restoration after page.goBack()
3. the scroll restoration isn’t actually happening.

2 already has a waitForTimeout and I’m more inclined to believe it’s a flaky test than 3 if there hasn't been any issue raised. I’d like to try something like:

  const y1 = await page.evaluate(() => {
    document.querySelector('[href="/scroll/cross-document/b"]').scrollIntoView();
    return scrollY;
  });

I’ll see if I can set up the GitHub Actions and reproduce the failure locally.

2) FLAKY macOS-latest [safari-dev+js] › test/test.js:1673:2 › Page options › disables router if router=false Expected: "clicks: 1" Received: "clicks: 0"

This fails at the third expect statement 3/3 while testing the whole suite and 0/10 in isolation. Removing the .parallel on 'Page options' does nothing (no other test goes to /no-router/ anyway). I’m not sure why the await Promise.all isn’t working, but it seems like Playwright is actually clicking the button before the navigation finishes.

I tried adding the page route as an id to the button and then clicking/querying button#a and button#b, but it didn't work. Unfortunately, the only way I've found to make it pass consistently is adding either a fixed delay on mouseup or heavy handed timeout between the nav and click.

    await Promise.all([page.waitForNavigation(), page.click('[href="/no-router/b"]')]);
    expect(await page.textContent('button')).toBe('clicks: 0');
-   await page.click('button');
+   await page.click('button', { delay: 250 });
    expect(await page.textContent('button')).toBe('clicks: 1'); // fails here without delay/timeout

[timothycohen]

Also, I hit this locally

FLAKY macOS-latest [safari-build+js] › test/test.js:1844:2 › $app/stores › navigating store clears after aborted navigation

page.click: Target closed

=========================== logs ===========================
waiting for selector "a[href="/store/navigating/a"]"
selector resolved to visible <a href="/store/navigating/a">a</a>
attempting click action
waiting for element to be visible, enabled and stable
element is visible, enabled and stable
scrolling into view if needed
done scrolling
performing click action
============================================================

I wonder if the test should explicitly not wait for the /c nav which has a second delay in its load function.

5/5 pass with and 2/4 without

-    page.click('a[href="/store/navigating/c"]');
+    page.click('a[href="/store/navigating/c"]', { noWaitAfter: true });
     await page.waitForTimeout(100); // gross, but necessary since no navigation occurs
     page.click('a[href="/store/navigating/a"]');

[dominikg]

thank you for the detailed response!

rebased from master, artifacts from last run for mac: https://github.com/sveltejs/kit/suites/6717829409/artifacts/255850042

[benmccann]

I'd prefer to upload as many logs from flakey tests as possible. I don't think we're in danger of running out of space since old logs get cleared

[dominikg]

removed the exclusions. still believe this is wasteful but lets see how much space they claim.

[benmccann]

we should be avoiding waitForTimeout wherever possible. Is there another way this can be written? If not, let's at least add a comment explaining why not (but this far I believe we've only had one case where it's actually necessary)

[dominikg]

the wait is there to ensure native behavior for the preceding key press has finished. added a comment, no idea how to improve this other than polling for the expected result and giving up if that doesn't happen which would slow things down too