fix(tests): serialize package builds across parallel vitest workers#3480
Open
arham766 wants to merge 1 commit into
Open
fix(tests): serialize package builds across parallel vitest workers#3480arham766 wants to merge 1 commit into
arham766 wants to merge 1 commit into
Conversation
Three test files build into packages/core/dist from beforeAll (build-css.test.mjs runs pnpm build unconditionally; the two build-theme files build core when dist/theme is missing). Vitest runs test files in parallel worker processes, so two builds could run concurrently and race each other's rimraf/output, failing CI with ENOTEMPTY on dist/utils or an unresolvable dist/index.js. Adds a repo-wide lock (atomic mkdir with a bounded wait) shared by all three, with the exists-check re-run inside the lock so waiters skip a rebuild the holder already did. Fixes facebook#3479
|
@arham766 is attempting to deploy a commit to the Meta Open Source Team on Vercel. A member of the Team first needs to authorize it. |
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.
Summary
Fixes #3479. Three test files build into
packages/core/distfrombeforeAll:scripts/build-css.test.mjsruns a fullpnpm buildunconditionally, and the twobuild-themetest files build core whendist/theme/index.jsis missing. Vitest runs test files in parallel worker processes, so two of those builds can run concurrently over the same dist directory: one build'srimraf distraces the other's output, failing withENOTEMPTY: rmdir dist/utilsoresbuild: Could not resolve dist/index.js. That is exactly the failure on the CI run for #3466 (a PR whose diff touches none of these files; adding a test file reshuffled vitest's worker distribution enough to expose the race).The fix adds
scripts/repo-build-lock.mjs: awithRepoBuildLock(fn)helper that serializes the builds with an atomic lock directory (fs.mkdirSynceither creates it or throws EEXIST) and a bounded wait with a stale-lock hint. All three files run their build under the lock, and the build-theme files re-checkexistsSyncinside it, so a waiter skips the rebuild the holder already completed. The lock directory is gitignored.Test plan
build-theme.prose.test.mjspasses 2/2 locally;build-theme.import-path.test.mjsis 4/5 with the one failure being the pre-existing Windows path-separator issue tracked in [Bug] Test suite fails on Windows due to path separator and URL.pathname issues #3233 (present on main before this change).execFileSync('pnpm', ...)cannot spawnpnpm.cmdthere (also pre-existing, part of [Bug] Test suite fails on Windows due to path separator and URL.pathname issues #3233), so the concurrency correctness is covered by the stress test above plus Linux CI on this PR.node scripts/check-changesets.mjspasses.