test: route test scratch through mkdtempForTest; forbid node:os in tests - #2629
Conversation
|
Reviewed at dea914c. The migration looks sound, but the new gate has three holes that let raw
The scan at The allowlist at The "allowlist holds" test in A few cleanup leftovers: On design: this adds the 11th and 12th copy of the two-line mkdtemp helper, and the gate then finds helpers by file name. Would exempting only functions named The branch conflicts with main. #2618 already removed the snapshot-helper The 5 reported checks pass, and the new gate runs in the tooling job. Next step: rebase, then close the three gate holes with model tests that fail without each fix. |
dea914c to
6a17e00
Compare
thymikee
changed the title
Size Report
Startup median (7 runs, lower is better):
|
6a17e00 to
853f612
Compare
|
Reviewed at 6a17e00, as a follow-up to the review at dea914c. Banning the The enforced glob at oxlint.config.ts#L152 matches only The override for the 9 exempt tests at oxlint.config.ts#L174 clears A small leftover: session-close-shutdown.fixtures.ts#L3 imports All checks passed at 6a17e00 and there were no conflicts. The newer head 853f612 rebases onto main and adapts one fixture to an upstream mock; it does not change the lint config or the files named above, so these points still apply. Next: widen the glob and scope the exemptions to the justified calls. |
|
Addressed in Gate reworked per your design note. The bespoke
Cleanup leftovers fixed. Removed the On the 11th/12th helper copy — agreed it is duplication, but a single shared root test-surface helper importable by each package Validation at |
|
Checked at 853f612. Yes, please widen the ban to the helper files: The other two points from the 6a17e00 review still apply to this head. The nine exemptions at oxlint.config.ts#L171-L179 clear the rule for whole files, so a new unrelated The shared helper for all packages is fine as a follow-up. All checks pass and there are no conflicts. Next: widen the glob, scope the exemptions to lines, and drop the unused import. |
Route every product-test scratch directory through the per-package mkdtempForTest/mkdtempForTestSync helpers so it lands under the run's redirected TMPDIR and is removed once per run, instead of path.join(os.tmpdir(), name), which reuses a fixed path across the whole suite. Adds the missing tmp-dir helpers to platform-harmonyos, provider-webdriver, and replay-test and rounds out platform-apple's.
853f612 to
23c99eb
Compare
|
Rebased onto latest
Validation at this head: |
Add a no-restricted-imports override so a product *.test.ts that imports node:os is a lint error pointing at mkdtempForTest, mirroring the existing node:child_process ban. A follow-up override clears it for the handful of tests that mock production's os.tmpdir()/os.homedir() or assert a real /tmp socket.
23c99eb to
a605029
Compare
|
Reviewed at a605029, as a follow-up to the review at 853f612. The scratch routing looks good, but the new oxlint does not merge Can the Smoke Tests are still running; they exercise the device path, which this diff does not touch. There are no conflicts. Next: keep the earlier bans in the new overrides. |
…:os override matches oxlint doesn't merge no-restricted-imports options across overrides: when several overrides match a file, the last match's options replace the earlier ones instead of accumulating. The new PRODUCT_TEST_FILES override (node:os ban) and the tmp-dir exemption override both matched files that were already covered by the child_process/provider bans (fixtures.ts and test-utils files under src/**, packages/host-kit/src/**, and the host-kit tmp-dir helper), silently dropping those bans for those files. Compose every no-restricted-imports override from shared path/pattern constants so overlapping overrides restate the full union of bans that should apply, instead of one override's options clobbering another's.
|
Pushed a7dc020 to fix the lost import bans. Each override that touches |
|
Reviewed at a7dc020. The a605029 finding is fixed: the CI is green and there are no conflicts, so this is ready for human review. |
thymikee
added
the
ready-for-human
|
Rebasing onto main picked up the ban on node:os in tests (#2629's mkdtempForTest routing), and this test's own scratch dir predates that change. Route it through the same helper so the branch's own new test stays lint-clean; behavior is unchanged, only where the temp directory comes from.
) * fix(ios-snapshot): prepare the AX bridge off the capture deadline A cold host pays for the AX bridge inside the capture that happens to ask for it first, so the first capture of a session spent its whole deadline in a toolchain probe or a clang build and the lane lost the `wait` that was polling for a screen (#2491). Preparation is now a detached single-flight per runtime: the first capture that finds it running waits out a short budget and is served by the XCTest runner, the build keeps going for whoever asks next, and a failed attempt is answered as-is until a retry window measured from the failure expires. The grant is once per attempt rather than once per capture. A `wait` poll cycles every 200 ms or so, and a budget paid per poll would cost a cold build more in captured polls than the build itself costs, with every one of those captures ending up on the runner anyway. Both this and the pending target discovery in `snapshot-target.ts` are the same shape — one attempt per key, detached from whoever started it, a bounded wait, a typed answer while it runs — and they had already started to forget failures differently. One seam owns that shape; each owner declares its own wait budget, wait grant, retry window and pending error, and keeps its own cache of a finished value, because only the owner knows when that value stops being valid. The preparation carries the signal `close()` aborts. Once a capture has answered, no request owns this attempt any more: without that, `xcrun` could keep running for two minutes past shutdown and its cache write would land in a directory belonging to a source that is gone. * fix(ios-snapshot): end a detached wait the answer already settled `value()` raced the attempt against the caller's wait and left the loser running. A capture answered by the attempt still held its timer and abort listener until the budget ran out — 1.5 s for discovery, 2 s for preparation — so a poll loop could stack one pending timer and one listener per capture. The `awaitDiscovery` it replaced cleared both in a `finally`. `wait` now takes a per-call stop signal that `value()` aborts in a `finally`, and both owners release their timer and listener on it: `waitForDiscoveryAttempt` resolves, and `waitForSnapshotSourceDelay` grows an optional `stop` that ends the sleep without spending the deadline it is measured against. Waiting inside the caller's own deadline still matters, so a client abort keeps rejecting with the typed cancellation; the stop only ever lands after the race is already decided, which is why the losing wait resolves rather than rejecting, and why the race keeps a handler of its own. * test(ios-snapshot): prove the discovery wait stops when the discovery settles The stop path inside `waitForDiscoveryAttempt` had no test: the listener registration, the listener removal and the already-aborted check could each be deleted with every suite green. `snapshot-target.test.ts` now runs a deferred spawn through `createSimulatorSnapshotTargetResolver`, lets a second caller join the pending discovery, settles it, and asserts on the two things a leaked wait costs — a timer still pending and a listener still on the caller's `AbortSignal` — with fake timers, so neither is a timing race. `deadline.test.ts` measures the same two properties for the preparation owner and drops its already-aborted case. That case was one of the things the review asked about, and it is unreachable rather than untested: `value()` creates the stop moments before calling and aborts it in a `finally`, so the check can never be true and the stop's `{ once: true }` listener is always released by that abort. Both waits now drop the check and the redundant removal, and keep the cleanup that does matter, which is the caller's own signal: it outlives the wait, and forgetting it would leave a listener per capture. * test(ios-snapshot): count the discovery wait's listeners the way the type says * refactor(ios-snapshot): build a detached wait once, and read the retirement rule where it applies Three reviewers in a row had to be satisfied about the same promise plumbing because it existed twice: the discovery wait and the bridge-preparation wait each hand-rolled the timer, the caller's abort listener, the stop listener and the cleanup that keeps a leak from costing a timer and a listener per capture. `waitForDetachedAttempt` now implements the wait that `value()`'s contract describes, and both owners hand it their own sleep length and their own cancellation error — which also means the invariant is deleted-and-caught in one place instead of two. `disableGenerationFor` said one thing about one call site, so the rule moves inline next to the set it edits: a failed bridge retires the app generation, a bridge that is merely still building does not. * fix(ios-snapshot): remove the stop listener when a detached wait settles waitForDetachedAttempt added an abort listener to stop but only ever removed the one on signal, so every call with no explicit stop leaked one listener onto the module-level NO_STOP signal per production caller (lifecycle.ts's bridge-connect retry sleep and every bridge request's waitForSimulatorTurn). Make stop optional, remove its listener on every settle path, and delete NO_STOP now that the wait tolerates a missing stop directly. * test(ios-snapshot): end a wait on an already-aborted stop and drop a case that cannot fail waitForDetachedAttempt only checked the caller signal for an already-aborted case; a pre-aborted `stop` would sit until waitMs expired instead of ending at once. Add the same already-aborted check for `stop`, guarded so the first settle wins if both signals happen to be aborted together. deadline.test.ts's "a delay with no stop leaves no listener behind" case only exercised the caller signal and the timer, both of which the pre-fix code already cleaned up correctly, so it could never fail. Delete it; the detached-attempt.test.ts cases pin the actual leak. * fix(ios-snapshot): route preparation test scratch through mkdtempForTest Rebasing onto main picked up the ban on node:os in tests (#2629's mkdtempForTest routing), and this test's own scratch dir predates that change. Route it through the same helper so the branch's own new test stays lint-clean; behavior is unchanged, only where the temp directory comes from.
1 participant
Summary
Route every product-test scratch directory through the per-package
mkdtempForTest/mkdtempForTestSynchelpers so it lands under the run's redirected TMPDIR and is removed once per run, instead ofpath.join(os.tmpdir(), name), which reuses a fixed path across the whole suite.tmp-dirhelpers toplatform-harmonyos,provider-webdriver,replay-testand trimsplatform-apple's to the variant each tree uses. Also migrates a rawos.tmpdir()scratch dir that arrived mid-review (session-test-attempt.test.ts) and drops a deadnode:osre-export (session-close-shutdown.fixtures.ts).daemon.logidiom collapses to one scratch dir per site; redundantrandomUUID/Date.now()/pid suffixes inside already-unique dirs dropped (fixedtunnel.json/screenshot.png); two ad-hoc Android screenshot paths fold into the existingwithTempScreenshothelper.no-restricted-importsoverride inoxlint.config.ts: a product test file that importsnode:osis a lint error pointing atmkdtempForTest, mirroring the existingnode:child_processban. Scope is the test topology (*.test.ts,*.fixtures.ts, and.tsunder__tests__/andtest-utils/insrcandpackages/*/src); only thetmp-dirhelper modules are exempt by path. Any other justified reader (mocking productionos.tmpdir/os.homedir, a length-limited Unix socket path, or the TMPDIR mechanism itself) carries a co-located// oxlint-disable-next-line no-restricted-imports -- <reason>on its single import — there is no per-file allowlist in the config to drift.packages/maestro/test/**is out of scope.Scope: 95 files, ~977 gross lines, inherently all-or-nothing — the rule cannot land before its migration.
Validation
Tested at
23c99ebb79.pnpm check:affected --run→ all runnable checks passed (exit 0).pnpm lint,format:check, fulltypecheck,fallow --base origin/main,gate-manifest(+test),affected:test, and the unit run for every changed file are green. The rule is proven reachable: it errors on a plantednode:osimport in a*.fixtures.ts/helper file and stays silent on thetmp-dirhelpers and the 12 disabled readers.