Skip to content

test: build astro tsdk expectation with component joins (fixes deterministic Windows shard 1/3 failure) - #248

Closed
iceteaSA wants to merge 1 commit into
cortexkit:mainfrom
iceteaSA:windows-astro-tsdk-separators
Closed

test: build astro tsdk expectation with component joins (fixes deterministic Windows shard 1/3 failure)#248
iceteaSA wants to merge 1 commit into
cortexkit:mainfrom
iceteaSA:windows-astro-tsdk-separators

Conversation

@iceteaSA

@iceteaSA iceteaSA commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

Unit / Cargo integration shard 1/3 (Windows) has been red on main for a while. I called it a rotating flake on #246that was wrong, and the log on my rebased run shows why.

It's deterministic, not timing

thread 'lsp_manager_test::astro_uses_the_nearest_project_typescript_sdk' panicked at
crates\aft\tests\integration\lsp_manager_test.rs:471:5:
assertion `left == right` failed
  left: String("C:\\...\\workspace\\apps/site\\node_modules\\typescript\\lib")
 right:        "C:\\...\\workspace\\apps/site\\node_modules/typescript/lib"

node_modules\typescript\lib vs node_modules/typescript/lib. Not a timeout, not ordering — a path-separator mismatch that fails the same way every run.

Production walks component-wise (lsp/manager.rs:2636-2639):

let lib = directory.join("node_modules").join("typescript").join("lib");

The test built its expectation in one hop (lsp_manager_test.rs:453):

let member_tsdk = member.join("node_modules/typescript/lib");

Path::join doesn't normalize embedded separators — it appends the string as given. On Windows that leaves the expectation holding a mixed-separator path that the production walk cannot produce. On unix both spellings coincide, so it passes everywhere except the platform it breaks on.

This PR builds the expectation the way production builds the value. Same for the apps/site and src/page.astro joins in that test, so the whole test uses native separators consistently.

Why it looked like a flake

Two different failures live in that job and fail-fast surfaces whichever trips first:

main @ 1d2da675   lsp_manager_test::astro_uses_the_nearest_project_typescript_sdk   ← this bug
PR #246 @ bfaa0d93 configure_test::configure_does_not_warn_for_file_discovered_...   ← "timed out after 10s waiting for aft process exit"
PR #246 @ 967bedfa lsp_manager_test::astro_uses_the_nearest_project_typescript_sdk   ← this bug again

Rotating test names read as flake; the cancel point moving (317/493 vs 123/493) is timing, but one of the two underlying failures isn't. The process-exit timeout is still open and unrelated to this.

Verification

Linux, on this branch: 5 passed, 0 failed (astro_uses_the_nearest_project_typescript_sdk, astro_user_initialization_options_override_the_computed_tsdk, astro_without_project_typescript_is_unavailable_without_spawning). cargo fmt --check clean.

The red control is the CI log above, not a synthetic one — I have no Windows host, so I can't run the before/after locally. The failure reproduces on two different shas, and the fix makes both sides of the comparison use the same construction, so they're identical by construction on every platform. Your Windows runner is the actual green control.

Scope note

The same pattern — join("a/b/c") with embedded separators — appears 425 times across 49 files in crates/aft/tests/integration/. The vast majority are harmless: paths used for create_dir_all or fs::write, where Windows accepts forward slashes fine. Only ones whose value is compared against a production-built path can break, and fail-fast means any others are hidden behind the first failure.

I've deliberately fixed only the one that's demonstrably red rather than sweeping 425 call sites blind. Flagging the class in case you want a broader pass — a lint or a test helper that builds paths component-wise would prevent the next one.


View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.


Summary by cubic

Fixes a deterministic Windows failure in astro_uses_the_nearest_project_typescript_sdk by building the expected TypeScript SDK path with component-wise joins. The test previously used a single join with embedded slashes, producing mixed separators on Windows and a guaranteed mismatch with production.

  • Builds expected paths with join("node_modules").join("typescript").join("lib") to match production find_project_typescript_sdk; also updates apps/site and src/page.astro joins for consistency.
  • Test-only change; no runtime behavior change. Should stabilize Unit / Cargo integration shard 1/3 (Windows). The separate process-exit timeout remains unrelated.

Written for commit f6e2afc. Summary will update on new commits.

Review in cubic

Greptile Summary

This PR fixes a deterministic Windows-only failure in the Astro TypeScript SDK integration test.

  • Builds fixture and expected paths with component-wise joins so they use platform-native separators.
  • Aligns the expected SDK path construction with the production lookup implementation.
  • Leaves production behavior unchanged.

Confidence Score: 5/5

The PR appears safe to merge with no actionable issues identified.

The changed test constructs and serializes paths consistently with the production SDK lookup, fixing the Windows separator mismatch without changing fixture semantics on other platforms.

Important Files Changed

Filename Overview
crates/aft/tests/integration/lsp_manager_test.rs Replaces embedded slash-separated test paths with component-wise joins, correctly matching production path construction across platforms.

Reviews (1): Last reviewed commit: "test: build astro tsdk expectation with ..." | Re-trigger Greptile

astro_uses_the_nearest_project_typescript_sdk built its expected path as
member.join("node_modules/typescript/lib"). Path::join keeps embedded
forward slashes verbatim, so on Windows the expectation held
...\\apps/site\\node_modules/typescript/lib while find_project_typescript_sdk
walks with .join("node_modules").join("typescript").join("lib") and
produces ...\\node_modules\\typescript\\lib. Deterministic mismatch on
Windows, invisible on unix where both spellings coincide.

Build the expectation the way production builds the value.
@iceteaSA

Copy link
Copy Markdown
Contributor Author

Superseded by 9dae63eb on main — closing. We landed the same diagnosis independently about two minutes apart (yours 23:31:53 +0300, mine 23:33:51), so this was concurrent work rather than either of us duplicating the other.

Different fix shapes, worth a sentence in case it's useful rather than as an argument for reopening:

  • 9dae63eb normalizes both sides at the assertion (.replace('\\', "/")) — asserts the paths are equal modulo separator spelling, and is indifferent to how the fixture built its path.
  • This PR built the expectation the way find_project_typescript_sdk builds the value (component-wise joins) — asserts exact-byte equality.

The only case they diverge on: if the product ever emitted a genuinely mixed spelling (node_modules\typescript/lib), normalization passes and exact-byte fails. Whether that's worth catching depends on whether the exact string handed to the Astro server can matter to it — your call, and not worth a PR either way.

The 425-instance scope note from the description still stands independently of which fix landed: join("a/b/c") with embedded separators appears across 49 integration test files, harmless wherever the path is only fed to create_dir_all/fs::write, latent wherever it's compared against a production-built path — and fail-fast keeps any remaining ones hidden behind the first failure. A path-building test helper would close the class if it ever bites a third time.

#246 rebased onto 9dae63eb.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant