BEP: Extend infrastructure to allow reporting artifacts only once

Extend the functionality of the BuildEventStreamer to report those parts
of NestedSets of Artifacts not reported earlier. In this way, duplicate
reporting can be avoided, without the events themselves having to know
which artifacts are known already.

Change-Id: Ia959c28c440301860eac57ea5d9a712c0d49ebdf
PiperOrigin-RevId: 152497672

5 files changed, 152 insertions, 35 deletions

diff --git a/src/test/java/com/google/devtools/build/lib/buildeventstream/transports/BinaryFormatFileTransportTest.java b/src/test/java/com/google/devtools/build/lib/buildeventstream/transports/BinaryFormatFileTransportTest.java
index 1697bb9d1f..8f99efca83 100644
--- a/src/test/java/com/google/devtools/build/lib/buildeventstream/transports/BinaryFormatFileTransportTest.java
+++ b/src/test/java/com/google/devtools/build/lib/buildeventstream/transports/BinaryFormatFileTransportTest.java
@@ -19,6 +19,7 @@ import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertTrue;
 import static org.mockito.Mockito.when;
 
+import com.google.devtools.build.lib.buildeventstream.ArtifactGroupNamer;
 import com.google.devtools.build.lib.buildeventstream.BuildEvent;
 import com.google.devtools.build.lib.buildeventstream.BuildEventConverters;
 import com.google.devtools.build.lib.buildeventstream.BuildEventStreamProtos;
