From 568ec05a9723f435f2f16ea0473109f683c38cb4 Mon Sep 17 00:00:00 2001
From: Dmitry Gozman <dgozman@gmail.com>
Date: Wed, 22 Sep 2021 15:28:57 -0700
Subject: [PATCH] fix(junit): produce a single system-out entry (#9091)

Instead of multiple `system-out` entries we produce a single one
with concatenated content. This is compatible with various junit xml
parsers in the wild.
---
 src/test/reporters/junit.ts                  | 36 +++++++++-----------
 tests/playwright-test/junit-reporter.spec.ts | 22 +++++++++---
 2 files changed, 34 insertions(+), 24 deletions(-)

diff --git a/src/test/reporters/junit.ts b/src/test/reporters/junit.ts
index ef096a9fcb..6997091953 100644
--- a/src/test/reporters/junit.ts
+++ b/src/test/reporters/junit.ts
@@ -145,30 +145,28 @@ class JUnitReporter implements Reporter {
         text: stripAnsiEscapes(formatFailure(this.config, test))
       });
     }
-    for (const result of test.results) {
-      for (const stdout of result.stdout) {
-        entry.children.push({
-          name: 'system-out',
-          text: stdout.toString()
-        });
-      }
 
+    const systemOut: string[] = [];
+    const systemErr: string[] = [];
+    for (const result of test.results) {
+      systemOut.push(...result.stdout.map(item => item.toString()));
+      systemErr.push(...result.stderr.map(item => item.toString()));
       for (const attachment of result.attachments) {
-        if (attachment.path) {
-          entry.children.push({
-            name: 'system-out',
-            text: `[[ATTACHMENT|${path.relative(this.config.rootDir, attachment.path)}]]`
-          });
+        if (!attachment.path)
+          continue;
+        try {
+          if (fs.existsSync(attachment.path))
+            systemOut.push(`\n[[ATTACHMENT|${path.relative(this.config.rootDir, attachment.path)}]]\n`);
+        } catch (e) {
         }
       }
-
-      for (const stderr of result.stderr) {
-        entry.children.push({
-          name: 'system-err',
-          text: stderr.toString()
-        });
-      }
     }
+    // Note: it is important to only produce a single system-out/system-err entry
+    // so that parsers in the wild understand it.
+    if (systemOut.length)
+      entry.children.push({ name: 'system-out', text: systemOut.join('') });
+    if (systemErr.length)
+      entry.children.push({ name: 'system-err', text: systemErr.join('') });
   }
 }
 
diff --git a/tests/playwright-test/junit-reporter.spec.ts b/tests/playwright-test/junit-reporter.spec.ts
index 22afed280f..05b443f79a 100644
--- a/tests/playwright-test/junit-reporter.spec.ts
+++ b/tests/playwright-test/junit-reporter.spec.ts
@@ -103,6 +103,8 @@ test('should render stdout', async ({ runInlineTest }) => {
       const { test } = pwt;
       test('one', async ({}) => {
         console.log(colors.yellow('Hello world'));
+        console.log('Hello again');
+        console.error('My error');
         test.expect("abc").toBe('abcd');
       });
     `,
@@ -110,9 +112,10 @@ test('should render stdout', async ({ runInlineTest }) => {
   const xml = parseXML(result.output);
   const testcase = xml['testsuites']['testsuite'][0]['testcase'][0];
   expect(testcase['system-out'].length).toBe(1);
-  expect(testcase['system-out'][0]).toContain('Hello world');
+  expect(testcase['system-out'][0]).toContain('[33mHello world[39m\nHello again');
   expect(testcase['system-out'][0]).not.toContain('u00');
-  expect(testcase['failure'][0]['_']).toContain(`>  9 |         test.expect("abc").toBe('abcd');`);
+  expect(testcase['system-err'][0]).toContain('My error');
+  expect(testcase['failure'][0]['_']).toContain(`> 11 |         test.expect("abc").toBe('abcd');`);
   expect(result.exitCode).toBe(1);
 });
 
@@ -221,20 +224,29 @@ test('should render projects', async ({ runInlineTest }) => {
   expect(result.exitCode).toBe(0);
 });
 
-test('should render attachments', async ({ runInlineTest }) => {
+test('should render existing attachments, but not missing ones', async ({ runInlineTest }) => {
   const result = await runInlineTest({
     'a.test.js': `
       const { test } = pwt;
       test.use({ screenshot: 'on' });
-      test('one', async ({ page }) => {
+      test('one', async ({ page }, testInfo) => {
         await page.setContent('hello');
+        const file = testInfo.outputPath('file.txt');
+        require('fs').writeFileSync(file, 'my file', 'utf8');
+        testInfo.attachments.push({ name: 'my-file', path: file, contentType: 'text/plain' });
+        testInfo.attachments.push({ name: 'my-file-missing', path: file + '-missing', contentType: 'text/plain' });
+        console.log('log here');
       });
     `,
   }, { reporter: 'junit' });
   const xml = parseXML(result.output);
   const testcase = xml['testsuites']['testsuite'][0]['testcase'][0];
   expect(testcase['system-out'].length).toBe(1);
-  expect(testcase['system-out'][0].trim()).toBe(`[[ATTACHMENT|test-results${path.sep}a-one${path.sep}test-finished-1.png]]`);
+  expect(testcase['system-out'][0].trim()).toBe([
+    `log here`,
+    `\n[[ATTACHMENT|test-results${path.sep}a-one${path.sep}file.txt]]`,
+    `\n[[ATTACHMENT|test-results${path.sep}a-one${path.sep}test-finished-1.png]]`,
+  ].join('\n'));
   expect(result.exitCode).toBe(0);
 });