fix(test): stop two gates depending on the host and the process interleaving - #2993
Merged
Merged
Conversation
…ory root `check:tmpdir-leaks:test` runs four files in one `node --test` invocation, which parallelizes per file by default. Two of them sweep the real shared root: node-test-tmpdir.test.ts asserts `pruneAbandonedRunDirectories(TEST_RUN_TMP_ROOT)` both keeps and removes a planted directory, and vitest-tmpdir-global-setup.test.ts drives a real `vitest` whose global setup performs that same sweep. One file's sweep can therefore delete the run directory the other is mid-flight on, and the orphan test's "the next run prunes it" assertion observes `[]` instead of the directory. It passed 5/5 locally and failed on a CI runner with exactly that `actual: []`; the outcome depends on process interleaving, not the code. Forward `--test-concurrency=1` (cost measured at +2s: 3.4s -> 5.5s) and pin it. The pin checks the flag sits among the args the wrapper forwards to its child, not merely anywhere in the command: the wrapper spawns `node <forwarded...> --test`, so a flag written before the wrapper path is parsed by the wrapper's own node process and never reaches the runner, leaving the lane silently parallel. Verified that misplaced form fails the new test.
…wner The orphan-cleanup test recorded PID 101 and asserted the reaper cleared the record. 101 is only reliably dead on a fresh CI runner: on a Mac with a simulator runtime mounted it is `appleaccountd`, so the reaper inspects a live process, reads it as ownership-lost, retains the record rather than clearing it, and the test fails on that machine while staying green on CI. Use the `NEVER_A_PID` idiom the tmpdir lanes already use — INT32_MAX is outside every platform's pid range, so the owner is dead by construction on every host and cannot be recycled mid-test. That is also the branch the test means to exercise: a recorded owner that already exited, hence a stale record that must be cleared.
|
Size Report
Startup median (7 runs, lower is better):
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 3 files
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
…string
Review found the pin's `includes('--test-concurrency=1')` accepted two shapes
that still run files in parallel: `--test-concurrency=10`, and the flag sitting
in a later `&&` segment of a command whose sweep segment never serializes. Split
the command the way the wrapper ratchet above already does, read the tokens the
wrapper forwards to its child, and require exactly one that equals `--test-concurrency=1`.
A wrapper path that is not a standalone token reports the lane unsynchronized
rather than guessing.
Mutations verified to fail the pin: concurrency `10`, flag removed, flag moved
before the wrapper path, a `--test-concurrency=1` decoy in a later segment, a
duplicated flag, and a `./`-prefixed wrapper path.
Soften the NEVER_A_PID comment to the claim the code can support. Windows pids
are 32-bit, so INT32_MAX does not sit outside every platform's pid range by
construction; it sits above any pid a real host mints in practice, which is what
makes the recorded owner absent here.
Member
Author
|
Reviewed at 338e807. I found no blocking issues. All 22 checks pass, and the change is test and tooling only. I checked the serialization with an equivalent standalone repro and ran the new regression predicate against the old script line and six mutated forms. I did not run the gate against this PR's package.json, and I did not check the softened INT32_MAX claim on a win32 host. Not blocking: the comment near https://github.com/callstack/agent-device/blob/338e807/scripts/node-test-tmpdir.test.ts#L259 still makes the old universal INT32_MAX claim that this PR corrects in agent-browser-lifecycle.test.ts. Could a follow-up reword it to match? |
thymikee
added
the
ready-for-human
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
1 participant
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.
Summary
Two test-isolation defects where results depended on the machine or the process interleaving, both flagged during #2986.
check:tmpdir-leaks:test(the Repo Guards red). Four files run in onenode --testinvocation, parallelized per file by default. Two sweep the real shared root:node-test-tmpdir.test.tsassertspruneAbandonedRunDirectories(TEST_RUN_TMP_ROOT)both keeps and removes a planted directory, andvitest-tmpdir-global-setup.test.tsdrives a realvitestwhoseglobalSetupsweeps that same root. One file's sweep can delete the directory the other is mid-flight on, so it observes[]. It passed 5/5 locally and failed on CI with exactly thatactual: []. Fix: forward--test-concurrency=1(+2s: 3.4s → 5.5s), pinned by a test requiring the flag among the forwarded args — it spawnsnode <forwarded…> --test, so a flag before the wrapper path is parsed by the wrapper and leaves the lane silently parallel.agent-browser-lifecycle.test.ts(red on any Mac with a simulator runtime). It recorded PID 101 and asserted the reaper cleared it. PID 101 isappleaccountdthere, so the reaper reads ownership-lost and retains the record: green on CI, red locally. Fix: theNEVER_A_PIDidiom the tmpdir lanes already use, which is also the branch the test means to exercise.The iOS smoke flake is not addressed here; see the #2986 thread for why it needs the Mac lane.
Validation
9b9d184.check:affected --runpassed;test:unit11396 passed 0 failed (that file failed here before the fix);check:tmpdir-leaks:test25/25;check:gate-manifest:test,depgraph:test,check:layering,check:quickexit 0. The pin fails on the previouspackage.jsonand on the misplaced-flag form.