@@ -51,6 +52,7 @@ public class BinaryFormatFileTransportTest {
   @Mock public BuildEvent buildEvent;
 
   @Mock public PathConverter pathConverter;
+  @Mock public ArtifactGroupNamer artifactGroupNamer;
 
   @Before
   public void initMocks() {
@@ -73,19 +75,19 @@ public class BinaryFormatFileTransportTest {
     when(buildEvent.asStreamProto(Matchers.<BuildEventConverters>any())).thenReturn(started);
     BinaryFormatFileTransport transport =
         new BinaryFormatFileTransport(output.getAbsolutePath(), pathConverter);
-    transport.sendBuildEvent(buildEvent);
+    transport.sendBuildEvent(buildEvent, artifactGroupNamer);
 
     BuildEventStreamProtos.BuildEvent progress =
         BuildEventStreamProtos.BuildEvent.newBuilder().setProgress(Progress.newBuilder()).build();
     when(buildEvent.asStreamProto(Matchers.<BuildEventConverters>any())).thenReturn(progress);
-    transport.sendBuildEvent(buildEvent);
+    transport.sendBuildEvent(buildEvent, artifactGroupNamer);
 
     BuildEventStreamProtos.BuildEvent completed =
         BuildEventStreamProtos.BuildEvent.newBuilder()
             .setCompleted(TargetComplete.newBuilder().setSuccess(true))
             .build();
     when(buildEvent.asStreamProto(Matchers.<BuildEventConverters>any())).thenReturn(completed);
-    transport.sendBuildEvent(buildEvent);
+    transport.sendBuildEvent(buildEvent, artifactGroupNamer);
 
     transport.close().get();
     try (InputStream in = new FileInputStream(output)) {
@@ -109,7 +111,7 @@ public class BinaryFormatFileTransportTest {
             .build();
     when(buildEvent.asStreamProto(Matchers.<BuildEventConverters>any())).thenReturn(started);
     BinaryFormatFileTransport transport = new BinaryFormatFileTransport(path, pathConverter);
-    transport.sendBuildEvent(buildEvent);
+    transport.sendBuildEvent(buildEvent, artifactGroupNamer);
 
     transport.close().get();
     try (InputStream in = new FileInputStream(output)) {
@@ -136,7 +138,7 @@ public class BinaryFormatFileTransportTest {
     assertFalse(transport.ch.isOpen());
 
     // This should not throw an exception.
-    transport.sendBuildEvent(buildEvent);
+    transport.sendBuildEvent(buildEvent, artifactGroupNamer);
     transport.close().get();
 
     // Also, nothing should have been written to the file
@@ -158,10 +160,10 @@ public class BinaryFormatFileTransportTest {
     BinaryFormatFileTransport transport =
         new BinaryFormatFileTransport(output.getAbsolutePath(), pathConverter);
 
-    transport.sendBuildEvent(buildEvent);
+    transport.sendBuildEvent(buildEvent, artifactGroupNamer);
     Future<Void> closeFuture = transport.close();
     // This should not throw an exception, but also not perform any write.
-    transport.sendBuildEvent(buildEvent);
+    transport.sendBuildEvent(buildEvent, artifactGroupNamer);
 
     closeFuture.get();
     assertFalse(transport.ch.isOpen());
diff --git a/src/test/java/com/google/devtools/build/lib/buildeventstream/transports/BuildEventTransportFactoryTest.java b/src/test/java/com/google/devtools/build/lib/buildeventstream/transports/BuildEventTransportFactoryTest.java
index 31591c279b..c6762a8b5f 100644
--- a/src/test/java/com/google/devtools/build/lib/buildeventstream/transports/BuildEventTransportFactoryTest.java
+++ b/src/test/java/com/google/devtools/build/lib/buildeventstream/transports/BuildEventTransportFactoryTest.java
@@ -20,6 +20,7 @@ import static org.mockito.Mockito.when;
 import com.google.common.base.Function;
 import com.google.common.collect.FluentIterable;
 import com.google.common.collect.ImmutableSet;
+import com.google.devtools.build.lib.buildeventstream.ArtifactGroupNamer;
 import com.google.devtools.build.lib.buildeventstream.BuildEvent;
 import com.google.devtools.build.lib.buildeventstream.BuildEventConverters;
 import com.google.devtools.build.lib.buildeventstream.BuildEventStreamProtos;
@@ -64,6 +65,7 @@ public class BuildEventTransportFactoryTest {
   @Mock public BuildEvent buildEvent;
 
   @Mock public PathConverter pathConverter;
+  @Mock public ArtifactGroupNamer artifactGroupNamer;
 
   @Before
   public void before() {
@@ -129,7 +131,7 @@ public class BuildEventTransportFactoryTest {
   private void sendEventsAndClose(BuildEvent event, Iterable<BuildEventTransport> transports)
       throws IOException{
     for (BuildEventTransport transport : transports) {
-      transport.sendBuildEvent(event);
+      transport.sendBuildEvent(event, artifactGroupNamer);
       transport.close();
     }
   }
diff --git a/src/test/java/com/google/devtools/build/lib/buildeventstream/transports/TextFormatFileTransportTest.java b/src/test/java/com/google/devtools/build/lib/buildeventstream/transports/TextFormatFileTransportTest.java
index 7a008fabb0..d2ae4a8f72 100644
--- a/src/test/java/com/google/devtools/build/lib/buildeventstream/transports/TextFormatFileTransportTest.java
+++ b/src/test/java/com/google/devtools/build/lib/buildeventstream/transports/TextFormatFileTransportTest.java
@@ -19,6 +19,7 @@ import static org.mockito.Mockito.when;
 
 import com.google.common.base.Joiner;
 import com.google.common.io.Files;
+import com.google.devtools.build.lib.buildeventstream.ArtifactGroupNamer;
 import com.google.devtools.build.lib.buildeventstream.BuildEvent;
 import com.google.devtools.build.lib.buildeventstream.BuildEventConverters;
 import com.google.devtools.build.lib.buildeventstream.BuildEventStreamProtos;
@@ -50,6 +51,7 @@ public class TextFormatFileTransportTest {
   @Mock public BuildEvent buildEvent;
 
   @Mock public PathConverter pathConverter;
+  @Mock public ArtifactGroupNamer artifactGroupNamer;
 
   @Before
   public void initMocks() {
@@ -72,19 +74,19 @@ public class TextFormatFileTransportTest {
     when(buildEvent.asStreamProto(Matchers.<BuildEventConverters>any())).thenReturn(started);
     TextFormatFileTransport transport =
         new TextFormatFileTransport(output.getAbsolutePath(), pathConverter);
-    transport.sendBuildEvent(buildEvent);
+    transport.sendBuildEvent(buildEvent, artifactGroupNamer);
 
     BuildEventStreamProtos.BuildEvent progress =
         BuildEventStreamProtos.BuildEvent.newBuilder().setProgress(Progress.newBuilder()).build();
     when(buildEvent.asStreamProto(Matchers.<BuildEventConverters>any())).thenReturn(progress);
-    transport.sendBuildEvent(buildEvent);
+    transport.sendBuildEvent(buildEvent, artifactGroupNamer);
 
     BuildEventStreamProtos.BuildEvent completed =
         BuildEventStreamProtos.BuildEvent.newBuilder()
             .setCompleted(TargetComplete.newBuilder().setSuccess(true))
             .build();
     when(buildEvent.asStreamProto(Matchers.<BuildEventConverters>any())).thenReturn(completed);
-    transport.sendBuildEvent(buildEvent);
+    transport.sendBuildEvent(buildEvent, artifactGroupNamer);
 
     transport.close().get();
     String contents =
diff --git a/src/test/java/com/google/devtools/build/lib/runtime/BuildEventStreamerTest.java b/src/test/java/com/google/devtools/build/lib/runtime/BuildEventStreamerTest.java
index 8c6483d66d..3e7fe5d2dd 100644
--- a/src/test/java/com/google/devtools/build/lib/runtime/BuildEventStreamerTest.java
+++ b/src/test/java/com/google/devtools/build/lib/runtime/BuildEventStreamerTest.java
@@ -19,16 +19,28 @@ import static org.junit.Assert.assertTrue;
 
 import com.google.common.collect.ImmutableSet;
 import com.google.common.util.concurrent.Futures;
+import com.google.devtools.build.lib.actions.Artifact;
+import com.google.devtools.build.lib.actions.EventReportingArtifacts;
+import com.google.devtools.build.lib.actions.Root;
+import com.google.devtools.build.lib.buildeventstream.ArtifactGroupNamer;
 import com.google.devtools.build.lib.buildeventstream.BuildEvent;
 import com.google.devtools.build.lib.buildeventstream.BuildEventConverters;
 import com.google.devtools.build.lib.buildeventstream.BuildEventId;
 import com.google.devtools.build.lib.buildeventstream.BuildEventStreamProtos;
+import com.google.devtools.build.lib.buildeventstream.BuildEventStreamProtos.BuildEventId.NamedSetOfFilesId;
 import com.google.devtools.build.lib.buildeventstream.BuildEventTransport;
 import com.google.devtools.build.lib.buildeventstream.BuildEventWithOrderConstraint;
 import com.google.devtools.build.lib.buildeventstream.GenericBuildEvent;
+import com.google.devtools.build.lib.buildeventstream.PathConverter;
 import com.google.devtools.build.lib.buildeventstream.ProgressEvent;
 import com.google.devtools.build.lib.buildtool.BuildResult;
 import com.google.devtools.build.lib.buildtool.buildevent.BuildCompleteEvent;
+import com.google.devtools.build.lib.collect.nestedset.NestedSet;
+import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
+import com.google.devtools.build.lib.collect.nestedset.NestedSetView;
+import com.google.devtools.build.lib.testutil.FoundationTestCase;
+import com.google.devtools.build.lib.vfs.Path;
+import com.google.devtools.build.lib.vfs.PathFragment;
 import java.util.ArrayList;
 import java.util.Collection;
 import java.util.List;
@@ -39,18 +51,33 @@ import org.junit.runners.JUnit4;
 
 /** Tests {@link BuildEventStreamer}. */
 @RunWith(JUnit4.class)
-public class BuildEventStreamerTest {
+public class BuildEventStreamerTest extends FoundationTestCase {
 
   private static class RecordingBuildEventTransport implements BuildEventTransport {
-    private final List<BuildEvent> events;
-
-    RecordingBuildEventTransport() {
-      events = new ArrayList<>();
-    }
+    private final List<BuildEvent> events = new ArrayList<>();
+    private final List<BuildEventStreamProtos.BuildEvent> eventsAsProtos = new ArrayList<>();
 
     @Override
-    public void sendBuildEvent(BuildEvent event) {
+    public void sendBuildEvent(BuildEvent event, final ArtifactGroupNamer namer) {
       events.add(event);
+      eventsAsProtos.add(
+          event.asStreamProto(
+              new BuildEventConverters() {
+                @Override
+                public ArtifactGroupNamer artifactGroupNamer() {
+                  return namer;
+                }
+
+                @Override
+                public PathConverter pathConverter() {
+                  return new PathConverter() {
+                    @Override
+                    public String apply(Path path) {
+                      return path.toString();
+                    }
+                  };
+                }
+              }));
     }
 
     @Override
@@ -61,6 +88,10 @@ public class BuildEventStreamerTest {
     List<BuildEvent> getEvents() {
       return events;
     }
+
+    List<BuildEventStreamProtos.BuildEvent> getEventProtos() {
+      return eventsAsProtos;
+    }
   }
 
   private static class GenericOrderEvent implements BuildEventWithOrderConstraint {
@@ -100,6 +131,53 @@ public class BuildEventStreamerTest {
     }
   }
 
+  private static class GenericArtifactReportingEvent implements EventReportingArtifacts {
+    private final BuildEventId id;
+    private final Collection<BuildEventId> children;
+    private final Collection<NestedSet<Artifact>> artifacts;
+
+    GenericArtifactReportingEvent(
+        BuildEventId id,
+        Collection<BuildEventId> children,
+        Collection<NestedSet<Artifact>> artifacts) {
+      this.id = id;
+      this.children = children;
+      this.artifacts = artifacts;
+    }
+
+    GenericArtifactReportingEvent(BuildEventId id, Collection<NestedSet<Artifact>> artifacts) {
+      this(id, ImmutableSet.<BuildEventId>of(), artifacts);
+    }
+
+    @Override
+    public BuildEventId getEventId() {
+      return id;
+    }
+
+    @Override
+    public Collection<BuildEventId> getChildrenEvents() {
+      return children;
+    }
+
+    @Override
+    public Collection<NestedSet<Artifact>> reportedArtifacts() {
+      return artifacts;
+    }
+
+    @Override
+    public BuildEventStreamProtos.BuildEvent asStreamProto(BuildEventConverters converters) {
+      BuildEventStreamProtos.NamedSetOfFiles.Builder builder =
+          BuildEventStreamProtos.NamedSetOfFiles.newBuilder();
+      for (NestedSet<Artifact> artifactset : artifacts) {
+        builder.addFileSets(
+            converters
+                .artifactGroupNamer()
+                .apply((new NestedSetView<Artifact>(artifactset)).identifier()));
+      }
+      return GenericBuildEvent.protoChaining(this).setNamedSetOfFiles(builder.build()).build();
+    }
+  }
+
   private static BuildEventId testId(String opaque) {
     return BuildEventId.unknownBuildEventId(opaque);
   }
@@ -326,4 +404,54 @@ public class BuildEventStreamerTest {
     assertEquals(startEvent.getEventId(), allEventsSeen.get(0).getEventId());
     assertEquals(waitingForStart.getEventId(), allEventsSeen.get(1).getEventId());
   }
+
+  private Artifact makeArtifact(String pathString) {
+    Path path = outputBase.getRelative(PathFragment.create(pathString));
+    return new Artifact(path, Root.asSourceRoot(path));
+  }
+
+  @Test
+  public void testReportedArtifacts() {
+    // Verify that reported artifacts are correctly unfolded into the stream
+    RecordingBuildEventTransport transport = new RecordingBuildEventTransport();
+    BuildEventStreamer streamer =
+        new BuildEventStreamer(ImmutableSet.<BuildEventTransport>of(transport));
+
+    BuildEvent startEvent =
+        new GenericBuildEvent(
+            testId("Initial"),
+            ImmutableSet.<BuildEventId>of(ProgressEvent.INITIAL_PROGRESS_UPDATE));
+
+    Artifact a = makeArtifact("path/a");
+    Artifact b = makeArtifact("path/b");
+    Artifact c = makeArtifact("path/c");
+    NestedSet<Artifact> innerGroup = NestedSetBuilder.<Artifact>stableOrder().add(a).add(b).build();
+    NestedSet<Artifact> group =
+        NestedSetBuilder.<Artifact>stableOrder().addTransitive(innerGroup).add(c).build();
+    BuildEvent reportingArtifacts =
+        new GenericArtifactReportingEvent(testId("reporting"), ImmutableSet.of(group));
+
+    streamer.buildEvent(startEvent);
+    streamer.buildEvent(reportingArtifacts);
+
+    List<BuildEvent> allEventsSeen = transport.getEvents();
+    List<BuildEventStreamProtos.BuildEvent> eventProtos = transport.getEventProtos();
+    assertEquals(7, allEventsSeen.size());
+    assertEquals(startEvent.getEventId(), allEventsSeen.get(0).getEventId());
+    assertEquals(ProgressEvent.INITIAL_PROGRESS_UPDATE, allEventsSeen.get(1).getEventId());
+    List<BuildEventStreamProtos.File> firstSetDirects =
+        eventProtos.get(2).getNamedSetOfFiles().getFilesList();
+    assertEquals(2, firstSetDirects.size());
+    assertEquals(
+        ImmutableSet.of(a.getPath().toString(), b.getPath().toString()),
+        ImmutableSet.of(firstSetDirects.get(0).getUri(), firstSetDirects.get(1).getUri()));
+    List<NamedSetOfFilesId> secondSetTransitives =
+        eventProtos.get(4).getNamedSetOfFiles().getFileSetsList();
+    assertEquals(1, secondSetTransitives.size());
+    assertEquals(eventProtos.get(2).getId().getNamedSet(), secondSetTransitives.get(0));
+    List<NamedSetOfFilesId> reportedArtifactSets =
+        eventProtos.get(6).getNamedSetOfFiles().getFileSetsList();
+    assertEquals(1, reportedArtifactSets.size());
+    assertEquals(eventProtos.get(4).getId().getNamedSet(), reportedArtifactSets.get(0));
+  }
 }
diff --git a/src/test/shell/integration/build_event_stream_test.sh b/src/test/shell/integration/build_event_stream_test.sh
index fbf83aa8ac..4d9480e9c1 100755
--- a/src/test/shell/integration/build_event_stream_test.sh
+++ b/src/test/shell/integration/build_event_stream_test.sh
@@ -118,7 +118,6 @@ function test_basic() {
   expect_log 'build_finished'
   expect_log 'overall_success: true'
   expect_log 'finish_time'
-  expect_log_once '^progress '
   expect_not_log 'aborted'
 }
 
@@ -138,7 +137,6 @@ function test_test_summary() {
   bazel test --experimental_build_event_text_file=$TEST_log pkg:true \
     || fail "bazel test failed"
   expect_log_once '^test_summary '
-  expect_log_once '^progress '
   expect_not_log 'aborted'
   expect_log 'status.*PASSED'
   expect_not_log 'status.*FAILED'
@@ -156,7 +154,6 @@ function test_test_inidivual_results() {
   expect_log 'run.*1'
   expect_log 'status.*PASSED'
   expect_log_once '^test_summary '
-  expect_log_once '^progress '
   expect_not_log 'aborted'
 }
 
@@ -175,7 +172,6 @@ function test_test_attempts() {
   expect_log 'status.*FAILED'
   expect_not_log 'status.*PASSED'
   expect_not_log 'status.*FLAKY'
-  expect_log_once '^progress '
   expect_not_log 'aborted'
   expect_log '^test_result'
   expect_log 'test_action_output'
@@ -221,7 +217,6 @@ function test_test_attempts_multi_runs() {
     && fail "test failure expected" ) || true
   expect_log 'run.*1'
   expect_log 'attempt.*2'
-  expect_log_once '^progress '
   expect_not_log 'aborted'
 }
 
@@ -234,7 +229,6 @@ function test_test_attempts_multi_runs_flake_detection() {
     && fail "test failure expected" ) || true
   expect_log 'run.*1'
   expect_log 'attempt.*2'
-  expect_log_once '^progress '
   expect_not_log 'aborted'
 }
 
@@ -247,7 +241,6 @@ function test_cached_test_results() {
   expect_log '^test_result'
   expect_log 'name:.*test.log'
   expect_log 'name:.*test.xml'
-  expect_log_once '^progress '
   expect_not_log 'cached_locally'
   expect_not_log 'aborted'
   bazel test --experimental_build_event_text_file=$TEST_log pkg:true \
@@ -256,7 +249,6 @@ function test_cached_test_results() {
   expect_log 'name:.*test.log'
   expect_log 'name:.*test.xml'
   expect_log 'cached_locally'
-  expect_log_once '^progress '
   expect_not_log 'aborted'
 }
 
@@ -300,21 +292,12 @@ function test_build_only() {
     || fail "bazel build failed"
   expect_not_log 'aborted'
   expect_not_log 'test_summary '
-  expect_log_once '^progress'
   # Build Finished
   expect_log 'build_finished'
   expect_log 'overall_success: true'
   expect_log 'finish_time'
 }
 
-function test_build_test_suite() {
-  # Sucessfully building a test suite should not have any unexpected events;
-  # so we expect to see only one progress event.
-  bazel build --experimental_build_event_text_file=$TEST_log pkg:suite \
-    || fail "bazel build failed"
-  expect_log_once '^progress'
-}
-
 function test_multiple_transports() {
   # Verifies usage of multiple build event transports at the same time
     outdir=$(mktemp -d ${TEST_TMPDIR}/bazel.XXXXXXXX)