Skip to content

Reproducer test for #335 loose substring match in queryTargetsDependingOnModules#346

Closed
tinder-maxwellelliott wants to merge 4 commits into
masterfrom
claude/reproducer-issue-335-substring-match
Closed

Reproducer test for #335 loose substring match in queryTargetsDependingOnModules#346
tinder-maxwellelliott wants to merge 4 commits into
masterfrom
claude/reproducer-issue-335-substring-match

Conversation

@tinder-maxwellelliott
Copy link
Copy Markdown
Collaborator

Summary

  • Adds an @Ignored unit reproducer for #335 fix Update bazel-diff-example.sh #3 (loose substring match in CalculateImpactedTargetsInteractor.queryTargetsDependingOnModules). This is a follow-up to #340, which covered the parser-robustness piece (fix Typos #1) of the same issue.
  • The current code is:
val moduleRepos = allTargets.keys
    .filter { it.startsWith("@@") && it.contains(moduleName) }
    .map { it.substring(2).substringBefore("//") }
    .toSet()

it.contains(moduleName) is a very loose match. A single changed module like aspect_bazel_lib matches every canonical repo whose name starts with that string — @@aspect_bazel_lib+, @@aspect_bazel_lib++toolchains+bats_toolchains, etc. Each match becomes its own serial bazel query rdeps(//..., @@<repo>//...) subprocess. On the workspace in #335 that fan-out produced ~5,000 subprocesses and took multiple hours.

  • New test class CalculateImpactedTargetsInteractorIssue335Test:
    • Wires its own koin module with a mock BazelQueryService that captures every query expression.
    • Simulates a workspace with one module (aspect_bazel_lib) plus two module-extension repos whose canonical names begin with the same apparent name.
    • Asserts that resolving the changed module yields exactly one rdeps query, scoped to the actual changed repo.

The asked-for fix in #335 is either to match the canonical repo key exactly (e.g. aspect_bazel_lib+ — the part before any further +/~), or to look up the repo via bazel mod dump_repo_mapping instead of a substring scan. Drop @Ignore once that lands.

This is a test-only change. cli/BUILD adds the new kt_jvm_test target.

Test plan

  • bazel build //cli:CalculateImpactedTargetsInteractorIssue335Test succeeds.
  • bazel test //cli:CalculateImpactedTargetsInteractorIssue335TestPASSED in 0.5s (the new test is @Ignored and skipped).
  • After the match is tightened, remove @Ignore and re-run — exactly one rdeps query should be captured.

🤖 Generated with Claude Code

…endingOnModules

Issue #335 lists multiple potential fixes; my first PR (#340) covered
fix #1 (parser robustness). This PR adds a reproducer for fix #3: the
loose substring match in
CalculateImpactedTargetsInteractor.queryTargetsDependingOnModules.

The current code is:

  val moduleRepos = allTargets.keys
      .filter { it.startsWith("@@") && it.contains(moduleName) }
      .map { it.substring(2).substringBefore("//") }
      .toSet()

`it.contains(moduleName)` is a very loose match. A single changed
module like `aspect_bazel_lib` matches every canonical repo whose name
starts with that string -- e.g. `@@aspect_bazel_lib+`,
`@@aspect_bazel_lib++toolchains+bats_toolchains`, etc. Each match
becomes its own serial `bazel query rdeps(//..., @@<repo>//...)`
subprocess. On the workspace in #335 that fan-out produced ~5,000
subprocesses and took multiple hours.

New test class CalculateImpactedTargetsInteractorIssue335Test:
- Wires its own koin module with a mock BazelQueryService that captures
  every query expression.
- Simulates a workspace with one module (aspect_bazel_lib) plus two
  module-extension repos whose canonical names begin with the same
  apparent name.
- Asserts that resolving the changed module yields exactly ONE rdeps
  query, scoped to the actual changed repo.

@ignore'd until the match is tightened (match canonical repo key
exactly, or look up via `bazel mod dump_repo_mapping`). cli/BUILD wires
up the new kt_jvm_test target.

