Use a sorted set for the test suite expansion key to prevent non-dete…

…rminism.

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

src/main/java/com/google/devtools/build/lib/skyframe/TestSuiteExpansionFunction.java

@@ -55,16 +55,17 @@ public SkyValue compute(SkyKey key, Environment env) {
       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);
       } else if (TargetUtils.isTestSuiteRule(target)) {
         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 @@ public SkyValue compute(SkyKey key, Environment env) {
     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(

src/main/java/com/google/devtools/build/lib/skyframe/TestSuiteExpansionValue.java

@@ -16,6 +16,7 @@
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Preconditions;
 import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.ImmutableSortedSet;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.cmdline.ResolvedTargets;
 import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
@@ -27,6 +28,7 @@
 import java.io.ObjectInputStream;
 import java.io.ObjectOutputStream;
 import java.io.Serializable;
+import java.util.Collection;
 
 /**
  * A value referring to a computed set of resolved targets. This is used for the results of target
@@ -67,18 +69,19 @@ private void readObjectNoData() {
    * @param targets the set of targets to be expanded
    */
   @ThreadSafe
-  public static SkyKey key(ImmutableSet<Label> targets) {
-    return new SkyKey(SkyFunctions.TEST_SUITE_EXPANSION, new TestSuiteExpansion(targets));
+  public static SkyKey key(Collection<Label> targets) {
+    return new SkyKey(SkyFunctions.TEST_SUITE_EXPANSION,
+        new TestSuiteExpansion(ImmutableSortedSet.copyOf(targets)));
   }
 
   /**
    * A list of targets of which all test suites should be expanded.
    */
   @ThreadSafe
   static final class TestSuiteExpansion implements Serializable {
-    private final ImmutableSet<Label> targets;
+    private final ImmutableSortedSet<Label> targets;
 
-    public TestSuiteExpansion(ImmutableSet<Label> targets) {
+    public TestSuiteExpansion(ImmutableSortedSet<Label> targets) {
       this.targets = targets;
     }