From 6c6a5a5f3986867d6477a1da7efc8ea85402736f Mon Sep 17 00:00:00 2001 From: clawbot <35+clawbot@noreply.example.org> Date: Tue, 6 Oct 2026 04:55:11 +0000 Subject: [PATCH] Make the load-sensitive tests wait for what they check (closes #507) The browser test waits for each page a click opens to load, and for Alpine.js to start on it, before reading that page; before, a read could find an element of the page being left. The delivery tests' drain takes what is queued without a timer. The dispatch paths queue before they return, and the 25 ms timer could be due by the time select looked, which then chose at random between it and a queued task. The test phase keeps the tests' temporary directories, and with them their SQLite databases, on a tmpfs: waiting for the disk at each commit was about 40% of internal/handlers' run time on a busy host. Model: opus-5-5 --- Dockerfile | 9 ++++++- internal/delivery/inflight_test.go | 7 +++--- internal/server/alpine_browser_test.go | 34 +++++++++++++++++++------- 3 files changed, 37 insertions(+), 13 deletions(-) diff --git a/Dockerfile b/Dockerfile index 7a61e9a..c52265d 100644 --- a/Dockerfile +++ b/Dockerfile @@ -147,9 +147,16 @@ RUN script/assets # shown in full, so there is nothing to rerun. The step fails after the rerun # whatever its result: the first run already showed the suite is broken. # +# TMPDIR, where the tests keep their SQLite databases, is a tmpfs: SQLite +# waits for the disk at every commit, and on a busy host that waiting was +# about 40% of the slowest package's run time. GOTMPDIR keeps go's own +# build files, the test binaries among them, on disk. +# # bash with pipefail, so that the first run's status is go test's, not tee's. SHELL ["/bin/bash", "-o", "pipefail", "-c"] -RUN go test -race -cover -p 4 -parallel 8 -timeout 90s ./... 2>&1 | tee /tmp/go-test.log && exit 0; \ +RUN --mount=type=tmpfs,target=/tmp/tests,size=512m \ + export TMPDIR=/tmp/tests GOTMPDIR=/tmp; \ + go test -race -cover -p 4 -parallel 8 -timeout 90s ./... 2>&1 | tee /tmp/go-test.log && exit 0; \ tests="$(awk '/^--- FAIL: / { print $3 }' /tmp/go-test.log | paste -s -d '|' -)"; \ packages="$(awk '/^FAIL\t/ { print $2 }' /tmp/go-test.log)"; \ if [ -n "$tests" ]; then \ diff --git a/internal/delivery/inflight_test.go b/internal/delivery/inflight_test.go index 08a0f69..81ad665 100644 --- a/internal/delivery/inflight_test.go +++ b/internal/delivery/inflight_test.go @@ -48,8 +48,9 @@ func fSweepSetup( // // Every caller drives the dispatch paths synchronously and has already // waited for them to return, so anything they queued is in the channel -// by now. The short grace covers nothing but scheduler jitter, and is -// kept small because one of these tests runs the drain forty times. +// by now, and nothing is waited for. A timer here would race the queued +// tasks: on a busy host it can be due by the time select looks, and +// select picks at random among the cases that are ready. func fDrain(e *delivery.Engine) []delivery.Task { var out []delivery.Task @@ -59,7 +60,7 @@ func fDrain(e *delivery.Engine) []delivery.Task { out = append(out, task) case task := <-e.ExportRetryCh(): out = append(out, task) - case <-time.After(25 * time.Millisecond): + default: return out } } diff --git a/internal/server/alpine_browser_test.go b/internal/server/alpine_browser_test.go index 367e0f9..b865083 100644 --- a/internal/server/alpine_browser_test.go +++ b/internal/server/alpine_browser_test.go @@ -280,6 +280,23 @@ func click(ctx context.Context, t *testing.T, xpath string) { )) } +// clickAndLoad clicks the link or button matching an XPath expression +// and waits, as loadPage does, for the page the click opens to load and +// for Alpine.js to start on it. Reading earlier, a check can find an +// element of the page being left, gone by the time its value is read; +// and the wait in shown is too short for a page load on a busy host. +func clickAndLoad(ctx context.Context, t *testing.T, xpath string) { + t.Helper() + + _, err := chromedp.RunResponse( + ctx, chromedp.Click(xpath, chromedp.BySearch), + ) + require.NoError(t, err) + require.NoError(t, chromedp.Run( + ctx, chromedp.WaitNotPresent("[x-cloak]", chromedp.ByQuery), + )) +} + // checkAddEntrypoint loads a webhook page and checks that the add // entrypoint form stays hidden until the Add button beside its heading // is clicked. The click looks for a button element there, so it also @@ -422,7 +439,7 @@ func checkAddTarget( ))) } - click(ctx, t, saveButton) + clickAndLoad(ctx, t, saveButton) assert.Truef(t, shown(ctx, `//span[text()="`+name+ `"]/following-sibling::div/span[text()="`+badge+`"]`), "%s: the added target is not listed as %s", targetType, badge) @@ -485,10 +502,9 @@ func checkArchiveChoices(ctx context.Context, t *testing.T, url string) { `/following-sibling::span[text()="daily"]`), "a database target added with daily is not listed as daily") - click(ctx, t, row+`//a[text()="Edit"]`) + clickAndLoad(ctx, t, row+`//a[text()="Edit"]`) require.NoError(t, chromedp.Run( ctx, - chromedp.WaitReady("#expiry", chromedp.ByQuery), chromedp.Value("#expiry", &editedExpiry, chromedp.ByQuery), chromedp.Value("#rotation", &editedRotation, chromedp.ByQuery), )) @@ -523,7 +539,7 @@ func checkRefusedTarget(ctx context.Context, t *testing.T, url string) { chromedp.Click(forwardQuery, chromedp.ByQuery), )) - click(ctx, t, saveButton) + clickAndLoad(ctx, t, saveButton) assert.True(t, shown(ctx, reason), "a refused target does not show the reason") @@ -636,7 +652,7 @@ func checkRefusedEdits( )) } - click(ctx, t, `//button[text()="Save Changes"]`) + clickAndLoad(ctx, t, `//button[text()="Save Changes"]`) assert.Truef(t, shown(ctx, reason), "%s: a refused save does not show the reason", edit.url) @@ -760,7 +776,7 @@ func checkEntrypointEdit( require.NoError(t, chromedp.Run( ctx, chromedp.SendKeys(input, "Billing sender", chromedp.ByQuery), )) - click(ctx, t, saveEdit) + clickAndLoad(ctx, t, saveEdit) assert.True(t, shown(ctx, `//span[text()="Billing sender"]`), "saving the edit form does not change the description") @@ -798,7 +814,7 @@ func checkRecentEvents(ctx context.Context, t *testing.T, url string) { "clicking the newest event does not collapse it") require.NoError(t, chromedp.Run(ctx, loadPage(url))) - click(ctx, t, newest+`/ancestor::div[@x-data][1]//a[text()="Open"]`) + clickAndLoad(ctx, t, newest+`/ancestor::div[@x-data][1]//a[text()="Open"]`) assert.True(t, shown(ctx, `//h2[text()="Body"]`), "Open does not lead to the event's own page") @@ -1199,7 +1215,7 @@ func checkNewWebhookTargets( `","rotation":"none"}` } - click(ctx, t, createButton) + clickAndLoad(ctx, t, createButton) require.Truef(t, shown(ctx, `//h1[text()="`+name+`"]`), "%s: the new webhook's page does not open", name) @@ -1260,7 +1276,7 @@ func checkRefusedNewWebhook(ctx context.Context, t *testing.T, url string) { chromedp.SetValue(pruningChoice, "2160h", chromedp.BySearch), chromedp.SetValue("#archive_rotation", "monthly", chromedp.ByQuery), )) - click(ctx, t, createButton) + clickAndLoad(ctx, t, createButton) assert.True(t, shown(ctx, `//div[@class="alert-error"]`), "a refused webhook does not show the reason")