fix(cli): make diff keys and a preproc test Windows path-safe#498
Merged
Conversation
Two Windows-only CI failures (run 26799627900), both `/` vs `\` separator issues in the #482 epic work, latent until the push hit Windows CI (local dev is Linux). - metric_diff `path_to_key`: `to_str()` kept the OS separator, so a directory-set's relpath fallback key was `nested\y.json` on Windows and could not pair with a `/`-keyed set. Normalize the separator to `/` so directory-set diff keys are platform-independent, matching the `name`-field branch's `/`-form. - cli_smoke `preproc_resolves_cross_file_include_across_directory`: the test built `helper_h` via `join("sub/helper.h")`, a mixed-separator path on Windows (`...\sub/helper.h`) that never matched the walk-emitted, backslash-separated `indirect_includes` path. Build the path component-by-component so it uses the OS separator. Test-only; the production preproc output was internally consistent. Both changes are no-ops on Linux (verified the two tests plus the metric_diff / cli_smoke / diff_since / check_exclude / check_baseline suites still pass). Baseline refreshed for the one-closure growth in metric_diff.rs.
PR #498's CI failed the `test` job on all three platforms — 10 integration tests asserting clean stderr got: bca: --since/auto-detect via GITHUB_BASE_REF failed (not inside a git checkout); proceeding without diff scope Root cause (pre-existing, unrelated to the Windows path fix): `bca check` auto-detects a diff scope from `GITHUB_BASE_REF` / `BCA_DIFF_BASE` / `GITHUB_EVENT_BEFORE` (diff.rs `auto_detect_base`), but the test harness's `scrub_ci_env` only stripped `GITHUB_STEP_SUMMARY` / `GITHUB_ACTIONS`. On a `pull_request` event GitHub sets `GITHUB_BASE_REF`, so the hermetic-tempdir `bca check` invocations auto-enabled `--since`, failed to resolve `origin/main` (the tempdir is not a git checkout), and warned to stderr. Push events leave `GITHUB_BASE_REF` unset, so this stayed latent until the first PR — the prior push-to-main runs never set it. Scrub the three auto-detect vars in `scrub_ci_env` (all integration tests route through it). Verified test-via-revert: with `GITHUB_BASE_REF=main` the suppression test fails without the scrub and passes with it; the full CLI suite and the whole workspace are green under the simulated PR env. A test that wants a diff scope still sets the var explicitly after construction.
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.
Two Windows-only CI failures (run 26799627900), both
/vs\separator issues in the #482 epic work, latent until the push hit Windows CI (local dev is Linux).metric_diff
path_to_key:to_str()kept the OS separator, so a directory-set's relpath fallback key wasnested\y.jsonon Windows and could not pair with a/-keyed set. Normalize the separator to/so directory-set diff keys are platform-independent, matching thename-field branch's/-form.cli_smoke
preproc_resolves_cross_file_include_across_directory: the test builthelper_hviajoin("sub/helper.h"), a mixed-separator path on Windows (...\sub/helper.h) that never matched the walk-emitted, backslash-separatedindirect_includespath. Build the path component-by-component so it uses the OS separator. Test-only; the production preproc output was internally consistent.Both changes are no-ops on Linux (verified the two tests plus the metric_diff / cli_smoke / diff_since / check_exclude / check_baseline suites still pass). Baseline refreshed for the one-closure growth in metric_diff.rs.