Repository navigation
test: remove spec directories with the retrying async helper, since rmSync never retries on Windows - #622
Merged
Conversation
…mSync never retries on Windows Under Node 24 on Windows, rmSync with maxRetries reports a held directory as EPERM at once, so the retries in about sixty spec cleanups never ran. Each now awaits removeTestDirectory() from tools/test-cleanup.ts, whose promise form does retry. The two widget dev tests that delete and remake a folder within one turn of the event loop keep a synchronous rmSync without retries. A new invariant, test-cleanup-retries-asynchronously, fails when test code calls rmSync with maxRetries, and a Windows-only spec shows a directory held as a child process's working directory: the synchronous form fails at once, and the helper removes it once the holder lets go. Refs #535, #543
…, and read comments and strings out of the retry check The check now blanks comments and string text before it reads a file, so a call named in a comment is not a call and a parenthesis in a string no longer unbalances the call around it. It follows rmSync under a name the file gives it, honours an invariant-allow marker for the failing form shown on purpose, and covers the widget tooling smoke CI runs on Windows.
…leanup # Conflicts: # apps/runtime/test/widget-perform.spec.ts
Contributor
Author
|
Review attestation: ready to merge at A push to this PR makes this attestation stale; the new head needs its own review. |
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.
Fixes #563. Refs #535, #543, #561.
Why
On Windows,
rmSync(path, { recursive: true, force: true, maxRetries })reports a held directory asEPERM(Node 24) or
EBUSY(Node 22) at once and never runs the retries.fs.promises.rmwith the same options does retry. Measured on thismachine (Windows 11, Node v24.11.0). The directory was held as the working directory of a child process for 1.5 s:
rmSync(..., { maxRetries: 10, retryDelay: 100 })EPERMafter 0 msawait rm(..., { maxRetries: 10, retryDelay: 100 })A held directory is also why many spec cleanups asked for retries in the first place.
What changes
rmSyncin test code now awaitsremoveTestDirectory()fromtools/test-cleanup.ts(added bytest: wait for a timed-out body before cleanup, and retry removal where Windows retries #561). This covers 58 spec files and the
apps/runtime/test/live-nodes.tshelper: 64 call sites in total. EachafterEach/afterAllhook that cleans up becameasync, andrmSyncwas dropped from imports that no longer use it.Calls that were already in an async context were changed in place:
finallyblocks incoding-journeyandservice-container-engine;task-dispatch-scoped, whose steps are already awaited;liveNodes().stopAll();rmSync, now withoutmaxRetries. They delete a watched folder and makeit again within one turn of the event loop, and that synchronous step is what they test. A comment says why. The
other widget-dev deletions use the helper.
test-cleanup-retries-asynchronously(tools/invariants/, wired intopnpm invariants). It failswhen test code calls
rmSync(withmaxRetries. Test code means specs, plus the files undertest/ande2e/folders in
apps,packages,packs,toolsandexamples. The check finds each call's closing parenthesis bycounting brackets, so an options object that spans several lines is still read whole. The failure message names the
file and line and points to the helper. On
main, it reports all 64 call sites. On this branch it passes.tools/test/test-cleanup-retries.spec.ts:and which paths count as test code;
until it is released. In that test:
maxRetries: 10throwsEPERM/EBUSYin under 500 ms, and the directory remains;removeTestDirectory()succeeds only after the holder is released 700 ms later, which shows that the retries ran.Changes after review (round 1)
main. fix(browser): close the browser when its page cannot be opened #587'spacks/browser-playwright/test/new-page-failure.spec.tsis converted: itsafterAllis async and awaitsremoveTestDirectory(dir). Without this change, the merged tree failed the new invariant.tools/smoke-widget-tooling.mjsis converted, and the invariant now covers it, because CI runs it onwindows-latest. ItscleanUp()was already async, and it now awaitsrmfromnode:fs/promiseswith the same retries.(inside a string no longer makes it read to the end of the file. Code inside a template literal's${...}is still read.rmSyncunder a name the file gives it:rmSync as x,{ rmSync: x }, orconst x = rmSync.// invariant-allow: sync-rm-retries. Only the Windows test in the new spec does.Production code is out of scope and tracked in #626:
apps/runtime/src/worker-process.ts:344andapps/runtime/src/application/widget-dev-sessions.ts:645still callrmSyncwithmaxRetries.Verification (Windows 11, Node v24.11.0, before review)
pnpm invariantsmain, the new check fails with 64 call sites.pnpm typecheckpnpm lintlive-nodes.tsusers, 3 runs in a row on the final headtools/test/test-cleanup-retries.spec.ts, 5 runs in a rowpnpm test(full suite)An earlier set of three batch runs had failures, and none came from the converted cleanup:
and under load the synchronous removal succeeded, so the child was not holding the directory at that moment. The
child now reports that it is running and holds the directory until it is released. The test then passed 5 runs in a
row on its own and in all 3 later batch runs.
peer-delegation.spec.tsfailed. It then passed 5 runs in a row on its own, andin all 3 later batch runs.
After review, at the new head (Windows 11, Node v24.11.0):
tools/test/test-cleanup-retries.spec.ts, 3 runs in a rowpacks/browser-playwright/test/new-page-failure.spec.tspnpm invariantspnpm typecheckeslinton the changed filesnode --check tools/smoke-widget-tooling.mjsOverlap with open PRs
packages/core/test/widget-dev-engine.spec.tsandapps/runtime/test/widget-dev-sessions.spec.ts. fix(widgets): re-arm a dev session on a changed folder id and bound persistent stat errors #614 keepsrmSync(root, { recursive: true, force: true, maxRetries: 5 })there, so whichever PR lands second must dropmaxRetrieswhile it keeps fix(widgets): re-arm a dev session on a changed folder id and bound persistent stat errors #614's test bodies. Otherwise the new invariant fails.apps/runtime/test/widget-perform.spec.tsis resolved here by keeping both imports. That spec and the cleanup spec pass (90/90), along with invariants, typecheck and eslint, on the merged head.git merge-tree).Follow-ups (not in this PR)
worker-process.ts:344andwidget-dev-sessions.ts:645.