Conversation
…test subprocess The file spawned 28 bun subprocesses (one per TZ assertion) just to pin the process time zone. On ASAN debug each spawn is ~1s of startup, so the file took ~25s in CI and tests with two sequential spawns regularly hit the 5s default timeout. Assigning process.env.TZ at runtime updates WTF's time-zone override immediately (cron.test.ts already relies on this), so the assertions can run in-process behind a withTZ() helper. One spawn is kept to cover the startup-env path; everything else is pure computation. Also: the opt-vs-env equivalence test now asserts concrete expected ISO strings per zone instead of only 'viaOpt === viaEnv', and the fake-timers case uses in-process jest.useFakeTimers() like in-process-cron.test.ts does.
WalkthroughChangesCron time-zone tests now run mostly in-process by controlling Cron local-time behavior
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
There was a problem hiding this comment.
LGTM — test-only refactor that swaps 28 subprocess spawns for in-process process.env.TZ assignment, matching the pattern already used in cron.test.ts.
What was reviewed:
- All 30 original assertions preserved with identical expected values; the one strengthened test's new hardcoded ISO strings (Auckland 21:00Z, Lord Howe 22:30Z) check out against the zones' June offsets.
.concurrentcorrectly dropped sincewithTZ()mutates a process global; the?? ""restore matchescron.test.ts:1397's documented convention (empty string clears the WTF override,deletewould not).- The remaining spawn drains pipes concurrently and asserts a combined object; the fake-timers case mirrors
in-process-cron.test.ts's try/finally shape.
Extended reasoning...
Overview
This PR touches a single test file, test/js/bun/cron/cron-local-time.test.ts, converting it from spawning 28 bun -e subprocesses (one per assertion, each with a fixed TZ env var) to setting process.env.TZ in-process via a withTZ() helper and calling Bun.cron.parse directly. One subprocess is retained to keep coverage of the startup-env path. No production code is modified.
Security risks
None. Test-only change; no new inputs, no network, no filesystem writes beyond what the harness already does.
Level of scrutiny
Low-to-moderate. The concern for a test refactor like this is whether coverage was silently weakened. I traced each converted case:
- Every original
(tz, expr, from, expected)tuple is present with the same expected ISO string. - The
spring-forward: only the first match in the gapcase switched from two sequentialparseInTZcalls to onechainInTZ(..., 2); the chain feeds the first result (07:00Z) asfromto the second call, which is exactly what the old code did explicitly. - The
tz option matches the same zone set as process TZcase was strengthened: it now asserts concrete expected ISO strings per zone rather than onlyviaOpt === viaEnv. I verified the four new constants against each zone's June UTC offset (PDT −7, JST +9, NZST +12, LHST +10:30) and they are correct. describe.concurrent→describeis required now that tests mutate the process-globalTZ; a race here would have been a real flake source.
Other factors
- The
process.env.TZ = old ?? ""restore idiom exactly matchescron.test.ts:1395-1397, where a comment explains thatdelete process.env.TZwould not clearWTF::setTimeZoneOverrideand that empty-string assignment reverts to the system zone. The finder-raised concern about restoring to""when originally unset was correctly ruled out — this is the documented, intentional convention in the sibling file. - The fake-timers test uses the same
try { ... } finally { jest.useRealTimers() }shape asin-process-cron.test.ts:124-145, andusing _jobdisposes the cron before real timers are restored. - The one remaining subprocess test follows REVIEW.md's guidance:
Promise.allover stdout/stderr/exited and a single combined-objecttoEqual. - PR description reports 31 pass / 0 fail on debug+ASAN locally, down from ~35s wall to ~4-8s. No prior human or bot reviews to address.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/js/bun/cron/cron-local-time.test.ts`:
- Line 215: Remove the duplicate const actual declaration in the test scope,
keeping a single Record<string, { viaOpt: string; viaEnv: string }> binding for
the existing test logic.
- Around line 14-26: Update the TZ restoration logic in withTZ and the
beforeAll/afterAll hooks to preserve an unset process.env.TZ by deleting the
variable when its saved value is undefined, while assigning the saved string
value when defined. Ensure both helper calls and the test suite leave the
original environment state unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2ee9d55f-c7ae-46c4-a4a9-aafa2d21cb21
📒 Files selected for processing (1)
test/js/bun/cron/cron-local-time.test.ts
There was a problem hiding this comment.
LGTM — test-only refactor that swaps per-assertion subprocess spawns for in-process process.env.TZ assignment, matching the established pattern in cron.test.ts.
What was reviewed
- All 30 original assertions are preserved with identical expected values; one new startup-env spawn case added, no tests deleted or weakened.
describe.concurrentcorrectly dropped now that tests mutate process-globalTZ;withTZand the fake-timers case both restore infinally.- The
chainInTZrewrite of the spring-forward gap test is semantically equivalent (step 2's input equals the original second call's explicitfrom); the new Auckland/Lord Howe expected ISO strings check out against their June UTC offsets.
Extended reasoning...
Overview
This PR touches a single test file, test/js/bun/cron/cron-local-time.test.ts, refactoring it from spawning 28 bun -e subprocesses (one per assertion, solely to pin TZ) to setting process.env.TZ in-process via a withTZ() wrapper and calling Bun.cron.parse directly. One subprocess is retained to cover the startup-env path. No production code is touched.
Security risks
None. Test-only change; no new inputs, no network, no filesystem writes beyond what the harness already does.
Level of scrutiny
Low-medium. The main risk with a test refactor like this is silently weakening or losing coverage. I compared each rewritten test against the original: every expression/from/expected triple is preserved verbatim. The one substantive assertion change — tz option matches the same zone set as process TZ — is a strict strengthening (it now asserts concrete ISO strings per zone rather than only viaOpt === viaEnv, so a shared drift would now be caught). The spring-forward: only the first match in the gap test moved from two parseInTZ calls to one chainInTZ(…, 2); since chainInTZ feeds step 1's output (2025-03-09T07:00:00.000Z) into step 2, and the original second call used exactly that instant as from, the semantics are identical.
Other factors
- The
process.env.TZ = old ?? ""restore pattern (empty string reverts to system zone;deletewould strip the accessor without clearing WTF's override) is copied exactly fromcron.test.ts:1392-1397, so this is the established convention, not a new invention. .concurrentwas correctly removed since the suites now mutate process-global state; the PR comments this explicitly.jest.useFakeTimers()/useRealTimers()are wrapped in try/finally so a failure won't leak fake timers into later tests in the file.- I sanity-checked the two newly-hardcoded expected values (Pacific/Auckland at UTC+12 →
2026-06-15T21:00:00.000Z; Australia/Lord_Howe at UTC+10:30 →2026-06-15T22:30:00.000Z) and both are correct for June (no DST). - The PR description shows a 31/31 pass on debug+ASAN. This is a straightforward, well-motivated test-speedup that follows repo conventions.
bc4cb41 to
3beb389
Compare
There was a problem hiding this comment.
LGTM — test-only refactor that swaps per-assertion subprocess spawns for in-process process.env.TZ mutation, following the same pattern already in cron.test.ts.
What was reviewed
- All 30 original assertions are preserved with identical expected ISO strings;
.concurrentcorrectly dropped since tests now mutate globalprocess.env.TZ. withTZrestores via?? ""(notdelete) — matches the documented WTF override-clear idiom atcron.test.ts:1392-1397.- One subprocess kept for startup-env coverage; drains stdout/stderr/exited concurrently and asserts the combined object.
- Fake-timers case wraps
useRealTimers()infinally;using _jobdisposes the cron handle.
Extended reasoning...
Overview
This PR touches exactly one file, test/js/bun/cron/cron-local-time.test.ts, and no production code. It replaces 28 bun -e subprocess spawns (each ~1s under debug+ASAN, causing 5s-timeout failures in CI) with direct in-process calls to Bun.cron.parse, pinning the time zone per assertion via a small withTZ() helper that assigns process.env.TZ and restores it in finally. One subprocess is retained to cover the startup-env path that in-process assignment does not exercise. The fake-timers test moves from a spawned bun -e script to in-process jest.useFakeTimers().
Security risks
None. This is a test-only change with no production code, no new dependencies, and no network/file-system side effects beyond the single retained Bun.spawn of the debug binary itself.
Level of scrutiny
Low-to-moderate. The concern with a subprocess→in-process test refactor is silently weakening coverage. I checked each converted case against the diff: every (tz, expr, from, expected) tuple is preserved verbatim; two cases (spring-forward */15 and Lord Howe fall-back) are regrouped into single toEqual calls without changing inputs or expected outputs; tz option matches the same zone set as process TZ is strengthened to assert concrete ISO strings per zone rather than only viaOpt === viaEnv. No test is deleted, skipped, or has its expected value changed.
The process.env.TZ mutation approach is the established pattern in this directory — cron.test.ts:1390-1397 uses the identical beforeAll/afterAll + ?? "" restore, with a comment explaining why delete would not clear WTF::setTimeZoneOverride. The describe.concurrent → describe change is required because the tests now share mutable global state; the PR comments this explicitly and each in-process test runs in a few ms so serialization costs nothing.
Other factors
Both CodeRabbit findings on this PR were false positives and were withdrawn (the ?? "" restore is intentional per the cron.test.ts precedent; there is only one const actual). The bug-hunting system found nothing. The PR description shows 31/31 passing on debug+ASAN. The retained subprocess test follows harness conventions: {...bunEnv, TZ: ...}, concurrent pipe drain via Promise.all, and a single toEqual({stdout, stderr, exitCode}) assertion. The fake-timers test correctly restores real timers in finally and disposes the cron job via using.
|
Diff is green on every lane that ran tests: CI red is unrelated build infrastructure that also fails on main (#80215 has the same pattern):
This PR is a single-file |
What
test/js/bun/cron/cron-local-time.test.tsspawned 28bun -esubprocesses, one per(tz, expr, from)assertion, solely to pin the process time zone. On debug+ASAN each spawn is roughly a second of startup, so the file took ~25s wall in CI (build #80083, debian-13 x64-asan) and tests that did two sequential spawns (e.g. the Lord Howe fall-back case) regularly hit the 5s default timeout.Change
Assigning
process.env.TZat runtime updates WTF's time-zone override immediately;cron.test.ts'sBun.cron.parsesuite already relies on this. So the helpers now setprocess.env.TZin-process behind a smallwithTZ()wrapper and callBun.cron.parsedirectly:One subprocess is kept (
TZ set in the spawned process's environment is honored at startup) to keep explicit coverage of the startup-env path thatwithTZ()does not exercise. The fake-timers case now uses in-processjest.useFakeTimers()the same wayin-process-cron.test.tsdoes.The
describeblocks drop.concurrentsince they now mutate globalprocess.env.TZ; each in-process test is a few ms so sequential is still fast.Assertion improvements
tz option matches the same zone set as process TZnow asserts the concrete expected ISO string per zone rather than onlyviaOpt === viaEnv, so a drift in both paths would be caught.spring-forward: only the first match in the gap fires shifteduses a singlechainInTZ+toEqualinstead of two separate awaited spawns.Lord Howe: 30-minute fall-backgroups both sub-cases into onetoEqualfor a clearer diff on failure.{ stdout, stderr, exitCode }withtoEqualrather than separatetoBecalls.No tests were deleted or skipped; the 30 original cases are all present plus the one new startup-env case (31 total).
Timing (local, debug+ASAN,
./build/debug/bun-debug test)The before/after variance is FS-dependent; CI's 25s maps to the same 28-spawn cost.
[stamp-90s] gate passed · iteration 2 · 1 files touched
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 1 rejected · iteration 2
evidence per changed file