ci: speed up test suite, especially SEA tests#244
Open
robertsLando wants to merge 4 commits intomainfrom
Open
Conversation
…elism - Move SEA tests 85-92 into npmTests (only-npm) — they ignore the target argument and always build for the host Node version, so running them in both test:22 and test:24 was redundant (42 heavy invocations removed) - Share build artifacts: build job uploads lib-es5/ once, test jobs download it instead of each running yarn build (~30s saved × 18 jobs) - Add restore-keys fallback to pkg-cache for better cache hit rates - Add bounded-concurrency test runner (TEST_CONCURRENCY env var): test:22/test:24 run 4 tests in parallel, test:host stays sequential (pnpm/SEA tests have global state that races under concurrency) Closes #241 Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
The first commit introduced parallel test execution but pkg-fetch uses a deterministic temp filename (*.downloading) that collides when multiple processes download the same binary concurrently. This caused ENOENT errors on CI where the cache was cold (cache key was renamed). Fix by pre-downloading binaries for all platforms (linux, macos, win) with a single sequential pkg invocation before the parallel test loop. Also add the old cache key format as a restore-key fallback so existing caches from previous runs are not wasted. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Two independent fixes for the CI parallelism issues:
1. lib/index.ts: macOS bytecode signing no longer races across concurrent
pkg processes. The old rm + copy + sign sequence wrote to the shared
'{binary}-signed' path on every invocation, so parallel processes
truncated each other's writes. Now: reuse the signed binary if it
exists; otherwise write to a unique temp path and atomically rename
(rename replaces the target on POSIX, so late writers don't corrupt
earlier readers whose file handles remain valid).
2. test/test.js: use pkg-fetch's need() API directly to warm the binary
cache before parallel execution, instead of spawning pkg. Cleaner,
skips pkg's codesign step, and covers all platform/arch combinations
the tests could target (linux/macos/win × x64/arm64).
Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
getNodejsExecutable ran checksum verification and archive extraction on every invocation, even when the cached archive and extracted binary were already on disk. nodejs.org archives are immutable, so re-hashing 100 MB and re-extracting buys nothing but does cost ~5-7 s per call. - Checksum verification now runs only when we actually download. - extract() returns early if the target binary already exists. This saves ~7 s per SEA test invocation after the first in each CI job, ~50 s off test_host on CI (7 SEA tests run in that suite). Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
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
npmTestsso they only run intest:host— they ignore the target argument (always build for the host Node version), so running them in bothtest:22andtest:24was redundant (42 heavy invocations removed)lib-es5/+prelude/sea-bootstrap.bundle.jsonce, test jobs download it instead of each runningyarn build(~30s saved × 18 jobs)restore-keysfallback topkg-cachefor better cache hit rates on first runsTEST_CONCURRENCYenv var —test:22/test:24run 4 tests in parallel (~2.5x speedup),test:hoststays sequential (pnpm/SEA tests have global state that races under concurrency)Expected impact
yarn buildin test jobsTest plan
test:22 no-npmwithTEST_CONCURRENCY=4: 140/141 pass (1 pre-existing failure in test-77), deterministic across 3 runstest:host only-npmwithTEST_CONCURRENCY=1: 20/20 passCloses #241
🤖 Generated with Claude Code