Add test for SynchronizedOutputStream
Restoring test from https://github.com/bazelbuild/bazel/commit/886d7d590fae2f860788a4c8ccb4624e59fc212d, updated to deal in strings, and including a
disabled test demonstrating how binary data should work (it doesn't now).
PiperOrigin-RevId: 307671104
diff --git a/src/main/java/com/google/devtools/build/lib/runtime/SynchronizedOutputStream.java b/src/main/java/com/google/devtools/build/lib/runtime/SynchronizedOutputStream.java
index 2c4c124..ea0909b 100644
--- a/src/main/java/com/google/devtools/build/lib/runtime/SynchronizedOutputStream.java
+++ b/src/main/java/com/google/devtools/build/lib/runtime/SynchronizedOutputStream.java
@@ -19,6 +19,7 @@
import com.google.common.base.Preconditions;
import com.google.common.base.Splitter;
import com.google.common.collect.ImmutableList;
+import java.io.ByteArrayOutputStream;
import java.io.IOException;
import java.io.OutputStream;
@@ -46,7 +47,6 @@
private byte[] buf;
private long count;
- private boolean discardAll;
// The event streamer that is supposed to flush stdout/stderr.
private BuildEventStreamer streamer;
@@ -55,7 +55,6 @@
Preconditions.checkArgument(maxChunkSize > 0);
buf = new byte[64];
count = 0;
- discardAll = false;
this.maxBufferedLength = maxBufferedLength;
this.maxChunkSize = Math.max(maxChunkSize, maxBufferedLength);
}
@@ -79,9 +78,6 @@
@Override
public void write(int oneByte) throws IOException {
- if (discardAll) {
- return;
- }
// We change the dependency with respect to that of the super class: write(int)
// now calls write(int[], int, int) which is implemented without any dependencies.
write(new byte[] {(byte) oneByte}, 0, 1);
@@ -91,11 +87,6 @@
public void write(byte[] buffer, int offset, int count) throws IOException {
// As we base the less common write(int) on this method, we may not depend not call write(int)
// directly or indirectly (e.g., by calling super.write(int[], int, int)).
- synchronized (this) {
- if (discardAll) {
- return;
- }
- }
boolean shouldFlush = false;
// As we have to do the flushing outside the synchronized block, we have to expect
// other writes to come immediately after flushing, so we have to do the check inside
diff --git a/src/test/java/com/google/devtools/build/lib/runtime/SynchronizedOutputStreamTest.java b/src/test/java/com/google/devtools/build/lib/runtime/SynchronizedOutputStreamTest.java
new file mode 100644
index 0000000..2a27123
--- /dev/null
+++ b/src/test/java/com/google/devtools/build/lib/runtime/SynchronizedOutputStreamTest.java
@@ -0,0 +1,107 @@
+// Copyright 2020 The Bazel Authors. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.devtools.build.lib.runtime;
+
+import static com.google.common.truth.Truth.assertThat;
+import static org.mockito.Mockito.doAnswer;
+import static org.mockito.Mockito.mock;
+
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.Iterables;
+import java.io.IOException;
+import java.nio.charset.StandardCharsets;
+import java.util.ArrayList;
+import java.util.List;
+import org.junit.Ignore;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.JUnit4;
+
+/**
+ * Tests for {@link SynchronizedOutputStream}.
+ *
+ * <p>Note that, at the time of writing, these tests serve to document the actual behavior of the
+ * class, not necessarily the desired behavior.
+ */
+@RunWith(JUnit4.class)
+public class SynchronizedOutputStreamTest {
+
+ @Test
+ public void testReadAndResetReturnsChunkedWritesSinceLastCall() throws IOException {
+ SynchronizedOutputStream underTest =
+ new SynchronizedOutputStream(/*maxBufferedLength=*/ 5, /*maxChunkSize=*/ 5);
+
+ underTest.write(new byte[] {'a', 'b', 'c', 'd', 'e', 'f', 'g', 'h'});
+ assertThat(underTest.readAndReset()).containsExactly("abcde", "fgh").inOrder();
+
+ assertThat(underTest.readAndReset()).isEmpty();
+
+ underTest.write(new byte[] {'i', 'j', 'k'});
+ assertThat(underTest.readAndReset()).containsExactly("ijk");
+ }
+
+ @Test
+ public void testWriteFlushesStreamerWhenMaxBufferedLengthReached() throws IOException {
+ SynchronizedOutputStream underTest =
+ new SynchronizedOutputStream(/*maxBufferedLength=*/ 3, /*maxChunkSize=*/ 3);
+
+ List<List<String>> writes = new ArrayList<>();
+ BuildEventStreamer mockStreamer = mock(BuildEventStreamer.class);
+ doAnswer(
+ inv -> {
+ writes.add(ImmutableList.copyOf(underTest.readAndReset()));
+ return null;
+ })
+ .when(mockStreamer)
+ .flush();
+ underTest.registerStreamer(mockStreamer);
+
+ underTest.write(new byte[] {'a', 'b', 'c', 'd', 'e', 'f', 'g', 'h'});
+ underTest.write(new byte[] {'i', 'j'});
+ underTest.write(new byte[] {'k', 'l'});
+
+ assertThat(writes)
+ .containsExactly(
+ ImmutableList.of("abc", "def", "gh"),
+ // The write of {'k', 'j'} would have put the buffer over size, so {'i', 'j'} was
+ // flushed.
+ ImmutableList.of("ij"))
+ .inOrder();
+ }
+
+ @Test
+ public void testUsesMaxOfMaxBufferedSizeAndMaxChunkSizeForChunking() throws IOException {
+ SynchronizedOutputStream underTest =
+ new SynchronizedOutputStream(/*maxBufferedLength=*/ 2, /*maxChunkSize=*/ 1);
+
+ underTest.write(new byte[] {'a', 'b', 'c', 'd'});
+ assertThat(underTest.readAndReset()).containsExactly("ab", "cd").inOrder();
+ }
+
+ // TODO(b/154242266): Make BES handle binary data correctly.
+ @Ignore("b/154242266 - BES doesn't handle binary stdout/err correctly")
+ @Test
+ public void testHandlesArbitraryBinaryDataCorrectly() throws IOException {
+ SynchronizedOutputStream underTest =
+ new SynchronizedOutputStream(/*maxBufferedLength=*/ 1, /*maxChunkSize=*/ 1);
+
+ byte[] input = new byte[] {(byte) 0xff};
+ underTest.write(input);
+
+ String result = Iterables.getOnlyElement(underTest.readAndReset());
+ // In the real code the result eventually winds up in a protobuf, which treats strings as utf8.
+ assertThat(result.getBytes(StandardCharsets.UTF_8)).isEqualTo(input);
+ }
+}
