book deploy: retry the flaky redirect check, and upload the Playwright report on failure

[dscho]

The scheduled update-book.yml workflow rebuilds the ProGit book and, in its push-updates job, deploys to GitHub Pages through the deploy-to-github-pages composite Action, which runs the Playwright smoke tests against the freshly deployed site. A recent scheduled run failed there, and the failure was needlessly hard to diagnose because, unlike the pull-request workflow in ci.yml, the deploy Action never uploaded the Playwright report that embeds the failure screenshots.

The first commit closes that observability gap. The report's upload-artifact step is guarded by if: always() && steps.playwright.outputs.result != '', but the Playwright step in the Action never wrote anything to $GITHUB_OUTPUT, so the guard was always false and the report was silently dropped. The PR workflow avoids this by writing result=$PLAYWRIGHT_TEST_URL before running the tests; this mirrors that, so the deploy job now leaves the report behind on failure.

The second commit fixes the failure itself. The book test navigates to /book/ and expects to land on /book/en/v2, but that redirect is purely client-side: Hugo emits /book/index.html as a stub whose only content is <meta http-equiv="refresh" content="0; url=../book/en/v2">. When that zero-delay refresh fires, Chrome occasionally aborts the load that page.goto() started and strands the page on its internal error page, chrome-error://chromewebdata/. Because that is a committed navigation, the existing expect(page).toHaveURL(...), which only polls the URL, can never recover. This is a known transient Playwright/Chromium issue. The fix wraps the navigate-and-assert in expect(async () => { ... }).toPass(), which re-triggers the navigation until the expected URL is reached.

In the interest of full disclosure: the chrome-error is a live-network race that does not reproduce against a local localhost build, so I could not exercise it directly. The refactor was verified only to not regress the normal pass, by running the full suite against a local Hugo build before and after the change with identical results.

[dscho]

@To1ne @jvns could I bother any of you for a review?

[jvns]

what's the timeout on this?

edit: the docs say "by default toPass has timeout 0 and does not respect custom expect timeout.", should we set an explicit timeout?

https://playwright.dev/docs/test-assertions#expecttopass

[dscho]

@jvns good point. The docs say:

Note that by default toPass has timeout 0 and does not respect custom expect timeout.

We should probably use something like this instead:

  // Retry every second for up to a minute in total
}).toPass({ intervals: [1_000], timeout: 60_000 });

What do you think?

[jvns]

maybe start with a shorter timeout, in case there are some actual failures that need to be caught more quickly? I don't feel strongly though

[dscho]

@jvns here's the...

range-diff

• 1: aa7ed1d ! 1: 7da7e6f tests: retry the book redirect to dodge "chrome-error://chromewebdata/"

  @@ tests/git-scm.spec.js: test.afterEach(async ({ page }, testInfo) => {
   +  await expect(async () => {
   +    await page.goto(gotoURL)
   +    await expect(page).toHaveURL(expectedURL)
  -+  }).toPass()
  ++    // Retry every second for up to 15 seconds in total
  ++  }).toPass({ intervals: [1_000], timeout: 15_000 });
   +}
   +
    test('generator is Hugo', async ({page}) => {

[jvns]

I find the commit messages kind of hard to read (there's some jargon like "committed navigation" that reminds me of how LLMs talk that's hard for me to understand, and it feels more verbose than it needs to be)

i think the messages could be a lot more concise, like for instance for the first one:

When there are test failures like in https://github.com/git/git-scm.com/actions/runs/28149818103, the Playwright report isn't uploaded.

This is because upload-artifact step is guarded by if: always() && steps.playwright.outputs.result != '', but the Playwright step in the Action never wrote anything to $GITHUB_OUTPUT, so steps.playwright.outputs.result was always the empty string.

This writes result=$PLAYWRIGHT_TEST_URL before running the tests to make the Playwright report upload.

[dscho]

When there are test failures like in https://github.com/git/git-scm.com/actions/runs/28149818103, the Playwright report isn't uploaded.

This is because upload-artifact step is guarded by if: always() && steps.playwright.outputs.result != '', but the Playwright step in the Action never wrote anything to $GITHUB_OUTPUT, so steps.playwright.outputs.result was always the empty string.

This writes result=$PLAYWRIGHT_TEST_URL before running the tests to make the Playwright report upload.

You're right, @jvns, I did let LLM write those messages (while working on some Git for Windows v2.55.0 release stuff, MinGit-BusyBox is currently quite broken, for example).

I've now reworded both commits (taking your suggestion verbatim). Good?

[jvns]

yep, those are so much better, thank you!