Skip to content

build: order smoke_test and dsymutil after strip - #36139

Merged
Jarred-Sumner merged 2 commits into
mainfrom
claude/farm/3c644de7/smoke-test-strip-race
Jul 28, 2026
Merged

build: order smoke_test and dsymutil after strip#36139
Jarred-Sumner merged 2 commits into
mainfrom
claude/farm/3c644de7/smoke-test-strip-race

build: order smoke_test and dsymutil after strip

02a9ccc
Select commit
Loading
Failed to load commit list.
Claude / Claude Code Review completed Jul 27, 2026 in 14m 48s

Code review found 2 potential issues

Found 3 candidates, confirmed 2. See review comments for details.

Details

Severity Count
🔴 Important 0
🟡 Nit 2
🟣 Pre-existing 0
Severity File:Line Issue
🟡 Nit test/internal/build-post-link-ordering.test.ts:72-77 buildEdge() unwrap regex doesn't match Ninja's actual continuation format
🟡 Nit test/internal/build-post-link-ordering.test.ts:22-23 mockToolchain() is the fourth copy of this helper in test/internal/

Annotations

Check warning on line 77 in test/internal/build-post-link-ordering.test.ts

See this annotation in the file changed.

@claude claude / Claude Code Review

buildEdge() unwrap regex doesn't match Ninja's actual continuation format

The unwrap regex `/\$\n {2}/g` doesn't match what `wrapLongLine()` in `scripts/build/ninja.ts` actually emits — continuations are ` $` + newline + **4** spaces, so a wrapped `foo $\n    bar` would unwrap to `foo   bar` (3 stray spaces) and the exact `.toBe()` assertions below would fail with a confusing whitespace diff. Currently latent (these edges are ~51-73 chars, under the 120-char wrap threshold), but `/ \$\n {4}/g` → `" "` would make the helper match its own docstring.

Check warning on line 23 in test/internal/build-post-link-ordering.test.ts

See this annotation in the file changed.

@claude claude / Claude Code Review

mockToolchain() is the fourth copy of this helper in test/internal/

This is the fourth copy of `mockToolchain()` in `test/internal/` — near-identical helpers already exist in `macos-cross-config.test.ts`, `source-lints/webkit-prebuilt-url.test.ts`, and `source-lints/windows-cross-config.test.ts`. The copies are already drifting (this one had to add `hostCc`/`hostCxx` that the others lack), so it may be worth extracting a shared `mockToolchain()` into something like `test/internal/build-helpers.ts` and importing it in all four places. Non-blocking — test-only, an