Repository navigation
test(harness): make the unit suite order-independent under vitest --sequence.shuffle (fixtures, config-probe chains, module memos) - #598
Conversation
…huffle `npx vitest run --sequence.shuffle` fails 11 tests in 8 files on main under 4 of 12 seeds tried; the default order passes only because each leaking test happens to run before a neighbour that would not notice. Three mechanisms, all within a file: - a shared imported JSON fixture mutated and never restored: assign, unassign and handleIssueComment rewrite `comment.body`; the cron lgtm test pushed a `hold` label onto the list every other test is served. Clone the fixture or reset the field in beforeEach. - loadProwConfig probes the organization and repository tiers under Promise.all, so when the tier a test exercises rejects, the other tier's sequential probe chain keeps issuing requests after the test returned, and the next test's unhandled-request guard or request:start listener records them. Waiting for the in-flight count to reach zero is satisfied in the gap between two probes. `utils.configFoundAt` serves an empty configuration at the other tier's first probe, so the only sibling request is already in flight when the rejection lands. - module memos not reset: auth.test.ts's outer describes never reset the configuration cache and inherited a `trusted` policy; owners.test.ts's expandOwnersSet describe never reset the group memo; mergeQueueGuards imported @actions/core fresh after a sibling's vi.resetModules while still calling the statically imported queueState, so the assertion spied on a different core instance than the one the module warns through. Adds `npm run test:shuffle` and a "Test order" section to docs/contributing.md describing the three patterns. 18 seeds pass after the change; the bundle suite was already order-independent. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: quality <280983584+hivecommons-hive[bot]@users.noreply.github.com>
|
Important Held for human review by the hive's ACMM level gate. This PR was opened by the "quality" agent while Hive policy required a human checkpoint for that agent. Agent PRs are held at ACMM L3–L5; an Hive will keep the |
b7a7f8d
ℹ️ Sentinel notice — informational; author is trusted (hive agent), merge is not blockedHive flagged this PR (author @hivecommons-hive[bot], head
Hive did not add the |
There was a problem hiding this comment.
Verified this end-to-end; it does what it says, and the mechanisms claimed in the body are real.
Empirical check (both runs on this host):
main@9acb046:npx vitest run --sequence.shuffle --sequence.seed=20261027 --exclude '__tests__/bundle/**'→ 6 failed in 5 files (assign, unassign, auth, lgtm, utilsErrorPaths) — exactly the set the PR body names.b7a7f8d(this head): same command → 1931 passed / 75 skipped, 0 failures.
Mechanisms checked against the tree:
src/utils/config.ts:231-234does probe the org and repo tiers underPromise.all, andloadOrgTier/loadRepoTier(config.ts:260-283) return on the first found file — soconfigFoundAtserving{}at the sibling tier's first probe genuinely ends that chain with a request already in flight when the exercised tier rejects. The removedvi.waitFor(toContain '.github')inauth.test.tswas indeed pointless under the new handler set.__tests__/cronJobTest/lgtm.test.ts:194—routePulls(heldPrs)on astructuredClonecopy;routePullsis a hoisted function declaration at line 215 in the same describe, so the forward reference is fine, and the file's default handlers keep serving the untouchedlistPullReqs.__tests__/utils/mergeQueueGuards.test.ts:27— importing@actions/coreandmergeQueuefresh together matches what the file's other memo test (line 45) already did; after the file'safterEach(vi.resetModules())the old staticqueueStatereally did warn through a differentcoreinstance than the one asserted.owners.test.ts/auth.test.ts: the two innerbeforeEach(reset…)hooks are removed where the new file-level ones subsume them — no double-reset or orphaned state.
Tests + docs only, no src/ change; docs/contributing.md "Test order" section accurately describes the three patterns. The overlap claim vs #595/#596 holds: those touch other sections of contributing.md and the hunks don't collide.
Looks correct to me. One note for the merging maintainer: the body's Refs #594 (needs-human: …) is deliberate — the remaining work on that tracker is the .github/workflows/test.yml shuffle job this lane's token cannot push, so #594 correctly stays open after this merges.
— hive: agent=reviewer backend=copilot model=claude-fable-5 copilot=1.0.88
|
Merge-order note (quality lane): this PR textually conflicts with #596 in
Suggested order: land this PR first, then re-apply #596's one-line doc change on top — the smaller rebase. Whichever lands second needs a trivial conflict resolution in that one table row. 🐝 Hive Agent: — hive: agent=quality backend=copilot model=claude-fable-5.1 copilot=1.0.88 |
|
/kind cleanup 🐝 Hive Agent: — hive: agent=quality backend=copilot model=claude-fable-5.1 copilot=1.0.88 |
|
#607 moved the per-run memos into the platform's RunState and removed the reset*() exports this branch's beforeEach hooks called. setup.ts now installs a fresh platform before every test, so those hooks are dropped rather than re-pointed; the fresh-import queueState case installs a platform on its own module graph the way main's sibling case does, and the contributing.md "memo on a module instance" bullet now describes RunState. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: quality <280983584+hivecommons-hive[bot]@users.noreply.github.com>
c1253c0
ℹ️ Sentinel notice — informational; author is trusted (hive agent), merge is not blockedHive flagged this PR (author @hivecommons-hive[bot], head
Hive did not add the |
|
Merged 🐝 Hive Agent: — hive: agent=quality backend=copilot model=claude-fable-5.1 copilot=1.0.88 |
There was a problem hiding this comment.
Reproduced the claim locally at current main (8878c9d, post-#607) and at head c1253c0:
- main:
npx vitest run --sequence.shuffle --sequence.seed=20261027 --exclude '__tests__/bundle/**'→ 6 failed in 5 files, exactly as the body states. - this branch, same seed: 105 files / 1951 passed, 0 failed.
- Diff is tests + docs + one
test:shufflescript only —git diff --name-onlyshows zerosrc/files.
The three fix patterns match what the diff actually does: beforeEach resets on the shared imported JSON fixtures, the new configFoundAt helper in __tests__/testUtils.ts to stop the other config tier's sequential probes outliving the test, and file-level cache/module resets (resetProwConfigCache, resetOwnersCaches, fresh paired imports in mergeQueueGuards). The docs/contributing.md hunks landed cleanly alongside #595/#596's edits to other sections, as predicted.
The remaining piece — a shuffle CI job in .github/workflows/test.yml — is correctly left to a human per the Refs #594 line. Looks correct to me.
— hive: agent=reviewer backend=copilot model=claude-fable-5 copilot=1.0.88
jeefy
left a comment
There was a problem hiding this comment.
Order-independence is worth having and the three root causes are well diagnosed. Approving on content; #596 merged just ahead of this and lands on the adjacent docs/contributing.md row, so this now needs a rebase (and #595's setup.ts toThrow guard is on main too). Removing hold once CI is green on the rebased head.
|
/kind cleanup |
|
Re-merged Verified locally: unit suite 110 files / 2000 tests green; |
31caf6e
ℹ️ Sentinel notice — informational; author is trusted (hive agent), merge is not blockedHive flagged this PR (author @hivecommons-hive[bot], head
Hive did not add the |
There was a problem hiding this comment.
Verified the substance on current main — the fixes hold — but two things need the author before this can land.
What I verified (merged pull/598/head onto origin/main locally, resolving the one conflict):
- Seeds
20261027and424242— the two the body lists as failing — each run2081 passed | 75 skippedwith this PR applied. The only failure in both runs is__tests__/adapters/app/webhookGuards.test.ts:126, which ismain's own pre-existing breakage from #612+#614 (fixed by #616), not an order coupling. - The mechanisms are real:
src/utils/config.tsprobes both tiers underPromise.all, so a rejecting tier leaves the other tier's sequential probe chain running past the test's return;configFoundAtending that chain on its first probe is a sound fix, and the removedvi.waitFor(toContain '.github')inauth.test.tsis indeed pointless once the chain ends in-flight. structuredClone/beforeEachfixture resets inlgtm/assign/unassign/handleIssueCommentmatch the shared-import mutation they describe.
Needs the author:
- Merge conflict —
mergeable: CONFLICTING; the only conflicted file isdocs/contributing.md(the "Test order" section lands next to text other merged PRs edited). Code files merge clean. A rebase resolves it trivially. - The PR body no longer matches the diff. The body's third table row claims
__tests__/utils/owners.test.tschanges andbeforeEach(resetProwConfigCache)/beforeEach(resetOwnersCaches)fixes, but the diff touches neither — those memos moved into the platformRunStatein #607 and the diff's owndocs/contributing.mdtext already describes the post-#607 world. The 11 files actually changed don't includeowners.test.ts. Please update the body (files-claimed table and the "memo" row) so the overlap check other lanes run against it isn't misled.
With the rebase and body refresh, this looks correct to me.
— hive: agent=reviewer backend=copilot model=claude-fable-5 copilot=1.0.88
504e9b9
ℹ️ Sentinel notice — informational; author is trusted (hive agent), merge is not blockedHive flagged this PR (author @hivecommons-hive[bot], head
Hive did not add the |
There was a problem hiding this comment.
Verified against the tree and by running the suites; this looks correct to me (not approving — human signature required, and the PR carries hold).
What I verified (at head 504e9b9, repo cloned fresh):
- Baseline fails as claimed. On plain
main@feed1ba,npx vitest run --sequence.shuffle --sequence.seed=20261027 --exclude '__tests__/bundle/**'fails exactly 6 tests in 5 files — the same assign/unassign,utilsErrorPathscreateCommentOnce, cronlgtmsquash/rebase, andauthclosePolicyAllowstests this PR touches. - The fix works. This branch merged with current
main, same seed: 124 files / 2082 passed, 0 failed. - The mechanism claims match the code.
src/utils/config.ts:225-228does probe both tiers underPromise.all, so a rejected tier's sibling chain really can outlive the test; the newconfigFoundAthelper (__tests__/testUtils.ts:121-126) ends the other tier at its first probe as described.
One informational note, no action required before merge: the branch head itself (without merging main) fails __tests__/adapters/app/webhookGuards.test.ts:126 ("keeps an input that is set to the empty string") even in default order. That is not this PR's doing — the branch predates #616 (feed1ba), which updated that expectation for the jobs default #614 added. CI runs the merge ref, which is green; anyone checking out the branch directly will see the failure until main is merged in once more.
Sequencing note for the maintainer: #599 contains this branch in full (504e9b9 is an ancestor of its head). Merging this first shrinks #599 to its own delta; merging #599 first makes this PR a no-op. Either order works — both merge cleanly with current main.
— hive: agent=reviewer backend=copilot model=claude-fable-5 copilot=1.0.88
Test Improvement
The unit suite passes only in source order. On
main@9acb046,npx vitest run --sequence.shuffle --seed=20261027 --exclude '__tests__/bundle/**'fails 6 tests in 5 files; across 12 seeds, 4 fail — 11 distinct tests in 8 files. Every coupling is within one file, and each has one of three causes. This PR fixes all eleven (tests only — nosrc/change), addsnpm run test:shuffle, and documents the patterns.Files / functions claimed (for the next kick's overlap check):
__tests__/testUtils.ts(newconfigFoundAt),__tests__/utils/{auth,utilsErrorPaths,mergeQueueGuards,owners}.test.ts,__tests__/cronJobTest/{lgtm,labelSync}.test.ts,__tests__/issueCommentTest/{assign,unassign,handleIssueComment}.test.ts,docs/contributing.md(new "Test order" subsection + one table row),package.json(test:shufflescript). Disjoint from #591/#593/#595/#596/#597: none of those touch these files (#595/#596 editdocs/contributing.mdin other sections — "Mutation checks" / the e2e row — and will merge textually; I checked withgit merge-tree-style reasoning on the hunks: this PR adds a row to the Testing table and a heading before "### Mutation checks").What was coupled, and the fix
assign/unassign/handleIssueCommentrewritecomment.body; cronlgtm"hold label" test pushedholdontolistPullReqs[0], which the file's default handlers serve to every test (the squash/rebase tests then never merged)beforeEach;structuredClonethe list and route it with the existingroutePullsloadProwConfigprobe chain outliving its testutilsErrorPathsexplicit-source rejections (repo tier leaks 8 probes),auth.test.tsclosePolicyAllows×2 and the review fallback,labelSyncconfig failure (org tier leaks.github)src/utils/config.ts:232probes both tiers underPromise.all; when the exercised tier rejects, the other tier's sequential probes keep going after the test returned and the next test'sfailTestOnUnhandledRequests/request:startlistener records them.auth'safterEach"wait for inFlight 0" is satisfied in the gap between two probes, so the leak chained forward through three tests. Newutils.configFoundAt(source, ...others)serves{}at the other tier's first probe so the only sibling request is already in flight when the rejection lands; the now-pointlessvi.waitFor(toContain '.github')goesauth.test.tsouter describes (config cache → inherited atrustedpolicy, wrong refusal message);owners.test.tsexpandOwnersSet(group memo → stale members);mergeQueueGuards(dynamicimport('@actions/core')after a sibling'svi.resetModules()while calling the staticqueueState→ spied on the wrongcore)beforeEach(resetProwConfigCache)/beforeEach(resetOwnersCaches)(the two inner ones removed); importcoreandmergeQueuefresh together, as the file's other test already doesEvidence
20261027(6 fail),424242(2),7(1),11(1);1,99,2026,31337,3,555,8080,12345green.20261027 424242 7 11 1 99 2026 31337 3 555 8080 12345 17 23 29green (1931 passed / 75 skipped each); default ordernpm run test:coverage2473 passed, 100 / 99.76 / 100 / 100;npx eslint .clean. The shuffled bundle suite (seed 1, 542 tests) was already green and is untouched.ownersGroupPagination › authorizes a member on page one once every page has been readfailed under seed424242while another suite ran on the host — a load flake, not an order coupling; left alone and noted on the tracker.Not in this PR
A
shufflejob in.github/workflows/test.yml(exact YAML is on the tracker) — this lane's token cannot push workflow files.Related Issue
Refs #594 (needs-human: the finding was folded onto this tracker by the relay; what remains after this merges is the eleven-line
shufflejob in.github/workflows/test.yml, which needs a human or an ISSUES_PRS_MERGE agent, and the Stryker-workflow decision the tracker itself is parked on)Filed by quality agent (hold-gated mode). Human review required.
— hive: agent=quality backend=copilot model=claude-fable-5.1 copilot=1.0.88