Repository navigation
test(analytics): contract test for shared scope-stats fetches - #55
Merged
Merged
Conversation
…l contract test
The old test asserted "exactly one api('/scope-stats' call exists",
which was true when written but broke when a second, legitimate call
site (Foreign Traffic tab, analytics.js ~6148) was added that
deliberately reuses the Scopes tab's cached response rather than
duplicating the request. A source-text occurrence count can't tell a
real duplicate fetch apart from two call sites correctly sharing one
cache entry.
Replaces it with two independent checks: a structural check with no
call-count ceiling (every /scope-stats call site uses the correct
relative form, however many exist), and a behavioral check that drives
both real production entry points (registerPage('analytics').init and
window._analyticsRenderForeignTrafficTab) through six caching/dedup/
retry scenarios against the real, unmodified api() helper, using
production analytics.js/app.js loaded via vm rather than copying their
logic into the test.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Kpa-clawbot#1375 Removes the global `process.on('unhandledRejection', ...)` handler that blanket-logged and swallowed any unhandled rejection during this file's run. This test's Part 3 scenarios load the real, unmodified public/app.js via vm, so they exercise api()'s actual `promise.finally(() => _inflight.delete(path))` line -- when the underlying request rejects (e.g. scenario F's induced 500), the discarded `.finally()` promise rejects too with nothing observing it, which is a genuine unhandled rejection that will crash the process under Node's default behavior. That crash depends entirely on whether PR #53's fix to that line (`.catch(() => {})`) is present in the tree. Replaced the suppression with a short comment documenting this as a real, intentional cross-branch dependency instead of hiding it: running this file against `codex/fix-1375-scope-stats-test` alone (without PR #53) is expected to crash for exactly this reason, and that is correct, not a bug in this test. Also added a minimal completion guarantee mirroring the reviewed pattern already landed in the sibling file test-app-api-inflight-cleanup-rejection.js: `process.exitCode` is set to 1 up front and only flipped to 0 after every scenario A-F is confirmed to have run (tracked via a `ranScenarios` set populated in checkAsync's `finally` block) and all of them passed. No watchdog timer was added -- scenario D's existing pre-resolve `assert.strictEqual(midCalls.length, 1, ...)` already throws synchronously before the one hang-prone `Promise.all()` in this file's scenarios is ever reached, so a broader rewrite wasn't warranted. Verified via a synthetic (non-merge) integration tree combining this change with PR #53's HEAD tree (3406159): 10x runs of this file and 3x runs of the sibling file both reached and passed all scenarios (6/6 and 8/8) under `node --unhandled-rejections=strict`, four negative controls confirmed the intended failure modes (PR #53 reverted -> genuine crash with the documented signature; doubled-prefix and cache-sharing regressions -> real structural/behavioral test failures, not crashes; broken dedup -> the completion guarantee prevents a silent clean exit 0), and the integration tree's own CI JS test sequence (from .github/workflows/deploy.yml) reached and passed both this file and the sibling file in their declared order before hitting an unrelated, pre-existing failure later in the sequence (test-issue-1648-m4-emoji-scan.js, unrelated to this change). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
test-issue-1375-scope-stats-fetch.jsguards against a historical bug in upstream issue 1375: the Scopes tab calledapi('/api/scope-stats…'), producing a doubled/api/api/…URL. The old test also required exactly one staticapi('/scope-stats'call inpublic/analytics.js. That was an accident of the file's state when the test was written. A second, legitimate consumer was added later: the Foreign Traffic tab deliberately reuses the Scopes tab's cached/scope-stats?window=24hresponse. So the test has been failing ever since on correct code (found 2), even though the two call sites share one cache entry and do not duplicate requests.What changes
Test-only: the only changed file is
test-issue-1375-scope-stats-fetch.js. No production code, workflow or test registration changes.api('/api/scope-stats'is unchanged. The count check is replaced by "every/scope-statscall site uses the relative form, however many there are, and at least one exists". Every captured call is checked, not only calls that already look correct.public/app.js(api()with its real TTL cache and in-flight dedup) and the realpublic/analytics.js. Both real entrypoints are driven: Scopes via the analytics pageinit, and Foreign Traffic viawindow._analyticsRenderForeignTrafficTab. Onlyfetchand DOM boundaries are stubbed, and exact fetch counts and URLs are asserted:process.on('unhandledRejection', …)handler is gone, with no replacement handler, flag, broad catch or stderr filtering.process.exitCode = 1is set first. Exit 0 requires all six scenarios to have completed and zero failures. A scenario that never settles leaves the exit code at 1.Dependency on #53 (already merged)
Scenario F induces a 500. Before #53,
api()discarded the promise returned by.finally(), so a failed request also produced an orphaned unhandled rejection, which crashes Node. #53 fixed that inpublic/app.jsand is merged intomaster(merge commit24760c3d). This PR relies on that fix and does not include or change it. Without #53's fix, this test is expected to crash in scenario F withAPI 500: /scope-stats?window=24h; that was verified deliberately.Verification (local, not GitHub CI)
On the merge of this branch with current
master, computed in isolation: conflict-free, tree8eae5d8ea89845934bf01698dee111f433071b34, and exactly this one file differs frommaster.--unhandled-rejections=strict, outer time limit:Ran: A, B, C, D, E, F (6/6), exit 0, empty stderr;test-app-api-inflight-cleanup-rejection.js:8/8, exit 0.node --checkandgit diff --check: clean./api/apiprefix reintroduced: structural checks and scenarios A–D fail;deploy.yml) passedtest-app-api-inflight-cleanup-rejection.jsand this test, then stopped at command 55 of 67 on the known baseline failure below.Known baseline failures and limitations
test-issue-1648-m4-emoji-scan.js(upstream issue 1648) fails on unmodifiedmasterbecause of a▾in a comment atpublic/map.js:681. Its output is identical onmaster24760c3dand on this merge. It is not fixed or bypassed here, so the JS unit-test step is still expected to fail at that point, and later steps and jobs will be skipped.test-live-dedup.js(needs Playwright), the fulltest-all.sh, and the 12 JS-step commands after the emoji-scan failure.test-frontend-helpers.jshas two knownfavStarfailures onmaster, unrelated to this change.node …and sets noNODE_OPTIONS.🤖 Generated with Claude Code