JUnit XML `failure` element and attribute are incorrect when using `node:assert`

[CuriousStork]

Version

v24.4.1

Platform

Microsoft Windows NT 10.0.19045.0 x64

Subsystem

test

What steps will reproduce the bug?

1. Create any test file, like

   import { equal } from "node:assert/strict";
   import { test } from "node:test";
   
   const value = 1;
   
   await test("testName", () => {
     equal(value, 0, `The value must be 0`);
   });

2. Run the test via console

   node --test --test-reporter=junit ".\junit.test.mts"

3. Check the report from stdout.

How often does it reproduce? Is there a required condition?

Always.

What is the expected behavior? Why is that the expected behavior?

• failure element should have a correct message attribute. I guess having "The value must be 0" in this example would be fine
• testcase element shouldn't have the failure attribute.

What do you see instead?

failure in testcase element and message in failure element are "The value must be 01 !== 0":

        <testcase name="testName" time="0.002234" classname="test" failure="The value must be 01 !== 0">
                <failure type="testCodeFailure" message="The value must be 01 !== 0">
Error [ERR_TEST_FAILURE]: The value must be 0

1 !== 0

    at async file:///C:/code/project/junit.test.mts:6:1 {
  code: 'ERR_TEST_FAILURE',
  failureType: 'testCodeFailure',
  cause: AssertionError [ERR_ASSERTION]: The value must be 0
  
  1 !== 0
  
      at TestContext.&lt;anonymous> (file:///C:/code/project/junit.test.mts:7:3)
      at Test.runInAsyncScope (node:async_hooks:214:14)
      at Test.run (node:internal/test_runner/test:1062:25)
      at Test.start (node:internal/test_runner/test:959:17)
      at startSubtestAfterBootstrap (node:internal/test_runner/harness:332:17)
      at async file:///C:/code/project/junit.test.mts:6:1 {
    generatedMessage: false,
    code: 'ERR_ASSERTION',
    actual: 1,
    expected: 0,
    operator: 'strictEqual'
  }
}
                </failure>
        </testcase>

Additional information

No response

[devholic22]

@CuriousStork Thanks for reporting this issue. Would you be open to me submitting a PR to help resolve it?

[CuriousStork]

@devholic22 Hello and thank you for taking a look. I would appreciate any help with this issue 👍

Obviously, I'm not a collaborator/maintainer here, so don't know exactly how to do it properly.

[devholic22]

@CuriousStork I created Pull Request for fix this issue.

In this PR, I’ve removed the failure attribute from the element.
As for the message field, I believe there are multiple possible approaches depending on the intended behavior, so I wanted to propose my suggestion before implementing it.

Here’s the idea:
• If the error message is user-defined (generatedMessage === false), the entire message should be preserved.
• If the message is automatically generated, only the first line (before a newline) should be shown. This avoids mixing user-defined content with system-generated diagnostics.

Originally, the code in lib/internal/test_runner/reporter/junit.js looks like this:

if (event.type === 'test:fail') {
  const error = event.data.details?.error;
  currentTest.children.push({
    __proto__: null,
    nesting: event.data.nesting + 1,
    tag: 'failure',
    attrs: {
      __proto__: null,
      type: error?.failureType || error?.code,
      message: error?.message ?? '',
    },
    children: [inspectWithNoCustomRetry(error, inspectOptions)],
  });
  currentTest.failures = 1;
}

My proposed change is as follows:

if (event.type === 'test:fail') {
  const error = event.data.details?.error;
  let summaryMessage = '';
  if (typeof error?.message === 'string') {
    if (error.generatedMessage === false) {
      summaryMessage = error.message;
    } else {
      summaryMessage = error.message.includes('\n')
        ? error.message.split('\n')[0]
        : error.message;
    }
  }
  currentTest.children.push({
    __proto__: null,
    nesting: event.data.nesting + 1,
    tag: 'failure',
    attrs: {
      __proto__: null,
      type: error?.failureType || error?.code,
      message: summaryMessage,
    },
    children: [inspectWithNoCustomRetry(error, inspectOptions)],
  });
  currentTest.failures = 1;
}

I would appreciate your feedback on whether this approach seems reasonable, or if there’s a better way to handle error message formatting.

[CuriousStork]

@devholic22 Thank you for the pull request.

I guess the cause of the issue is an incorrect attribute escaping. As I understand your idea is to use just the first line of the multiline text. I think we can use the whole multiline text, but fix its encoding/escaping for XML.

I'd suggest checking this piece of code:

node/lib/internal/test_runner/reporter/junit.js

Lines 21 to 23 in f36de72

	function escapeAttribute(s = '') {
	return escapeContent(RegExpPrototypeSymbolReplace(/"/g, RegExpPrototypeSymbolReplace(/\n/g, s, ''), '"'));
	}

This seems to replace \n with an empty string '' so far. I'd try to use the correct Line Feed code for XML attributes (
) and see if it works:

function escapeAttribute(s = '') {
  return escapeContent(RegExpPrototypeSymbolReplace(/"/g, RegExpPrototypeSymbolReplace(/\n/g, s, '
'), '"'));
}

Optimistically this should put the whole multiline string into the attribute instead of its first line. I guess this should work for any alternative assertion module (like chai etc.) and doesn't require checking generatedMessage.

I also noticed that node:assert produces error.message with a trailing \n. Probably removing it via trim() could improve the resulting XML:

message: error?.message.trim() ?? ''

[AlexCannonball]

Hello, is anyone working on failure:message attribute fix? I'd like to attempt fixing escapeAttribute function so that it escapes \n instead of erasing it.