test_runner: junit fixes

lib/internal/test_runner/reporter/junit.js

@@ -16,21 +16,33 @@ const {
 } = primordials;
 
 const { inspectWithNoCustomRetry } = require('internal/errors');
+const { stripVTControlCharacters } = require('internal/util/inspect');
 const { hostname } = require('os');
 
 const inspectOptions = { __proto__: null, colors: false, breakLength: Infinity };
 const HOSTNAME = hostname();
 
+// Characters that are not allowed anywhere in an XML 1.0 document, not even
+// as character references: C0 controls other than tab, LF and CR, lone
+// surrogates, and U+FFFE/U+FFFF.
+const kInvalidXMLChars = /[^\t\n\r\x20-\uD7FF\uE000-\uFFFD\u{10000}-\u{10FFFF}]/gu;
+
+function sanitize(s = '') {
+  // Drop ANSI escape sequences entirely (e.g. colored assertion diffs), then
+  // replace any remaining illegal characters with U+FFFD.
+  return RegExpPrototypeSymbolReplace(kInvalidXMLChars, stripVTControlCharacters(s), '\uFFFD');
+}
+
 function escapeAttribute(s = '') {
   return RegExpPrototypeSymbolReplace(/"/g, RegExpPrototypeSymbolReplace(/\n/g, escapeContent(s), '
'), '"');
 }
 
 function escapeContent(s = '') {
-  return RegExpPrototypeSymbolReplace(/</g, RegExpPrototypeSymbolReplace(/(&)(?!#\d{1,7};)/g, s, '&amp;'), '&lt;');
+  return RegExpPrototypeSymbolReplace(/</g, RegExpPrototypeSymbolReplace(/&/g, sanitize(s), '&amp;'), '&lt;');
 }
 
 function escapeComment(s = '') {
-  return RegExpPrototypeSymbolReplace(/--/g, s, '&#45;&#45;');
+  return RegExpPrototypeSymbolReplace(/--/g, sanitize(s), '&#45;&#45;');
 }
 
 function treeToXML(tree) {
@@ -64,13 +76,52 @@ function isSkipped(node) {
   return (node?.children && ArrayPrototypeSome(node.children, (c) => c.tag === 'skipped')) || node?.attrs?.skipped;
 }
 
+function setSuiteAttrs(node, tests) {
+  node.tag = 'testsuite';
+  node.attrs.disabled = 0;
+  node.attrs.errors = 0;
+  node.attrs.tests = tests;
+  node.attrs.failures = ArrayPrototypeFilter(node.children, isFailure).length;
+  node.attrs.skipped = ArrayPrototypeFilter(node.children, isSkipped).length;
+}
+
+// Give a test that started but never reported a result (e.g. because a test
+// child process exited before all of its output was read) a valid element.
+// A leaf is reported as a failure so that lost results don't look green.
+function closeIncomplete(node) {
+  const tests = ArrayPrototypeFilter(node.children, (c) => c.comment == null).length;
+  if (tests > 0) {
+    setSuiteAttrs(node, tests);
+    node.attrs.hostname = HOSTNAME;
+    return;
+  }
+  const message = 'test did not report a result';
+  node.tag = 'testcase';
+  node.attrs.classname = 'test';
+  node.attrs.failure = message;
+  ArrayPrototypePush(node.children, {
+    __proto__: null, nesting: node.nesting + 1, tag: 'failure',
+    attrs: { __proto__: null, type: 'incomplete', message },
+  });
+}
+
 module.exports = async function* junitReporter(source) {
   yield '<?xml version="1.0" encoding="utf-8"?>\n';
   yield '<testsuites>\n';
   let currentSuite = null;
   const roots = [];
 
+  // Close any unfinished tests nested at `nesting` or deeper, so that later
+  // results are not attached to a test whose own result never arrived.
+  function closeUnfinished(nesting) {
+    while (currentSuite != null && currentSuite.nesting >= nesting) {
+      closeIncomplete(currentSuite);
+      currentSuite = currentSuite.parent;
+    }
+  }
+
   function startTest(event) {
+    closeUnfinished(event.data.nesting);
     const originalSuite = currentSuite;
     currentSuite = {
       __proto__: null,
@@ -95,11 +146,11 @@ module.exports = async function* junitReporter(source) {
       }
       case 'test:pass':
       case 'test:fail': {
-        if (!currentSuite) {
-          startTest({ __proto__: null, data: { __proto__: null, name: 'root', nesting: 0 } });
-        }
-        if (currentSuite.attrs.name !== event.data.name ||
+        closeUnfinished(event.data.nesting + 1);
+        if (currentSuite == null ||
+          currentSuite.attrs.name !== event.data.name ||
           currentSuite.nesting !== event.data.nesting) {
+          // No matching test:start was seen; start the test now.
           startTest(event);
         }
         const currentTest = currentSuite;
@@ -109,12 +160,7 @@ module.exports = async function* junitReporter(source) {
         currentTest.attrs.time = NumberPrototypeToFixed(event.data.details.duration_ms / 1000, 6);
         const nonCommentChildren = ArrayPrototypeFilter(currentTest.children, (c) => c.comment == null);
         if (nonCommentChildren.length > 0) {
-          currentTest.tag = 'testsuite';
-          currentTest.attrs.disabled = 0;
-          currentTest.attrs.errors = 0;
-          currentTest.attrs.tests = nonCommentChildren.length;
-          currentTest.attrs.failures = ArrayPrototypeFilter(currentTest.children, isFailure).length;
-          currentTest.attrs.skipped = ArrayPrototypeFilter(currentTest.children, isSkipped).length;
+          setSuiteAttrs(currentTest, nonCommentChildren.length);
           // A suite's `test:start` is emitted lazily (when its first subtest
           // reports), so derive the start time from the end minus the measured
           // duration rather than stamping the (late) test:start moment.
@@ -165,6 +211,7 @@ module.exports = async function* junitReporter(source) {
         break;
     }
   }
+  closeUnfinished(0);
   for (const suite of roots) {
     yield treeToXML(suite);
   }

lib/internal/test_runner/test.js

@@ -1525,6 +1525,8 @@ class Test extends AsyncResource {
           destination.on('unpipe', () => {
             if (!destination.closed && typeof destination.close === 'function') {
               destination.close(resolve);
+            } else if (destination.writableLength > 0) {
+              destination.write('', resolve);
             } else {
               resolve();
             }

test/fixtures/test-runner/junit-colored-diff.js

@@ -0,0 +1,5 @@
+'use strict';
+const assert = require('node:assert');
+const { test } = require('node:test');
+
+test('diff', () => assert.deepStrictEqual({ a: 1 }, { a: 2 }));

test/parallel/test-runner-force-exit-stdout-drain.js

@@ -0,0 +1,51 @@
+'use strict';
+// With --test-force-exit, buffered reporter output on stdout (both in test
+// child processes and in the main runner) must be flushed before exiting.
+// Refs: https://github.com/nodejs/node/issues/67149
+require('../common');
+const tmpdir = require('../common/tmpdir');
+const assert = require('node:assert');
+const { spawn } = require('node:child_process');
+const { writeFileSync } = require('node:fs');
+const { once } = require('node:events');
+const { setTimeout } = require('node:timers/promises');
+const { test } = require('node:test');
+
+const kFiles = 8;
+const kTestsPerFile = 300;
+
+tmpdir.refresh();
+const files = [];
+for (let i = 0; i < kFiles; i++) {
+  const file = tmpdir.resolve(`t${i}.test.js`);
+  writeFileSync(file, `'use strict';
+const { test } = require('node:test');
+for (let i = 0; i < ${kTestsPerFile}; i++) {
+  test('test ' + i + ' ' + 'x'.repeat(1000), () => {});
+}
+`);
+  files.push(file);
+}
+
+test('no results are lost when stdout is read slowly', async () => {
+  const child = spawn(process.execPath, [
+    '--test',
+    '--test-force-exit',
+    `--test-concurrency=${kFiles}`,
+    '--test-reporter=junit',
+    ...files,
+  ], { cwd: tmpdir.path });
+
+  // Let the pipes back up before reading anything.
+  child.stdout.pause();
+  await setTimeout(1000);
+  let stdout = '';
+  child.stdout.setEncoding('utf8');
+  child.stdout.on('data', (chunk) => { stdout += chunk; });
+  child.stdout.resume();
+
+  const [code] = await once(child, 'close');
+  assert.strictEqual(code, 0);
+  assert.strictEqual(stdout.match(/<testcase /g)?.length, kFiles * kTestsPerFile);
+  assert.match(stdout, /<\/testsuites>\n$/);
+});

test/parallel/test-runner-junit-invalid-xml.js

@@ -0,0 +1,96 @@
+'use strict';
+// Regression tests for the junit reporter emitting malformed XML.
+require('../common');
+const fixtures = require('../common/fixtures');
+const assert = require('node:assert');
+const { spawnSync } = require('node:child_process');
+const { test } = require('node:test');
+const { junit } = require('node:test/reporters');
+
+// Characters that XML 1.0 forbids anywhere, even as character references.
+const kInvalidXMLChars = /[^\t\n\r\x20-퟿-�\u{10000}-\u{10FFFF}]/u;
+
+function assertWellFormedChars(xml) {
+  assert.doesNotMatch(xml, kInvalidXMLChars);
+  for (const { 1: dec, 2: hex } of xml.matchAll(/&#(?:(\d+)|x([0-9a-fA-F]+));/g)) {
+    const code = dec !== undefined ? Number(dec) : Number.parseInt(hex, 16);
+    assert.doesNotMatch(String.fromCodePoint(code), kInvalidXMLChars,
+                        `illegal character reference &#${dec ?? `x${hex}`};`);
+  }
+}
+
+async function render(events) {
+  let out = '';
+  for await (const chunk of junit((async function*() { yield* events; })())) {
+    out += chunk;
+  }
+  return out;
+}
+
+const details = { duration_ms: 1 };
+
+test('control characters are not written to the report', async () => {
+  const name = 'name\x00with\x1b[31mcontrols\x1b[39m\x07￾\uD800';
+  const error = new Error('\x1b[32mexpected\x1b[39m\x0b\x0c\x00');
+  const xml = await render([
+    { type: 'test:start', data: { name, nesting: 0 } },
+    { type: 'test:fail', data: { name, nesting: 0, details: { ...details, error } } },
+    { type: 'test:diagnostic', data: { nesting: 0, message: 'diag\x00\x1b[1m' } },
+  ]);
+  assertWellFormedChars(xml);
+  // ANSI escape sequences are stripped, other illegal characters are replaced.
+  assert.match(xml, /name="name�withcontrols���"/);
+  assert.match(xml, /message="expected���"/);
+  assert.match(xml, /<!-- diag� -->/);
+});
+
+test('colored assertion diffs produce a valid report', () => {
+  const child = spawnSync(process.execPath, [
+    '--test-reporter=junit',
+    fixtures.path('test-runner', 'junit-colored-diff.js'),
+  ], { env: { ...process.env, FORCE_COLOR: '1' } });
+  const xml = child.stdout.toString();
+  assert.strictEqual(child.status, 1, child.stderr.toString());
+  assert.match(xml, /<failure /);
+  assertWellFormedChars(xml);
+});
+
+test('a test that never reports a result does not swallow later tests', async () => {
+  const xml = await render([
+    { type: 'test:start', data: { name: 'suite', nesting: 0 } },
+    { type: 'test:start', data: { name: 'first', nesting: 1 } },
+    { type: 'test:pass', data: { name: 'first', nesting: 1, details } },
+    { type: 'test:start', data: { name: 'next file', nesting: 0 } },
+    { type: 'test:pass', data: { name: 'next file', nesting: 0, details } },
+  ]);
+  assert.doesNotMatch(xml, /undefined/);
+  assert.strictEqual(xml, [
+    '<?xml version="1.0" encoding="utf-8"?>',
+    '<testsuites>',
+    `\t<testsuite name="suite" disabled="0" errors="0" tests="1" failures="0" skipped="0" hostname="${require('os').hostname()}">`,
+    '\t\t<testcase name="first" time="0.001000" classname="test"/>',
+    '\t</testsuite>',
+    '\t<testcase name="next file" time="0.001000" classname="test"/>',
+    '</testsuites>',
+    '',
+  ].join('\n'));
+});
+
+test('an unfinished leaf test is reported as a failure', async () => {
+  const xml = await render([
+    { type: 'test:start', data: { name: 'lost', nesting: 0 } },
+    { type: 'test:start', data: { name: 'ok', nesting: 0 } },
+    { type: 'test:pass', data: { name: 'ok', nesting: 0, details } },
+  ]);
+  assert.match(xml, /<testcase name="lost" classname="test" failure="test did not report a result">\n\t\t<failure type="incomplete" message="test did not report a result"\/>\n\t<\/testcase>/);
+  assert.match(xml, /<testcase name="ok" time="0\.001000" classname="test"\/>/);
+});
+
+test('results without a test:start are not wrapped in an undefined element', async () => {
+  const xml = await render([
+    { type: 'test:pass', data: { name: 'a', nesting: 0, details } },
+    { type: 'test:pass', data: { name: 'b', nesting: 0, details } },
+  ]);
+  assert.doesNotMatch(xml, /undefined|name="root"/);
+  assert.match(xml, /^\t<testcase name="a" [^\n]*\/>\n\t<testcase name="b" [^\n]*\/>$/m);
+});