Fix SkyframeLoadingPhaseRunner posting of EventBus events.

The TargetParsingCompleteEvent was posting the post-expansion targets, and the
LoadingPhaseCompleteEvent was missing the test-suite targets.

--
MOS_MIGRATED_REVID=114312273

6 files changed, 92 insertions, 48 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/pkgcache/LegacyLoadingPhaseRunner.java b/src/main/java/com/google/devtools/build/lib/pkgcache/LegacyLoadingPhaseRunner.java
index 9b6b6841b3..7fda021118 100644
--- a/src/main/java/com/google/devtools/build/lib/pkgcache/LegacyLoadingPhaseRunner.java
+++ b/src/main/java/com/google/devtools/build/lib/pkgcache/LegacyLoadingPhaseRunner.java
@@ -281,7 +281,8 @@ public final class LegacyLoadingPhaseRunner extends LoadingPhaseRunner {
   private void postLoadingLogging(EventBus eventBus, ImmutableSet<Target> originalTargetsToLoad,
       ImmutableSet<Target> expandedTargetsToLoad, Stopwatch timer) {
     Set<Target> testSuiteTargets = Sets.difference(originalTargetsToLoad, expandedTargetsToLoad);
-    eventBus.post(new LoadingPhaseCompleteEvent(expandedTargetsToLoad, testSuiteTargets,
+    eventBus.post(new LoadingPhaseCompleteEvent(
+        expandedTargetsToLoad, ImmutableSet.copyOf(testSuiteTargets),
         packageManager.getStatistics(), timer.stop().elapsed(TimeUnit.MILLISECONDS)));
     LOG.info("Loading phase finished"); 
   }
diff --git a/src/main/java/com/google/devtools/build/lib/pkgcache/LoadingPhaseCompleteEvent.java b/src/main/java/com/google/devtools/build/lib/pkgcache/LoadingPhaseCompleteEvent.java
index 914ce3b530..4f42c0fe8d 100644
--- a/src/main/java/com/google/devtools/build/lib/pkgcache/LoadingPhaseCompleteEvent.java
+++ b/src/main/java/com/google/devtools/build/lib/pkgcache/LoadingPhaseCompleteEvent.java
@@ -14,18 +14,18 @@
 package com.google.devtools.build.lib.pkgcache;
 
 import com.google.common.base.Function;
+import com.google.common.base.Preconditions;
+import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Iterables;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.packages.Target;
 
-import java.util.Collection;
-
 /**
  * This event is fired after the loading phase is complete.
  */
-public class LoadingPhaseCompleteEvent {
-  private final Collection<Target> targets;
-  private final Collection<Target> filteredTargets;
+public final class LoadingPhaseCompleteEvent {
+  private final ImmutableSet<Target> targets;
+  private final ImmutableSet<Target> filteredTargets;
   private final PackageManager.PackageManagerStatistics pkgManagerStats;
   private final long timeInMs;
 
@@ -35,11 +35,12 @@ public class LoadingPhaseCompleteEvent {
    * @param targets the set of active targets that remain
    * @param pkgManagerStats statistics about the package cache
    */
-  public LoadingPhaseCompleteEvent(Collection<Target> targets, Collection<Target> filteredTargets,
-      PackageManager.PackageManagerStatistics pkgManagerStats, long timeInMs) {
-    this.targets = targets;
-    this.filteredTargets = filteredTargets;
-    this.pkgManagerStats = pkgManagerStats;
+  public LoadingPhaseCompleteEvent(ImmutableSet<Target> targets,
+      ImmutableSet<Target> filteredTargets, PackageManager.PackageManagerStatistics pkgManagerStats,
+      long timeInMs) {
+    this.targets = Preconditions.checkNotNull(targets);
+    this.filteredTargets = Preconditions.checkNotNull(filteredTargets);
+    this.pkgManagerStats = Preconditions.checkNotNull(pkgManagerStats);
     this.timeInMs = timeInMs;
   }
 
@@ -47,14 +48,14 @@ public class LoadingPhaseCompleteEvent {
    * @return The set of active targets remaining, which is a subset of the
    *         targets we attempted to load.
    */
-  public Collection<Target> getTargets() {
+  public ImmutableSet<Target> getTargets() {
     return targets;
   }
 
   /**
    * @return The set of filtered targets.
    */
-  public Collection<Target> getFilteredTargets() {
+  public ImmutableSet<Target> getFilteredTargets() {
     return filteredTargets;
   }
 
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
index 12c38a8cba..92c680424f 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
@@ -1728,7 +1728,6 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory {
    * Skyframe-based implementation of {@link LoadingPhaseRunner} based on {@link
    * TargetPatternPhaseFunction}.
    */
-  // TODO(ulfjack): This is still incomplete.
   final class SkyframeLoadingPhaseRunner extends LoadingPhaseRunner {
     private final TargetPatternEvaluator targetPatternEvaluator;
     private final Set<String> ruleClassNames;
@@ -1785,17 +1784,14 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory {
       long time = timer.stop().elapsed(TimeUnit.MILLISECONDS);
 
       TargetPatternPhaseValue patternParsingValue = evalResult.get(key);
-      // TODO(ulfjack): The first event should be pre-test_suite expansion, the second post.
-      eventBus.post(new TargetParsingCompleteEvent(patternParsingValue.getTargets(),
+      eventBus.post(new TargetParsingCompleteEvent(patternParsingValue.getOriginalTargets(),
           patternParsingValue.getFilteredTargets(), patternParsingValue.getTestFilteredTargets(),
           time));
       if (callback != null) {
         callback.notifyTargets(patternParsingValue.getTargets());
       }
       eventBus.post(new LoadingPhaseCompleteEvent(
-          /*was expandedTargetsToLoad*/patternParsingValue.getTargets(),
-          // TODO(ulfjack): Should be: Sets.difference(originalTargetsToLoad, expandedTargetsToLoad)
-          /*was testSuiteTargets*/ImmutableSet.<Target>of(),
+          patternParsingValue.getTargets(), patternParsingValue.getTestSuiteTargets(),
           packageManager.getStatistics(), /*timeInMs=*/0));
       return patternParsingValue.toLoadingResult();
     }
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/TargetPatternPhaseFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/TargetPatternPhaseFunction.java
index ea30c0584c..9608878f34 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/TargetPatternPhaseFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/TargetPatternPhaseFunction.java
@@ -159,9 +159,12 @@ final class TargetPatternPhaseFunction implements SkyFunction {
         expandedTargetsBuilder.add(target);
       }
     }
-    targets = expandedTargetsBuilder.build();
-    return new TargetPatternPhaseValue(targets.getTargets(), testsToRun, preExpansionError,
-        targets.hasError(), filteredTargets, testFilteredTargets);
+    ResolvedTargets<Target> expandedTargets = expandedTargetsBuilder.build();
+    Set<Target> testSuiteTargets =
+        Sets.difference(targets.getTargets(), expandedTargets.getTargets());
+    return new TargetPatternPhaseValue(expandedTargets.getTargets(), testsToRun, preExpansionError,
+        expandedTargets.hasError(), filteredTargets, testFilteredTargets,
+        targets.getTargets(), ImmutableSet.copyOf(testSuiteTargets));
   }
 
   /**
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/TargetPatternPhaseValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/TargetPatternPhaseValue.java
index becc737239..4c864db3d8 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/TargetPatternPhaseValue.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/TargetPatternPhaseValue.java
@@ -52,15 +52,23 @@ public final class TargetPatternPhaseValue implements SkyValue {
   private final ImmutableSet<Target> filteredTargets;
   private final ImmutableSet<Target> testFilteredTargets;
 
+  // These two fields are only for the purposes of generating the TargetParsingCompleteEvent.
+  // TODO(ulfjack): Support EventBus event posting in Skyframe, and remove this code again.
+  private final ImmutableSet<Target> originalTargets;
+  private final ImmutableSet<Target> testSuiteTargets;
+
   TargetPatternPhaseValue(ImmutableSet<Target> targets, @Nullable ImmutableSet<Target> testsToRun,
       boolean hasError, boolean hasPostExpansionError, ImmutableSet<Target> filteredTargets,
-      ImmutableSet<Target> testFilteredTargets) {
+      ImmutableSet<Target> testFilteredTargets, ImmutableSet<Target> originalTargets,
+      ImmutableSet<Target> testSuiteTargets) {
     this.targets = Preconditions.checkNotNull(targets);
     this.testsToRun = testsToRun;
     this.hasError = hasError;
     this.hasPostExpansionError = hasPostExpansionError;
     this.filteredTargets = Preconditions.checkNotNull(filteredTargets);
     this.testFilteredTargets = Preconditions.checkNotNull(testFilteredTargets);
+    this.originalTargets = Preconditions.checkNotNull(originalTargets);
+    this.testSuiteTargets = Preconditions.checkNotNull(testSuiteTargets);
   }
 
   public ImmutableSet<Target> getTargets() {
@@ -88,6 +96,14 @@ public final class TargetPatternPhaseValue implements SkyValue {
     return testFilteredTargets;
   }
 
+  public ImmutableSet<Target> getOriginalTargets() {
+    return originalTargets;
+  }
+
+  public ImmutableSet<Target> getTestSuiteTargets() {
+    return testSuiteTargets;
+  }
+
   public LoadingResult toLoadingResult() {
     return new LoadingResult(hasError(), hasPostExpansionError(), getTargets(), getTestsToRun(),
         ImmutableMap.<PackageIdentifier, Path>of());
diff --git a/src/test/java/com/google/devtools/build/lib/pkgcache/LoadingPhaseRunnerTest.java b/src/test/java/com/google/devtools/build/lib/pkgcache/LoadingPhaseRunnerTest.java
index 1e0b234d89..65c10cb007 100644
--- a/src/test/java/com/google/devtools/build/lib/pkgcache/LoadingPhaseRunnerTest.java
+++ b/src/test/java/com/google/devtools/build/lib/pkgcache/LoadingPhaseRunnerTest.java
@@ -27,6 +27,7 @@ import com.google.common.base.Predicates;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableListMultimap;
 import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Iterables;
 import com.google.common.eventbus.EventBus;
 import com.google.common.eventbus.Subscribe;
@@ -230,8 +231,8 @@ public class LoadingPhaseRunnerTest {
     tester.addFile("my_test/BUILD",
         "sh_test(name = 'my_test', srcs = ['test.cc'])");
     assertNoErrors(tester.loadTests("-//my_test"));
-    assertThat(tester.filteredTargets).containsExactlyElementsIn(getTargets());
-    assertThat(tester.testFilteredTargets).containsExactlyElementsIn(getTargets());
+    assertThat(tester.getFilteredTargets()).containsExactlyElementsIn(getTargets());
+    assertThat(tester.getTestFilteredTargets()).containsExactlyElementsIn(getTargets());
   }
 
   @Test
@@ -239,8 +240,8 @@ public class LoadingPhaseRunnerTest {
     tester.addFile("my_library/BUILD",
         "cc_library(name = 'my_library', srcs = ['test.cc'])");
     assertNoErrors(tester.loadTests("-//my_library"));
-    assertThat(tester.filteredTargets).containsExactlyElementsIn(getTargets());
-    assertThat(tester.testFilteredTargets).containsExactlyElementsIn(getTargets());
+    assertThat(tester.getFilteredTargets()).containsExactlyElementsIn(getTargets());
+    assertThat(tester.getTestFilteredTargets()).containsExactlyElementsIn(getTargets());
   }
 
   private void writeBuildFilesForTestFiltering() throws Exception {
@@ -258,8 +259,8 @@ public class LoadingPhaseRunnerTest {
         .containsExactlyElementsIn(getTargets("//tests:t1", "//tests:t2"));
     assertThat(loadingResult.getTestsToRun())
         .containsExactlyElementsIn(getTargets("//tests:t1", "//tests:t2"));
-    assertThat(tester.filteredTargets).containsExactlyElementsIn(getTargets());
-    assertThat(tester.testFilteredTargets).containsExactlyElementsIn(getTargets());
+    assertThat(tester.getFilteredTargets()).containsExactlyElementsIn(getTargets());
+    assertThat(tester.getTestFilteredTargets()).containsExactlyElementsIn(getTargets());
   }
 
   @Test
@@ -271,8 +272,8 @@ public class LoadingPhaseRunnerTest {
         .containsExactlyElementsIn(getTargets("//tests:t1", "//tests:t2"));
     assertThat(loadingResult.getTestsToRun())
         .containsExactlyElementsIn(getTargets("//tests:t1", "//tests:t2"));
-    assertThat(tester.filteredTargets).containsExactlyElementsIn(getTargets());
-    assertThat(tester.testFilteredTargets).containsExactlyElementsIn(getTargets());
+    assertThat(tester.getFilteredTargets()).containsExactlyElementsIn(getTargets());
+    assertThat(tester.getTestFilteredTargets()).containsExactlyElementsIn(getTargets());
   }
 
   @Test
@@ -283,8 +284,8 @@ public class LoadingPhaseRunnerTest {
     assertThat(loadingResult.getTargets())
         .containsExactlyElementsIn(getTargets("//tests:t1", "//tests:t2"));
     assertThat(loadingResult.getTestsToRun()).containsExactlyElementsIn(getTargets("//tests:t1"));
-    assertThat(tester.filteredTargets).containsExactlyElementsIn(getTargets());
-    assertThat(tester.testFilteredTargets).containsExactlyElementsIn(getTargets());
+    assertThat(tester.getFilteredTargets()).containsExactlyElementsIn(getTargets());
+    assertThat(tester.getTestFilteredTargets()).containsExactlyElementsIn(getTargets());
   }
 
   @Test
@@ -294,8 +295,8 @@ public class LoadingPhaseRunnerTest {
     LoadingResult loadingResult = assertNoErrors(tester.loadTests("//tests:all"));
     assertThat(loadingResult.getTargets()).containsExactlyElementsIn(getTargets("//tests:t1"));
     assertThat(loadingResult.getTestsToRun()).containsExactlyElementsIn(getTargets("//tests:t1"));
-    assertThat(tester.filteredTargets).containsExactlyElementsIn(getTargets());
-    assertThat(tester.testFilteredTargets).containsExactlyElementsIn(getTargets("//tests:t2"));
+    assertThat(tester.getFilteredTargets()).containsExactlyElementsIn(getTargets());
+    assertThat(tester.getTestFilteredTargets()).containsExactlyElementsIn(getTargets("//tests:t2"));
   }
 
   @Test
@@ -307,8 +308,8 @@ public class LoadingPhaseRunnerTest {
         .containsExactlyElementsIn(getTargets("//tests:t1", "//tests:t3"));
     assertThat(loadingResult.getTestsToRun())
         .containsExactlyElementsIn(getTargets("//tests:t1", "//tests:t3"));
-    assertThat(tester.filteredTargets).containsExactlyElementsIn(getTargets());
-    assertThat(tester.testFilteredTargets).containsExactlyElementsIn(getTargets("//tests:t2"));
+    assertThat(tester.getFilteredTargets()).containsExactlyElementsIn(getTargets());
+    assertThat(tester.getTestFilteredTargets()).containsExactlyElementsIn(getTargets("//tests:t2"));
   }
 
   @Test
@@ -324,6 +325,10 @@ public class LoadingPhaseRunnerTest {
       assertThat(loadingResult.getPackageRoots().entrySet())
           .contains(entryFor(PackageIdentifier.createInDefaultRepo("cc"), tester.getWorkspace()));
     }
+    assertThat(tester.getOriginalTargets())
+        .containsExactlyElementsIn(getTargets("//cc:tests", "//cc:my_test"));
+    assertThat(tester.getTestSuiteTargets())
+        .containsExactlyElementsIn(getTargets("//cc:tests"));
   }
 
   @Test
@@ -384,8 +389,9 @@ public class LoadingPhaseRunnerTest {
         .containsExactlyElementsIn(getTargets("//foo:foo", "//foo:baz"));
     assertThat(loadingResult.getTestsToRun())
         .containsExactlyElementsIn(getTargets("//foo:foo", "//foo:baz"));
-    assertThat(tester.filteredTargets).containsExactlyElementsIn(getTargets());
-    assertThat(tester.testFilteredTargets).containsExactlyElementsIn(getTargets("//foo:foo_suite"));
+    assertThat(tester.getFilteredTargets()).containsExactlyElementsIn(getTargets());
+    assertThat(tester.getTestFilteredTargets())
+        .containsExactlyElementsIn(getTargets("//foo:foo_suite"));
   }
 
   /** Regression test for bug: "subtracting tests from test doesn't work" */
@@ -630,8 +636,8 @@ public class LoadingPhaseRunnerTest {
     private final StoredEventHandler storedErrors;
     private LoadingCallback loadingCallback;
 
-    private Set<Target> filteredTargets;
-    private Set<Target> testFilteredTargets;
+    private TargetParsingCompleteEvent targetParsingCompleteEvent;
+    private LoadingPhaseCompleteEvent loadingPhaseCompleteEvent;
 
     private MockToolsConfig mockToolsConfig;
 
@@ -709,8 +715,8 @@ public class LoadingPhaseRunnerTest {
         result = loadingPhaseRunner.execute(storedErrors, eventBus,
             ImmutableList.copyOf(patterns), options, ImmutableListMultimap.<String, Label>of(),
             keepGoing, /*enableLoading=*/true, determineTests, loadingCallback);
-        this.filteredTargets = listener.filteredTargets;
-        this.testFilteredTargets = listener.testFilteredTargets;
+        this.targetParsingCompleteEvent = listener.targetParsingCompleteEvent;
+        this.loadingPhaseCompleteEvent = listener.loadingPhaseCompleteEvent;
       } catch (LoadingFailedException e) {
         System.err.println(storedErrors.getEvents());
         throw e;
@@ -780,6 +786,22 @@ public class LoadingPhaseRunnerTest {
       return skyframeExecutor.getPackageManager();
     }
 
+    public ImmutableSet<Target> getFilteredTargets() {
+      return targetParsingCompleteEvent.getFilteredTargets();
+    }
+
+    public ImmutableSet<Target> getTestFilteredTargets() {
+      return targetParsingCompleteEvent.getTestFilteredTargets();
+    }
+
+    public ImmutableSet<Target> getOriginalTargets() {
+      return targetParsingCompleteEvent.getTargets();
+    }
+
+    public ImmutableSet<Target> getTestSuiteTargets() {
+      return loadingPhaseCompleteEvent.getFilteredTargets();
+    }
+
     private Iterable<Event> filteredEvents() {
       return Iterables.filter(storedErrors.getEvents(), new Predicate<Event>() {
         @Override
@@ -808,12 +830,17 @@ public class LoadingPhaseRunnerTest {
   }
 
   public static class FilteredTargetListener {
-    private Set<Target> filteredTargets;
-    private Set<Target> testFilteredTargets;
+    private TargetParsingCompleteEvent targetParsingCompleteEvent;
+    private LoadingPhaseCompleteEvent loadingPhaseCompleteEvent;
+
+    @Subscribe
+    public void targetParsingComplete(TargetParsingCompleteEvent event) {
+      this.targetParsingCompleteEvent = event;
+    }
+
     @Subscribe
-    public void notifyFilteredTargets(TargetParsingCompleteEvent event) {
-      filteredTargets = event.getFilteredTargets();
-      testFilteredTargets = event.getTestFilteredTargets();
+    public void loadingPhaseComplete(LoadingPhaseCompleteEvent event) {
+      this.loadingPhaseCompleteEvent = event;
     }
   }
 }