Repository: bazelbuild/bazel
Issue: FilterTestHeaderOutputStream doesn't respect system line separator
Labels: area-Windows
untriaged
team-OSS

`private static final int NEWLINE = '\n';`

https://github.com/bazelbuild/bazel/blob/59f17d6e0550bf63a0b6ef182e2d63474e058ede/src/main/java/com/google/devtools/build/lib/exec/TestLogHelper.java#L95

cc @meteorcloudy

Issue comments:
See also https://github.com/bazelbuild/bazel/blob/master/src/test/java/com/google/devtools/build/lib/exec/StreamedTestOutputTest.java#L70

Pull request review comments:

Patch that resolved the issue:
diff --git a/src/main/java/com/google/devtools/build/lib/exec/TestLogHelper.java b/src/main/java/com/google/devtools/build/lib/exec/TestLogHelper.java
index 409c10d903a9ce..5023a26af9a3d4 100644
--- a/src/main/java/com/google/devtools/build/lib/exec/TestLogHelper.java
+++ b/src/main/java/com/google/devtools/build/lib/exec/TestLogHelper.java
@@ -15,6 +15,7 @@
 
 import com.google.common.io.ByteStreams;
 import com.google.devtools.build.lib.exec.TestStrategy.TestOutputFormat;
+import com.google.devtools.build.lib.util.OS;
 import com.google.devtools.build.lib.vfs.Path;
 import java.io.BufferedOutputStream;
 import java.io.FilterOutputStream;
@@ -105,7 +106,9 @@ public void write(int b) throws IOException {
       } else if (b == NEWLINE) {
         String line = lineBuilder.toString();
         lineBuilder = new StringBuilder();
-        if (line.equals(TestLogHelper.HEADER_DELIMITER)) {
+        if (line.equals(TestLogHelper.HEADER_DELIMITER) ||
+            // On Windows, the line break could be \r\n, we want this case to work as well.
+            (OS.getCurrent() == OS.WINDOWS && line.equals(TestLogHelper.HEADER_DELIMITER + "\r"))) {
           seenDelimiter = true;
         }
       } else if (lineBuilder.length() <= TestLogHelper.HEADER_DELIMITER.length()) {


Test patch:
diff --git a/src/test/java/com/google/devtools/build/lib/exec/StreamedTestOutputTest.java b/src/test/java/com/google/devtools/build/lib/exec/StreamedTestOutputTest.java
index cc04eadf9d1051..fb044675176cdd 100644
--- a/src/test/java/com/google/devtools/build/lib/exec/StreamedTestOutputTest.java
+++ b/src/test/java/com/google/devtools/build/lib/exec/StreamedTestOutputTest.java
@@ -66,11 +66,6 @@ public void testNoHeaderOutputsEntireFile() throws IOException {
 
   @Test
   public void testOnlyOutputsContentsAfterHeaderWhenPresent() throws IOException {
-    if (OS.getCurrent() == OS.WINDOWS) {
-      // TODO(b/151095783): Disabled because underlying code doesn't respect system line separator.
-      return;
-    }
-
     Path watchedPath = fileSystem.getPath("/myfile");
     FileSystemUtils.writeLinesAs(
         watchedPath,
@@ -86,7 +81,8 @@ public void testOnlyOutputsContentsAfterHeaderWhenPresent() throws IOException {
     try (StreamedTestOutput underTest =
         new StreamedTestOutput(OutErr.create(out, err), fileSystem.getPath("/myfile"))) {}
 
-    assertThat(out.toString(StandardCharsets.UTF_8.name())).isEqualTo("included\nlines\n");
+    assertThat(out.toString(StandardCharsets.UTF_8.name()))
+        .isEqualTo(String.format("included%nlines%n"));
     assertThat(err.toString(StandardCharsets.UTF_8.name())).isEmpty();
   }