Refs #335

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@tinder-maxwellelliott tinder-maxwellelliott force-pushed the claude/reproducer-issue-335-substring-match branch from 647fa53 to 5fd35f4 Compare May 13, 2026 19:46
tinder-maxwellelliott added a commit that referenced this pull request May 18, 2026
…gOnModules (#354)

* Add reproducer test for #335 loose substring match in queryTargetsDependingOnModules

Issue #335 lists multiple potential fixes; my first PR (#340) covered
fix #1 (parser robustness). This PR adds a reproducer for fix #3: the
loose substring match in
CalculateImpactedTargetsInteractor.queryTargetsDependingOnModules.

The current code is:

  val moduleRepos = allTargets.keys
      .filter { it.startsWith("@@") && it.contains(moduleName) }
      .map { it.substring(2).substringBefore("//") }
      .toSet()

`it.contains(moduleName)` is a very loose match. A single changed
module like `aspect_bazel_lib` matches every canonical repo whose name
starts with that string -- e.g. `@@aspect_bazel_lib+`,
`@@aspect_bazel_lib++toolchains+bats_toolchains`, etc. Each match
becomes its own serial `bazel query rdeps(//..., @@<repo>//...)`
subprocess. On the workspace in #335 that fan-out produced ~5,000
subprocesses and took multiple hours.

New test class CalculateImpactedTargetsInteractorIssue335Test:
- Wires its own koin module with a mock BazelQueryService that captures
  every query expression.
- Simulates a workspace with one module (aspect_bazel_lib) plus two
  module-extension repos whose canonical names begin with the same
  apparent name.
- Asserts that resolving the changed module yields exactly ONE rdeps
  query, scoped to the actual changed repo.

@ignore'd until the match is tightened (match canonical repo key
exactly, or look up via `bazel mod dump_repo_mapping`). cli/BUILD wires
up the new kt_jvm_test target.

Refs #335

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* Fix #335 fix #3: tighten canonical-repo match in queryTargetsDependingOnModules

`queryTargetsDependingOnModules` resolved each changed module key to its
canonical bzlmod repos by scanning `allTargets.keys` with
`it.startsWith("@@") && it.contains(moduleName)`. The `contains` match
is far too loose: a single changed module like `aspect_bazel_lib` matched
every canonical repo whose name began with that string -- e.g.
`@@aspect_bazel_lib+`, `@@aspect_bazel_lib++toolchains+bats_toolchains`,
`@@aspect_bazel_lib++toolchains+yq_toolchains`, etc. Each match was then
unioned into the rdeps query expression (after the per-repo loop became
a single unioned query in an earlier change), but the same fan-out
behaviour on older revisions produced ~5,000 serial subprocesses on the
workspace in #335 and took multiple hours.

Tighten the match to the module's *base* canonical repo only:
`<moduleName><sep><versionSegment>` where `<sep>` is `+` or `~` and the
remainder contains no further separators. Module-extension repos always
layer additional `+`/`~` segments on top and are rejected here.

That narrowing is safe because extension-repo content changes already
propagate to main-repo consumers through the per-target hash dep chain:
`computeSimpleImpactedTargets` runs unconditionally for `@@*` labels and
the build-graph hasher folds dep hashes into each rule's hash, so a
consumer of a changed extension repo flips its own hash and is picked up
without needing an rdeps query targeting the extension repo. The rdeps
query exists to catch the harder case -- MODULE.bazel-only bumps that
don't flip any hash but do change which version of a base module repo is
selected -- and that is exactly the case the base-repo match handles.

The reproducer test class from PR #346 had a latent Mockito misuse in
its `@Before`: the second `whenever(queryService.query(any<String>()))`
stub omits the default-argument matcher and was throwing
`InvalidUseOfMatchersException` before any assertion ran. The test was
`@Ignore`d so the bug was never observed. Dropping the redundant stub
and dropping `@Ignore` -- the first stub already covers the production
caller, which always passes `useCquery = false` explicitly.

Verification:
- `bazel test //cli:CalculateImpactedTargetsInteractorTest` -- PASSED
  (21 pre-existing tests still pass, including
  `testUnionsRdepsAcrossChangedModules` which exercises the `~` form
  through the same code path).
- `bazel test //cli:CalculateImpactedTargetsInteractorIssue335Test` --
  PASSED (the un-`@Ignore`d reproducer now passes; reverting just the
  production change reproduces the bug with the loose match yielding
  `rdeps(//..., @@aspect_bazel_lib+//... + @@aspect_bazel_lib++toolchains+bats_toolchains//... + @@aspect_bazel_lib++toolchains+yq_toolchains//...)`).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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