Enforce the current ordering semantics for GlobFunction. These semantics are

fairly arbitrary, so future modifiers of GlobFunction can change the order if
they need to, but enforcing the order prevents non-determinism from
accidentally creeping in.

--
MOS_MIGRATED_REVID=116986512

1 files changed, 35 insertions, 29 deletions

diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/GlobFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/GlobFunctionTest.java
index 8fc2eb3393..e553ef48e7 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/GlobFunctionTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/GlobFunctionTest.java
@@ -284,7 +284,7 @@ public abstract class GlobFunctionTest {
   public void testStarStarDoesNotCrossPackageBoundary() throws Exception {
     FileSystemUtils.createEmptyFile(pkgPath.getRelative("foo/bar/BUILD"));
     // "foo/bar" should not be in the results because foo/bar is a separate package.
-    assertGlobMatches("foo/**", /* => */ "foo", "foo/barnacle", "foo/barnacle/wiz");
+    assertGlobMatches("foo/**", /* => */ "foo/barnacle/wiz", "foo/barnacle", "foo");
   }
 
   @Test
@@ -293,7 +293,7 @@ public abstract class GlobFunctionTest {
     FileSystemUtils.createEmptyFile(writableRoot.getRelative("pkg/foo/bar/BUILD"));
     // "foo/bar" should not be in the results because foo/bar is detected as a separate package,
     // even though it is under a different package path.
-    assertGlobMatches("foo/**", /* => */ "foo", "foo/barnacle", "foo/barnacle/wiz");
+    assertGlobMatches("foo/**", /* => */ "foo/barnacle/wiz", "foo/barnacle", "foo");
   }
 
   private void assertGlobMatches(String pattern, String... expecteds) throws Exception {
@@ -306,10 +306,16 @@ public abstract class GlobFunctionTest {
 
   private void assertGlobMatches(boolean excludeDirs, String pattern, String... expecteds)
       throws Exception {
+    // The order requirement is not strictly necessary -- a change to GlobFunction semantics that
+    // changes the output order is fine, but we require that the order be the same here to detect
+    // potential non-determinism in output order, which would be bad.
+    // The current order in the case of "**" or "*" is roughly that of nestedset.Order.STABLE_ORDER,
+    // putting subdirectories before directories, but putting ordinary files after their parent
+    // directories.
     assertThat(
             Iterables.transform(
                 runGlob(excludeDirs, pattern).getMatches(), Functions.toStringFunction()))
-        .containsExactlyElementsIn(ImmutableList.copyOf(expecteds));
+        .containsExactlyElementsIn(ImmutableList.copyOf(expecteds)).inOrder();
   }
 
   private void assertGlobsEqual(String pattern1, String pattern2) throws Exception {
@@ -450,7 +456,7 @@ public abstract class GlobFunctionTest {
     }
     // Note that these are not in the result: ".", ".."
     assertGlobMatches(
-        "*", "a1", "a2", "not.hidden", "foo", "fool", "food", "BUILD", ".hidden", "..also.hidden");
+        "*", "..also.hidden", ".hidden", "BUILD", "a1", "a2", "foo", "food", "fool", "not.hidden");
     assertGlobMatches("*.hidden", "not.hidden");
   }
 
@@ -458,63 +464,63 @@ public abstract class GlobFunctionTest {
   public void testDoubleStar() throws Exception {
     assertGlobMatches(
         "**",
-        "BUILD",
-        "a1",
-        "a1/b1",
         "a1/b1/c",
+        "a1/b1",
+        "a1",
         "a2",
-        "foo",
-        "foo/bar",
         "foo/bar/wiz",
         "foo/bar/wiz/file",
-        "foo/barnacle",
+        "foo/bar",
         "foo/barnacle/wiz",
-        "food",
-        "food/barnacle",
+        "foo/barnacle",
+        "foo",
         "food/barnacle/wiz",
-        "fool",
+        "food/barnacle",
+        "food",
+        "fool/barnacle/wiz",
         "fool/barnacle",
-        "fool/barnacle/wiz");
+        "fool",
+        "BUILD");
   }
 
   @Test
   public void testDoubleStarExcludeDirs() throws Exception {
-    assertGlobWithoutDirsMatches("**", "BUILD", "foo/bar/wiz/file");
+    assertGlobWithoutDirsMatches("**", "foo/bar/wiz/file", "BUILD");
   }
 
   @Test
   public void testDoubleDoubleStar() throws Exception {
     assertGlobMatches(
         "**/**",
-        "BUILD",
-        "a1",
-        "a1/b1",
         "a1/b1/c",
+        "a1/b1",
+        "a1",
         "a2",
-        "foo",
-        "foo/bar",
         "foo/bar/wiz",
         "foo/bar/wiz/file",
-        "foo/barnacle",
+        "foo/bar",
         "foo/barnacle/wiz",
-        "food",
-        "food/barnacle",
+        "foo/barnacle",
+        "foo",
         "food/barnacle/wiz",
-        "fool",
+        "food/barnacle",
+        "food",
+        "fool/barnacle/wiz",
         "fool/barnacle",
-        "fool/barnacle/wiz");
+        "fool",
+        "BUILD");
   }
 
   @Test
   public void testDirectoryWithDoubleStar() throws Exception {
     assertGlobMatches(
         "foo/**",
-        "foo",
-        "foo/bar",
         "foo/bar/wiz",
         "foo/bar/wiz/file",
+        "foo/bar",
+        "foo/barnacle/wiz",
         "foo/barnacle",
-        "foo/barnacle/wiz");
+        "foo");
   }
 
   @Test
@@ -535,8 +541,8 @@ public abstract class GlobFunctionTest {
     assertGlobMatches(
         "foo/**/wiz",
         "foo/bar/wiz",
-        "foo/barnacle/baz/wiz",
         "foo/barnacle/wiz",
+        "foo/barnacle/baz/wiz",
         "foo/barnacle/wiz/wiz");
   }