Review requested:

• @nodejs/test_runner

If you push the artificial error to your branch, we could try running a node-stress-single-test CI with a single run to validate how Jenkins would parse the test failure.

@aduh95 pushed

CI: https://ci.nodejs.org/job/node-stress-single-test/479/

Unfortunately, the Jenkins output is still not great and doesn't show the actual failure: https://ci.nodejs.org/job/node-stress-single-test/479/nodes=rhel8-x64/testReport/junit/(root)/parallel/test_runner_cli_concurrency/

EDIT: see comments below, that's not actually related to node:test, wrong assumption on my end.

Interesting. Do you know how that is rendered.

I was targeting the output in https://ci.nodejs.org/job/node-stress-single-test/479/nodes=rhel8-x64/console, which does look much better. But it would be nice to fix https://ci.nodejs.org/job/node-stress-single-test/479/nodes=rhel8-x64/testReport/junit/(root)/parallel/test_runner_cli_concurrency/.

It looks like Jenkins cuts the "stacktrace" when it encounters stack: |- – maybe because it thinks it's a substack? I honestly don't know, I can try to look around if they have any documentation around that behavior.
I think we're using JUnit plugin, but it's unclear how it actually parses the test output.

Is this just a general problem not related to the use of node:test? Looking at https://ci.nodejs.org/job/node-test-commit-linux-containered/42211/#showFailuresLink, I see 9 failures, and I don't think any of those tests use node:test. All of them look like this:

fail (1)

---
duration_ms: 815.156
exitcode: 1
severity: fail
...

EDIT: This seems to be related to tap2junit I think.

Unfortunately, the Jenkins output is still not great and doesn't show the actual failure: https://ci.nodejs.org/job/node-stress-single-test/479/nodes=rhel8-x64/testReport/junit/(root)/parallel/test_runner_cli_concurrency/

I think that's a tap2junit issue? Those Jenkins pages used to have the failure output in the "Stacktrace" box.

Now that nodejs/tap2junit#54 has landed, what are the next steps to seeing it working in Jenkins?

it needs to be released and the updated in all the ci machines CC @nodejs/build @cclauss

@cjihrig nodejs/build#3761 is now deployed. should this PR be closed?

Unsure. That PR was blocking me from evaluating this PR in the CI to see if it could be beneficial.

So this still doesn't seem to work with the view in https://ci.nodejs.org/job/node-stress-single-test/514/nodes=rhel8-x64/testReport/junit/(root)/parallel/test_runner_cli_concurrency/, and I don't know what needs to happen to appease tap2junit (both with and without the change in this PR).

The console output (shown in the OP) is still significantly better IMO.

@MoLow I'm interested in replacing the spec reporter with the less verbose one from this PR inside of GitHub Actions runs. However, it looks like the GitHub reporter is also sending output to stdout:

::debug::starting to run default concurrency
::debug::completed running default concurrency
::debug::starting to run concurrency of one
::debug::completed running concurrency of one
::debug::starting to run concurrency of two
::error title=concurrency of two,file=test/parallel/test-runner-cli-concurrency.js,line=26,col=9::[Error [ERR_TEST_FAILURE]: bye] {
  code: 'ERR_TEST_FAILURE',
  failureType: 'testCodeFailure',
  cause: Error: bye
      at TestContext.<anonymous> (/home/runner/work/node/node/test/parallel/test-runner-cli-concurrency.js:26:9)
      at Test.runInAsyncScope (node:async_hooks:211:14)
      at Test.run (node:internal/test_runner/test:931:25)
      at Test.processPendingSubtests (node:internal/test_runner/test:629:18)
      at Test.postRun (node:internal/test_runner/test:1042:19)
      at Test.run (node:internal/test_runner/test:970:12)
      at async Test.processPendingSubtests (node:internal/test_runner/test:629:7)
}

Is it possible to leverage the GitHub reporter for its annotations without it also logging things?

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.54%. Comparing base (0d00511) to head (5dc2c0a).
Report is 118 commits behind head on main.

@@            Coverage Diff             @@
##             main   #52189      +/-   ##
==========================================
+ Coverage   88.04%   88.54%   +0.50%     
==========================================
  Files         657      657              
  Lines      190290   190290              
  Branches    36245    36540     +295     
==========================================
+ Hits       167536   168488     +952     
+ Misses      15893    14971     -922     
+ Partials     6861     6831      -30     

see 89 files with indirect coverage changes

Sigh. It looks like the GitHub Actions toolkit writes directly to process.stdout, and I don't think there is a way to avoid that. actions/toolkit#1743 appears to be trying to do exactly what I'd like to do.

@JakobJingleheimer this might be something of interest to you.

Awesome, this is a great idea! Do you need/want help finishing it?

Help would definitely be welcomed. There are a few things left:

1. Figure out exactly what output everyone wants. I think this might be close since it only outputs failures with the test title and location.
2. Figure out if we want to drop the GitHub reporter. As I mentioned in test: add error only reporter for node:test #52189 (comment), it seems to create a lot of noise on stdout, which would go against the idea of only showing failure output. My personal opinion is that we can remove it even though the annotations are kind of neat.
3. It might be worth adding a fail fast feature to this reporter (which basically just exits the process when a test:fail event is encountered). Maybe this could be configured via an environment variable or something so that people who want that behavior can opt into it.

Help would definitely be welcomed. There are a few things left:

I'd like to help with this

1. Figure out exactly what output everyone wants. I think this might be close since it only outputs failures with the test title and location.
2. Figure out if we want to drop the GitHub reporter. As I mentioned in [test: add error only reporter for node:test test: add error only reporter for node:test #52189 (comment)]
   (test: add error only reporter for node:test #52189 (comment)), it seems to create a lot of noise on stdout, which would go against the idea of only showing failure output. My personal opinion is that we can remove it even though the annotations are kind of neat.

About the first two I'm not sure what we can do to get the consensus

3. It might be worth adding a fail fast feature to this reporter (which basically just exits the process when a test:fail event is encountered). Maybe this could be configured via an environment variable or something so that people who want that behavior can opt into it.

About this one I found an issue that suggests some collaborators don't think the fail fast option should be included, so not sure if we want to do this.

Lmk however I can help.

cc @cjihrig @JakobJingleheimer

About the first two I'm not sure what we can do to get the consensus

I would just open a PR with the proposed changes (or push the changes to this PR) and see how it is received. People have actively requested this.

About this one I found an #42990 that suggests some collaborators don't think the fail fast option should be included, so not sure if we want to do this.

That discussion is about including a fail fast mode in the test runner itself. While that is something I am supportive of adding, that is not really relevant here. This is more about combining this userland reporter with the reporter in this PR, but making it configurable via something like an environment variable.

About the first two I'm not sure what we can do to get the consensus

I would just open a PR with the proposed changes (or push the changes to this PR) and see how it is received. People have actively requested this.

About this one I found an #42990 that suggests some collaborators don't think the fail fast option should be included, so not sure if we want to do this.

That discussion is about including a fail fast mode in the test runner itself. While that is something I am supportive of adding, that is not really relevant here. This is more about combining this userland reporter with the reporter in this PR, but making it configurable via something like an environment variable.

Cool, I'll make that. I guess I can add

if (process.env.FAIL_FAST) {
  yield `\n\u001b[31m✖ Bailing on failed test: ${event.data.name}\u001b[0m\n`;
  throw new Error('Bail');
}

Any better naming?

That name sounds fine to me. This is also something that is only available internally to Node core, so it can be iterated on a bit more freely.

Closing this since #56438 landed with these changes.