Add reproducer for #441: package_group change not detected as impacted (false negative)

cli/src/test/kotlin/com/bazel_diff/e2e/E2ETest.kt

@@ -1459,6 +1459,97 @@ class E2ETest {
     assertThat(impacted.contains("//:scanner")).isEqualTo(false)
   }
 
+  @Test
+  fun testPackageGroupChangeIsNotImpacted_reproducerForIssue441() {
+    // Reproducer for https://github.com/Tinder/bazel-diff/issues/441
+    //
+    // The under-invalidation (false-negative) half of the issue. bazel-diff only
+    // keeps targets whose query `Discriminator` is RULE, SOURCE_FILE, or
+    // GENERATED_FILE; every other type is dropped
+    // (BazelQueryService.toBazelTarget -> logs "Unsupported target type" and
+    // returns null). `PACKAGE_GROUP` is one such dropped type, so a change to a
+    // package_group's `packages` list is never reflected in any hash -- even
+    // though it genuinely alters downstream visibility and can flip a consumer
+    // from building to failing.
+    //
+    // The `package_group_dropped` workspace:
+    //   //lib:consumers -- a package_group (created by a macro in lib/defs.bzl)
+    //                      that gates the visibility of //lib:thing. Its
+    //                      allow-list lives in defs.bzl as `ALLOWED`.
+    //   //lib:thing     -- a genrule declared DIRECTLY (not via the macro), so
+    //                      its per-rule `.bzl` seed never picks up defs.bzl.
+    //   //consumer:use_thing -- depends on //lib:thing.
+    //
+    // Between the two checkouts we edit ONLY lib/defs.bzl, emptying `ALLOWED`.
+    // That revokes //consumer's visibility of //lib:thing: workspace A builds
+    // //consumer:use_thing successfully, workspace B fails visibility analysis
+    // for it. The BUILD files and every native rule are byte-for-byte identical
+    // across the checkouts, so the sole semantic change rides entirely on the
+    // (dropped) PACKAGE_GROUP target.
+    //
+    // Current (buggy) behaviour: bazel-diff reports ZERO impacted targets. When
+    // #441's under-invalidation is fixed (e.g. by supporting PACKAGE_GROUP
+    // targets and their reverse-visibility dependents), the changed package_group
+    // -- and/or //consumer:use_thing -- should start appearing here and this
+    // assertion will flag that the behaviour changed.
+    val workspaceA = copyTestWorkspace("package_group_dropped")
+    val workspaceB = copyTestWorkspace("package_group_dropped")
+
+    // The ONLY change: revoke //consumer's visibility by emptying the macro's
+    // allow-list. Nothing else -- no BUILD file, no rule attribute -- changes.
+    val defsInB = File(workspaceB, "lib/defs.bzl")
+    defsInB.writeText(defsInB.readText().replace("ALLOWED = [\"//consumer\"]", "ALLOWED = []"))
+
+    val outputDir = temp.newFolder()
+    val from = File(outputDir, "starting_hashes.json")
+    val to = File(outputDir, "final_hashes.json")
+    val impactedTargetsOutput = File(outputDir, "impacted_targets.txt")
+
+    val cli = CommandLine(BazelDiff())
+
+    assertThat(
+            cli.execute(
+                "generate-hashes", "-w", workspaceA.absolutePath, "-b", "bazel", from.absolutePath))
+        .isEqualTo(0)
+    assertThat(
+            cli.execute(
+                "generate-hashes", "-w", workspaceB.absolutePath, "-b", "bazel", to.absolutePath))
+        .isEqualTo(0)
+    assertThat(
+            cli.execute(
+                "get-impacted-targets",
+                "-w",
+                workspaceB.absolutePath,
+                "-b",
+                "bazel",
+                "-sh",
+                from.absolutePath,
+                "-fh",
+                to.absolutePath,
+                "-o",
+                impactedTargetsOutput.absolutePath))
+        .isEqualTo(0)
+
+    val impacted =
+        filterBazelDiffInternalTargets(
+            impactedTargetsOutput.readLines().filter { it.isNotBlank() }.toSet())
+
+    // The genuinely-affected consumer is NOT reported -- pinning the false
+    // negative. (It flips from building to failing visibility analysis, yet its
+    // hash, and every other rule/source-file hash, is unchanged.)
+    assertThat(impacted.contains("//consumer:use_thing")).isEqualTo(false)
+
+    // Nothing at all is reported: the package_group is dropped, so the change is
+    // completely invisible to bazel-diff. This is exactly the under-invalidation
+    // #441 describes.
+    assertThat(impacted)
+        .transform(
+            "editing a dropped PACKAGE_GROUP's `packages` list should surface an impacted target, but got: $impacted") {
+              it.size
+            }
+        .isEqualTo(0)
+  }
+
   /**
    * Returns the Bazel version triple by running `bazel version`, or null if it cannot be
    * determined.

cli/src/test/resources/workspaces/package_group_dropped/MODULE.bazel

@@ -0,0 +1,4 @@
+module(
+    name = "package_group_dropped",
+    version = "0.0.0",
+)

cli/src/test/resources/workspaces/package_group_dropped/consumer/BUILD

@@ -0,0 +1,11 @@
+# Depends on //lib:thing, whose visibility is controlled by the //lib:consumers
+# package_group. Emptying that package_group's `packages` list (the only change
+# between the two checkouts) would make this target fail visibility analysis in
+# a real build -- yet bazel-diff reports zero impacted targets because the
+# package_group is dropped from the hashed graph. See //lib:BUILD.
+genrule(
+    name = "use_thing",
+    srcs = ["//lib:thing"],
+    outs = ["use_thing.txt"],
+    cmd = "cp $< $@",
+)

cli/src/test/resources/workspaces/package_group_dropped/lib/BUILD

@@ -0,0 +1,29 @@
+# Reproducer workspace for https://github.com/Tinder/bazel-diff/issues/441
+#
+# The under-invalidation (false-negative) half of the issue: bazel-diff drops
+# any target whose query `Discriminator` is not RULE / SOURCE_FILE /
+# GENERATED_FILE. `PACKAGE_GROUP` is one such dropped type
+# (BazelQueryService.toBazelTarget -> logs "Unsupported target type" and
+# returns null). Because the package_group is never hashed, a change to its
+# `packages` list -- which genuinely alters downstream visibility and can flip
+# a consumer from building to failing -- is invisible to bazel-diff, so
+# `get-impacted-targets` reports NOTHING.
+#
+# `consumers` is created by a macro (see defs.bzl) whose allow-list lives in
+# that `.bzl`. Between the two checkouts we edit ONLY defs.bzl (ALLOWED -> []),
+# so the BUILD files and `:thing` stay byte-identical; the only semantic change
+# rides entirely on the dropped PACKAGE_GROUP target. `:thing` is declared
+# directly (NOT via the macro) so its per-rule `.bzl` seed never picks up
+# defs.bzl -- isolating the dropped-package_group bug from the separate `.bzl`
+# provenance behaviour that is the over-invalidation half of #441.
+
+load(":defs.bzl", "define_consumers")
+
+define_consumers(name = "consumers")
+
+genrule(
+    name = "thing",
+    outs = ["thing.txt"],
+    cmd = "echo thing > $@",
+    visibility = [":consumers"],
+)

cli/src/test/resources/workspaces/package_group_dropped/lib/defs.bzl

@@ -0,0 +1,18 @@
+"""Macro that creates the visibility package_group for `//lib`.
+
+The `packages` allow-list lives here (not in the BUILD file) so that the ONLY
+change between the two checkouts is to a `.bzl` that feeds a `package_group`.
+The BUILD files, and every native rule in them, are byte-for-byte identical
+across the two revisions -- so the sole semantic change is carried entirely by
+the (dropped) PACKAGE_GROUP target. This mirrors issue #441's macro-created
+package_group experiment. Flip `ALLOWED` to `[]` in the "after" checkout to
+revoke `//consumer`'s visibility of `//lib:thing`.
+"""
+
+ALLOWED = ["//consumer"]
+
+def define_consumers(name):
+    native.package_group(
+        name = name,
+        packages = ALLOWED,
+    )