Skip to content

test: narrow subst pattern in non-existent-branch.t to avoid matching paths#14314

Merged
ElectreAAS merged 2 commits into
ocaml:mainfrom
robinbb:robinbb-fix-pin-stanza-path-sensitive
Apr 30, 2026
Merged

test: narrow subst pattern in non-existent-branch.t to avoid matching paths#14314
ElectreAAS merged 2 commits into
ocaml:mainfrom
robinbb:robinbb-fix-pin-stanza-path-sensitive

Conversation

@robinbb
Copy link
Copy Markdown
Collaborator

@robinbb robinbb commented Apr 23, 2026

Summary

The output post-processing in
test/blackbox-tests/test-cases/pkg/pin-stanza/non-existent-branch.t
uses dune_cmd subst '-\d+' '-eol' to normalise the variable
end-column in the characters 6-<N>: part of dune's error
location. The pattern is too broad: any -<digits> substring in
the output matches, including path components in the sandbox root.

When the repo is checked out at a worktree whose directory contains
-<digits> (e.g. dune-pr-14116/), the substitution rewrites
that path component and BUILD_PATH_PREFIX_MAP stripping no
longer matches, so the test output contains a raw sandbox path and
the assertion fails.

Anchor the pattern to characters 6-\d+ so it only matches the
column range it was meant to normalise.

Test plan

  • Reproduced the failure in a worktree checked out at a path
    containing -<digits> (e.g. dune-pr-14116/) on upstream/main.
  • Applied the fix and verified the test passes in the same
    worktree path.
  • Verified the test continues to pass in a worktree whose path
    does not match -<digits>.

The test's output post-processing used `dune_cmd subst '-\d+' '-eol'`
to normalise the variable end-column in the "characters 6-N:" part
of dune's error location. The pattern was too broad: any `-<digits>`
substring in the output would match, including path components in
the sandbox root (e.g. when the repo is checked out at a worktree
whose directory contains `-<digits>`, such as `dune-pr-14116/`, the
substitution rewrites the path and BUILD_PATH_PREFIX_MAP stripping
no longer matches, producing a raw-path diff in the test output).

Anchor the pattern to `characters 6-\d+` so it only matches the
column range it was meant to normalise.

Signed-off-by: Robin Bate Boerop <me@robinbb.com>
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes a flaky blackbox test by narrowing an overly broad output normalization substitution so it no longer rewrites unrelated -<digits> substrings (notably in sandbox/worktree paths), which could prevent BUILD_PATH_PREFIX_MAP path stripping and cause assertion failures.

Changes:

  • Tighten the dune_cmd subst regex from -\d+ to characters 6-\d+ in the test output post-processing.
  • Update the replacement string accordingly to normalize the end-column range without affecting paths.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@robinbb robinbb requested a review from ElectreAAS April 23, 2026 23:54
@robinbb robinbb marked this pull request as ready for review April 24, 2026 01:40
@robinbb robinbb assigned ElectreAAS and unassigned ElectreAAS Apr 24, 2026
@Alizter Alizter self-assigned this Apr 29, 2026
Comment thread test/blackbox-tests/test-cases/pkg/pin-stanza/non-existent-branch.t Outdated
Replaces [subst 'characters 6-\d+' 'characters 6-eol'] with
[subst 'characters \d+-\d+:' 'characters start-eol:'], so a future
shift in either column doesn't break the assertion. Mirrors the
substitution shape used in ocaml#13568 for an analogously-named test.

Co-authored-by: Ambre Austen Suhamy <ambre.suhamy@pm.me>
Signed-off-by: Robin Bate Boerop <me@robinbb.com>
@robinbb robinbb force-pushed the robinbb-fix-pin-stanza-path-sensitive branch from e3bdf64 to 50a924a Compare April 29, 2026 19:48
@ElectreAAS ElectreAAS merged commit 153a459 into ocaml:main Apr 30, 2026
31 checks passed
@robinbb robinbb deleted the robinbb-fix-pin-stanza-path-sensitive branch April 30, 2026 13:49
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.

4 participants