Use a sorted set for the test suite expansion key to prevent non-determinism.

While a normal set is theoretically sufficient, it can cause hard-to-reproduce
problems. In particular, the iteration order of the expansion result depends
on the iteration order of the requested targets. If there are multiple
requests for the same set of targets, but with different orders, the results
would depend on which request was made first. If a downstream function also
inadvertantly depends on the iteration order, it can be hard to debug why it
ended up with a different order than it requested.

Alternatively, we could sort the result before returning it. We generally
expect the key set to be smaller than the result set, so this should be
ever so slightly more efficient.

--
MOS_MIGRATED_REVID=105765514

1 files changed, 7 insertions, 4 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/TestSuiteExpansionFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/TestSuiteExpansionFunction.java
index bba5946413..ad2cff7a29 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/TestSuiteExpansionFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/TestSuiteExpansionFunction.java
@@ -55,8 +55,8 @@ final class TestSuiteExpansionFunction implements SkyFunction {
       return null;
     }
 
-    ResolvedTargets.Builder<Target> result = ResolvedTargets.builder();
-    result.mergeError(targets.hasError());
+    Set<Target> result = new LinkedHashSet<>();
+    boolean hasError = targets.hasError();
     for (Target target : targets.getTargets()) {
       if (TargetUtils.isTestRule(target)) {
         result.add(target);
@@ -64,7 +64,8 @@ final class TestSuiteExpansionFunction implements SkyFunction {
         TestsInSuiteValue value = (TestsInSuiteValue) testsInSuites.get(
             TestsInSuiteValue.key(target, true));
         if (value != null) {
-          result.merge(value.getTargets());
+          result.addAll(value.getTargets().getTargets());
+          hasError |= value.getTargets().hasError();
         }
       } else {
         result.add(target);
@@ -73,7 +74,9 @@ final class TestSuiteExpansionFunction implements SkyFunction {
     if (env.valuesMissing()) {
       return null;
     }
-    return new TestSuiteExpansionValue(result.build());
+    // We use ResolvedTargets in order to associate an error flag; the result should never contain
+    // any filtered targets.
+    return new TestSuiteExpansionValue(new ResolvedTargets<Target>(result, hasError));
   }
 
   static ResolvedTargets<Target> labelsToTargets(