commit ef758fa57c31ed7c04937c3b4a263a0e25315bc2
author Googler <yuyueny@google.com> Mon Dec 18 09:13:50 2023 -0800
committer Copybara-Service <copybara-worker@google.com> Mon Dec 18 09:15:22 2023 -0800
tree 04a77d22d9a7be158590c1b709c6acf05cbc2c0d
parent aa1e0b431f1eac8acb359e5e13b1a209f8c2e246

Automated rollback of commit 07eb2d77f94f9e11e64082dab9581ad7acf44dc7.

*** Reason for rollback ***

Re-fix the `subpackages()` bug when considering a corner case

In the initial change, we have introduced another bug:

Consider computing `subpackages(["sub/**"])` while `sub/BUILD` exists, we should return "sub", which is the behavior of file/directory glob matching. However, the initial fix returns EMPTY in this case.

This change also adds more test coverage for calling `subpackages(["sub/**"])` when `sub` is a subpackage.

PiperOrigin-RevId: 591917000
Change-Id: Iab41cc59b0da7f7794a90cd10aaac8900ee0d79b

src/test/java/com/google/devtools/build/lib/packages/GlobCacheTest.java

diff --git a/src/test/java/com/google/devtools/build/lib/packages/GlobCacheTest.java b/src/test/java/com/google/devtools/build/lib/packages/GlobCacheTest.java
index db5b460..0469085 100644
--- a/src/test/java/com/google/devtools/build/lib/packages/GlobCacheTest.java
+++ b/src/test/java/com/google/devtools/build/lib/packages/GlobCacheTest.java
@@ -338,11 +338,54 @@
   }
 
   @Test
-  public void testSubpackages() throws Exception {
+  public void testSubpackages_noWildcard() throws Exception {
+    assertThat(cache.globUnsorted(list("sub/sub.js"), list(), Globber.Operation.SUBPACKAGES, true))
+        .isEmpty();
+  }
+
+  @Test
+  public void testSubpackages_simpleDoubleStar() throws Exception {
     assertThat(cache.globUnsorted(list("**"), list(), Globber.Operation.SUBPACKAGES, true))
         .containsExactly("sub");
   }
 
+  @Test
+  public void testSubpackages_onlySub() throws Exception {
+    assertThat(cache.globUnsorted(list("sub"), list(), Globber.Operation.SUBPACKAGES, true))
+        .containsExactly("sub");
+  }
+
+  @Test
+  public void testSubpackages_singleStarsAfterSub() throws Exception {
+    assertThat(cache.globUnsorted(list("sub/*"), list(), Globber.Operation.SUBPACKAGES, true))
+        .isEmpty();
+  }
+
+  @Test
+  public void testSubpackages_doubleStarsAfterSub() throws Exception {
+    assertThat(cache.globUnsorted(list("sub/**"), list(), Globber.Operation.SUBPACKAGES, true))
+        .containsExactly("sub");
+  }
+
+  @Test
+  public void testSubpackages_twoDoubleStarsAfterSub() throws Exception {
+    // Both `**`s are considered to match no path fragments.
+    assertThat(cache.globUnsorted(list("sub/**/**"), list(), Globber.Operation.SUBPACKAGES, true))
+        .containsExactly("sub");
+  }
+
+  @Test
+  public void testSubpackages_doubleStarsAndOtherPathAfterSub() throws Exception {
+    assertThat(cache.globUnsorted(list("sub/**/foo"), list(), Globber.Operation.SUBPACKAGES, true))
+        .isEmpty();
+  }
+
+  @Test
+  public void testSubpackages_doubleStarWithTrailingPattern() throws Exception {
+    assertThat(cache.globUnsorted(list("**/bar"), list(), Globber.Operation.SUBPACKAGES, true))
+        .isEmpty();
+  }
+
   private void assertEmpty(Collection<?> glob) {
     assertThat(glob).isEmpty();
   }

src/test/java/com/google/devtools/build/lib/packages/NativeSubpackagesTest.java

diff --git a/src/test/java/com/google/devtools/build/lib/packages/NativeSubpackagesTest.java b/src/test/java/com/google/devtools/build/lib/packages/NativeSubpackagesTest.java
index 91f3410..f7b41a6 100644
--- a/src/test/java/com/google/devtools/build/lib/packages/NativeSubpackagesTest.java
+++ b/src/test/java/com/google/devtools/build/lib/packages/NativeSubpackagesTest.java
@@ -26,15 +26,16 @@
 import com.google.devtools.build.lib.vfs.ModifiedFileSet;
 import com.google.devtools.build.lib.vfs.PathFragment;
 import com.google.devtools.build.lib.vfs.Root;
+import com.google.testing.junit.testparameterinjector.TestParameter;
+import com.google.testing.junit.testparameterinjector.TestParameterInjector;
 import java.io.IOException;
 import java.util.List;
 import java.util.stream.Collectors;
 import org.junit.Test;
 import org.junit.runner.RunWith;
-import org.junit.runners.JUnit4;
 
 /** Tests for {@code native.subpackages} function. */
-@RunWith(JUnit4.class)
+@RunWith(TestParameterInjector.class)
 public class NativeSubpackagesTest extends BuildViewTestCase {
 
   private static final String ALL_SUBDIRS = "**";
@@ -50,7 +51,7 @@
 
   @Test
   public void subpackages_simple_include() throws Exception {
-    makeSubpackageFileGroup("test/starlark/BUILD", "sub1/**", null, null);
+    makeSubpackageFileGroup("test/starlark/BUILD", "sub1", null, null);
 
     makeFilesSubPackage("test/starlark/sub");
     makeFilesSubPackage("test/starlark/sub1");
@@ -232,19 +233,26 @@
   @Test
   public void includeValidMatchSubdir() throws Exception {
     scratch.file("foo/subdir/BUILD");
-    scratch.file(
-        "foo/BUILD", "[sh_library(name = p) for p in subpackages(include = ['subdir/*'])]");
-
+    scratch.file("foo/BUILD", "[sh_library(name = p) for p in subpackages(include = ['subdir'])]");
     getConfiguredTargetAndData("//foo:subdir");
   }
 
   @Test
-  public void includeValidSubMatchSubdir() throws Exception {
+  public void includeValidSubMatchSubdir(
+      @TestParameter({
+            "subdir/*/deeper",
+            "subdir/sub*/deeper",
+            "subdir/**",
+            "subdir/*/deeper/**",
+            "subdir/**/deeper/**"
+          })
+          String expression)
+      throws Exception {
     makeFilesSubPackage("test/starlark/subdir/sub/deeper");
     makeFilesSubPackage("test/starlark/subdir/sub2/deeper");
     makeFilesSubPackage("test/starlark/subdir/sub3/deeper");
 
-    makeSubpackageFileGroup("test/starlark/BUILD", "subdir/*/deeper", null, null);
+    makeSubpackageFileGroup("test/starlark/BUILD", expression, null, null);
 
     assertAttrLabelList(
         "//test/starlark:files",