test: serialize Steady-backed Jest runs#1976
Conversation
| function has_jest_worker_flag() { | ||
| for arg in "$@"; do | ||
| case "$arg" in | ||
| --runInBand|--runInBand=*|--maxWorkers|--maxWorkers=*|-i|-w) |
There was a problem hiding this comment.
This misses other Jest-supported spellings of the same worker option. Jest 29.7 accepts --max-workers=2, -w=2, and -w2, but each falls through this matcher, so the wrapper prepends --runInBand; Jest then exits 1 with Both --runInBand and --maxWorkers were specified, only one is allowed. I reproduced that directly with ./scripts/test --showConfig --max-workers=2, while invoking Jest directly with each spelling reports maxWorkers: 2. Please recognize the dashed long form and attached short forms (and add wrapper-level regression coverage) so valid existing pnpm test -- ... invocations do not break.
jbeckwith-oai
left a comment
jbeckwith-oai
left a comment
There was a problem hiding this comment.
I validated the intended serialization behavior and the motivating failure, but found one backward-compatibility defect in the worker-flag detection (inline).
Evidence reviewed:
- The PR has no linked GitHub issue. I traced the motivating failure to CI run 28808742182 (Node 20): the full parallel suite failed only
tests/api-resources/skills/skills.test.ts, receivingAPIConnectionError: Connection error.instead ofNotFoundError; 109 other suites / 1,141 tests passed, including the Skills versions suite. This matches an intermittent shared-Steady failure rather than an SDK behavior regression. - On the base commit, repeated local parallel stress runs of the non-Bedrock suite passed (the flake did not reproduce locally, consistent with its intermittent nature).
- On this head,
bash -n scripts/testpassed. The targeted Skills and Skills Versions run passed (2 suites, 16 tests)../scripts/test --showConfigreportsmaxWorkers: 1; explicit--maxWorkers=2and-w 2both remain atmaxWorkers: 2. Current CI is green across Node 20/22/24/26.2, build, lint, breaking-change detection, ecosystem, CodeQL, and OkTest 237/237. - A complete local run reached all suites but was not a clean signal because the shared checkout lacked the optional AWS dependency used by two Bedrock test files; all 108 other suites passed, including both Skills suites. This is environmental and unrelated to the patch.
Compatibility assessment: this changes only the repository test harness, not published runtime/types/API behavior. Default local/CI tests become serial (slower but safer), while TEST_API_BASE_URL continues to preserve Jest defaults. The blocking issue is that valid Jest aliases --max-workers=2, -w=2, and -w2 are not detected. The wrapper injects --runInBand, and Jest rejects the invocation because both worker modes are present. Since preserving explicit worker settings is an explicit contract of this PR and these invocations worked before, please cover all accepted worker-option spellings before merge.
2 participants
Summary
--runInBand.--maxWorkersor--runInBandwhen callers provide them.Root Cause
Two unrelated PRs saw a single multipart skills request fail with
APIConnectionError: Connection error.while expecting Steady to return a 404 for/_stainless_unknown_path. The failures happened under Jest's parallel worker mode while many tests shared one local Steady process on port 4010, and reruns passed. The rest of each job continued to communicate with Steady, which points to intermittent mock-server request handling under concurrency rather than product code.Impact
The default
./scripts/testpath is now more stable in CI and local runs that use the built-in Steady server. Developers can still opt into parallel workers explicitly when they are testing against another server or want to stress the harness.Validation
bash -n scripts/test./scripts/test tests/api-resources/skills/skills.test.ts tests/api-resources/skills/versions/versions.test.ts./scripts/test./scripts/test tests/api-resources/skills/skills.test.ts --maxWorkers=2npx pnpm@11.5.1 run lint