test_runner: display failed test stack trace with dot reporter by mihir254 · Pull Request #52655 · nodejs/node

mihir254 · 2024-04-23T17:23:18Z

Add failed test names and stack trace to dot reporter output

nodejs-github-bot · 2024-04-23T17:23:23Z

Review requested:

@nodejs/test_runner

avivkeller · 2024-04-23T18:23:22Z

  yield '\n';
+
+  if (failedTests.length > 0) {
+    yield `${colors.red}${failedTestSymbol} failing tests:${colors.white}\n`;


Use uppercase when possible

Failed tests:

Sure, I'll keep that in mind. I assumed it should be consistent with the spec reporter, which uses "failing tests:"

@nodejs/test_runner WDYT about the formatting?

Should I also make it Failed tests: for spec reporter?

can be in a separate PR

avivkeller · 2024-04-23T18:24:03Z

    }
    if (type === 'test:fail') {
      yield 'X';
+      failedTests.push(data);


For the internals of Node.js, we use primordials, so try to use them when possible (ArrayPrototypePush)

avivkeller · 2024-04-23T18:26:30Z

Also, please lint your source code and commit message.

(You just need to the put the change in the commit message, E.G. test: print failed tests with dot reporter, and the rest can go in the PR description)

atlowChemi

The commit should be prefixed with the node submodule changed, which in this case is test_runner and not test (test refers to the tests used in the Nodejs CI)

Please note that you will have to regenerate the snapshots for this to work. You will have to run the following command NODE_REGENERATE_SNAPSHOTS=1 ./node test/parallel/test-runner-output.mjs see:

node/test/common/README.md

Lines 690 to 694 in 1681786

    
           ### `NODE_REGENERATE_SNAPSHOTS` 
        
           If set, test snapshots for a the current test are regenerated. 
        
           for example `NODE_REGENERATE_SNAPSHOTS=1 out/Release/node test/parallel/test-runner-output.mjs` 
        
           will update all the test runner output snapshots.

atlowChemi · 2024-04-23T19:41:09Z

  yield '\n';
+
+  if (failedTests.length > 0) {
+    yield `${colors.red}${failedTestSymbol} failing tests:${colors.white}\n`;


We might want to use util.styleText, though I am not sure we want to, as it could be affected from user-land

since the issue mentioned spec reporter, I tried to match it to how it's done in spec.js. If it is required to be changed, should it only be done for dot reporter or for spec reporter as well?

The util.styleText was added after these reporters 🙂
Anyway, not sure we want to use it here. @MoLow @cjihrig WDYT?

It might be worth to change it globally, but I think that’s an issue for a different PR.

atlowChemi · 2024-04-23T19:42:34Z

+  if (failedTests.length > 0) {
+    yield `${colors.red}${failedTestSymbol} failing tests:${colors.white}\n`;
+    for (const test of failedTests) {
+      const message = `${colors.red}${failedTestSymbol} ${test.name} ${colors.gray}(${test.details.duration_ms}ms)\n`;


Don't we need to reset the output color after this?

mcollina

Can you please add a unit test?

MoLow · 2024-04-24T08:36:44Z

Can you please add a unit test?

there are existing tests in place that need to be updated. see #52655 (review)

avivkeller

A lot of the test snapshots should not have changed

mihir254 · 2024-04-24T20:59:03Z

▶ test runner output (4860.751ms)
ℹ tests 47
ℹ suites 1
ℹ pass 44
ℹ fail 0
ℹ cancelled 0
ℹ skipped 3
ℹ todo 0
ℹ duration_ms 4870.7587
node:assert:173
throw err;
^

AssertionError [ERR_ASSERTION]: Unexpected global(s) found: DT_AGENT_INJECTED
at process. (C:\Users\M_Bhansali\Desktop\Projects\open source\node\test\common\index.js:396:14)
at process.emit (node:events:532:35) {
generatedMessage: false,
code: 'ERR_ASSERTION',
actual: undefined,
expected: undefined,
operator: 'fail'
}

I get this after regenerating the snapshots, any idea what this could be about?
Thanks

avivkeller · 2024-04-24T21:02:21Z

Chances are something that your PR changed caused an issue with testing in general. Try isolating the issue (via fiddling with the REPL or tests)

atlowChemi · 2024-04-24T22:19:17Z

@@ -0,0 +1,15 @@
+'use strict'
+
+const symbols = {


I am not sure how I feel about this name 🤔
WDYT about reporterUnicodeSymbolMap etc?

We could also export them directly, or would that be an issue with prototypes?

It is not an issue... we need this when creating an object, not a string (so module.exports.something = '\u2176' is just fine 🤷🏽‍♂️
But I don't think we should export them directly, as we would want this file to have additional common code for the reporters, see #52655 (comment)

atlowChemi · 2024-04-25T06:11:55Z

@mihir254 the submodule in the first commit is wrong, and in addition, the commit is 123 chars long, while the max allowed is 72.
Could you rebase and fix the commit message? If you need any help with that please let me know 🙂
see https://github.com/nodejs/node/actions/runs/8823097502/job/24235510052?pr=52655#step:6:13 and

node/doc/contributing/pull-requests.md

Lines 160 to 214 in 1681786

    
           #### Commit message guidelines 
        
           A good commit message should describe what changed and why. 
        
           1. The first line should: 
        
              * contain a short description of the change (preferably 50 characters or 
        
                less, and no more than 72 characters) 
        
              * be entirely in lowercase with the exception of proper nouns, acronyms, and 
        
                the words that refer to code, like function/variable names 
        
              * be prefixed with the name of the changed [subsystem](#appendix-subsystems) 
        
                and start with an imperative verb. Check the output of `git log --oneline 
        
                files/you/changed` to find out what subsystems your changes touch. 
        
              Examples: 
        
              * `net: add localAddress and localPort to Socket` 
        
              * `src: fix typos in async_wrap.h` 
        
           2. Keep the second line blank. 
        
           3. Wrap all other lines at 72 columns (except for long URLs). 
        
           4. If your patch fixes an open issue, you can add a reference to it at the end 
        
              of the log. Use the `Fixes:` prefix and the full issue URL. For other 
        
              references use `Refs:`. 
        
              Examples: 
        
              * `Fixes: https://github.com/nodejs/node/issues/1337` 
        
              * `Refs: https://eslint.org/docs/rules/space-in-parens.html` 
        
              * `Refs: https://github.com/nodejs/node/pull/3615` 
        
           5. If your commit introduces a breaking change (`semver-major`), it should 
        
              contain an explanation about the reason of the breaking change, which 
        
              situation would trigger the breaking change, and what is the exact change. 
        
           Sample complete commit message: 
        
           ```text 
        
           subsystem: explain the commit in one line 
        
           The body of the commit message should be one or more paragraphs, explaining 
        
           things in more detail. Please word-wrap to keep columns to 72 characters or 
        
           less. 
        
           Fixes: https://github.com/nodejs/node/issues/1337 
        
           Refs: https://eslint.org/docs/rules/space-in-parens.html 
        
           ``` 
        
           If you are new to contributing to Node.js, please try to do your best at 
        
           conforming to these guidelines, but do not worry if you get something wrong. 
        
           One of the existing contributors will help get things situated and the 
        
           contributor landing the pull request will ensure that everything follows 
        
           the project guidelines.

avivkeller

I'm not quite sure if I like the amount of changes this PR has introduced (I'm not sure if they are all needed), but that's just my opinion.

As for changes, the globals error in most snapshots shouldn't be there (I think). AFAIK there is not global with that name.

avivkeller · 2024-04-25T21:28:02Z

+  throw err;
+  ^
+
+AssertionError [ERR_ASSERTION]: Unexpected global(s) found: __DT_AGENT_INJECTED__


I'm not quite sure what this global is, but I don't think it was here before

I perhaps did not expect so many commits to line up in the PR. Should I create a fresh PR with the latest changes?
I have changed 3 test_runner/reporter files and 2 snapshots, in total.

I can also create 2 different PRs, one for displaying mages in dot router and other for adding a stack trace to it.

You could rebase and squash some commits, if this is an issue (it would also allow fixing the first commit message)

mcollina · 2024-04-26T08:09:17Z

@@ -1 +1,11 @@
 .X
+✖  Failed tests:


I think there might be a space too much before Failed tests:

Also, I'm would remove the x before and add a newline after the dots.

nodejs-github-bot · 2024-04-26T12:07:49Z

CI: https://ci.nodejs.org/job/node-test-pull-request/58715/

avivkeller · 2024-04-26T12:24:35Z

@mihir254 according to the CI, there may be a merge conflict in the dot reporter

08:07:52 CONFLICT (content): Merge conflict in lib/internal/test_runner/reporter/dot.js

nodejs-github-bot · 2024-05-07T06:08:37Z

CI: https://ci.nodejs.org/job/node-test-pull-request/59004/

MoLow

LGTM. the fix indeed made the tests pass - the failures are unrelated

nodejs-github-bot · 2024-05-07T08:03:44Z

CI: https://ci.nodejs.org/job/node-test-pull-request/59007/

nodejs-github-bot · 2024-05-07T10:44:46Z

Commit Queue failed

- Loading data for nodejs/node/pull/52655
✔  Done loading data for nodejs/node/pull/52655
----------------------------------- PR info ------------------------------------
Title      test_runner: display failed test stack trace with dot reporter (#52655)
Author     Mihir Bhansali  (@mihir254, first-time contributor)
Branch     mihir254:modifying-dot-reporter -> nodejs:main
Labels     author ready, needs-ci, commit-queue-squash, test_runner
Commits    6
 - test_runner: dot reporter displays failed test names
 - lint
 - fix tests
 - test_runner: fix assertions in dot-reporter tests
 - fixed indentation and lint errors
 - test_runner: reporterColorMap changed to map functions
Committers 1
 - Mihir Bhansali 
PR-URL: https://github.com/nodejs/node/pull/52655
Fixes: https://github.com/nodejs/node/issues/51769
Reviewed-By: Matteo Collina 
Reviewed-By: Moshe Atlow 
Reviewed-By: Chemi Atlow 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/52655
Fixes: https://github.com/nodejs/node/issues/51769
Reviewed-By: Matteo Collina 
Reviewed-By: Moshe Atlow 
Reviewed-By: Chemi Atlow 
--------------------------------------------------------------------------------
   ℹ  This PR was created on Tue, 23 Apr 2024 17:23:18 GMT
   ✔  Approvals: 3
   ✔  - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/52655#pullrequestreview-2042141691
   ✔  - Moshe Atlow (@MoLow) (TSC): https://github.com/nodejs/node/pull/52655#pullrequestreview-2042377524
   ✔  - Chemi Atlow (@atlowChemi): https://github.com/nodejs/node/pull/52655#pullrequestreview-2031600698
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2024-05-07T08:03:44Z: https://ci.nodejs.org/job/node-test-pull-request/59007/
- Querying data for job/node-test-pull-request/59007/
   ✔  Last Jenkins CI successful
   ⚠  PR author is a new contributor: @mihir254([email protected])
   ⚠  - commit 18eeb02527f3 is authored by [email protected]
   ⚠  - commit 748da16cb019 is authored by [email protected]
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu

https://github.com/nodejs/node/actions/runs/8983972314

PR-URL: nodejs#52655 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: Chemi Atlow <[email protected]>

MoLow · 2024-05-07T11:34:49Z

Landed in 69f2ace

MoLow · 2024-05-07T11:36:32Z

Thanks for the contribution @mihir254 🎉

nodejs-github-bot added needs-ci PRs that need a full CI run. test_runner Issues and PRs related to the test runner subsystem. labels Apr 23, 2024

avivkeller requested changes Apr 23, 2024

View reviewed changes

atlowChemi reviewed Apr 23, 2024

View reviewed changes

MoLow reviewed Apr 24, 2024

View reviewed changes

Comment thread lib/internal/test_runner/reporter/dot.js Outdated

mcollina requested changes Apr 24, 2024

View reviewed changes

mihir254 requested review from MoLow, atlowChemi, avivkeller and mcollina April 24, 2024 20:49

avivkeller requested changes Apr 24, 2024

View reviewed changes

avivkeller reviewed Apr 24, 2024

View reviewed changes

Comment thread lib/internal/test_runner/reporter/spec.js Outdated

Comment thread lib/internal/test_runner/reporter/utils.js

atlowChemi reviewed Apr 24, 2024

View reviewed changes

mihir254 changed the title ~~test: enhance dot reporter to display failed test names~~ test_runner: display failed test stack trace with dot reporter Apr 25, 2024

mihir254 requested a review from avivkeller April 25, 2024 21:26

avivkeller requested changes Apr 25, 2024

View reviewed changes

mihir254 requested a review from avivkeller April 26, 2024 04:38

MoLow reviewed Apr 26, 2024

View reviewed changes

Comment thread lib/internal/test_runner/reporter/utils.js Outdated

MoLow approved these changes Apr 26, 2024

View reviewed changes

mcollina reviewed Apr 26, 2024

View reviewed changes

avivkeller added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 26, 2024

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 26, 2024

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 7, 2024

MoLow approved these changes May 7, 2024

View reviewed changes

nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels May 7, 2024

test_runner: display failed test stack trace with dot reporter

69f2ace

PR-URL: nodejs#52655 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: Chemi Atlow <[email protected]>

MoLow force-pushed the modifying-dot-reporter branch from c295eaf to 69f2ace Compare May 7, 2024 11:34

MoLow merged commit 69f2ace into nodejs:main May 7, 2024

targos mentioned this pull request May 13, 2024

v22.2.0 release proposal #52971

Merged

avivkeller mentioned this pull request May 16, 2024

dot test-reporter includes more info #53031

Closed

targos mentioned this pull request Sep 30, 2024

v20.18.0 release proposal #55170

Merged

jsteiner mentioned this pull request Nov 28, 2025

test_runner: print failed coverage reports with dot runner #60884

Open

madkate42 mentioned this pull request Feb 19, 2026

test_runner: print coverage and diagnostics with dot reporter #61891

Open

KJyang-0114 mentioned this pull request Mar 3, 2026

test_runner: print failed coverage reports with dot reporter fixes #60884 #62091

Open

zakiscoding mentioned this pull request Mar 12, 2026

test_runner: print failed coverage reports with dot runner #62234

Open

	### `NODE_REGENERATE_SNAPSHOTS`

	If set, test snapshots for a the current test are regenerated.
	for example `NODE_REGENERATE_SNAPSHOTS=1 out/Release/node test/parallel/test-runner-output.mjs`
	will update all the test runner output snapshots.

Uh oh!

Conversation

mihir254 commented Apr 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Apr 23, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mihir254 Apr 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

avivkeller commented Apr 23, 2024

Uh oh!

atlowChemi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

MoLow commented Apr 24, 2024

Uh oh!

avivkeller left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mihir254 commented Apr 24, 2024

Uh oh!

avivkeller commented Apr 24, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

avivkeller Apr 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

atlowChemi commented Apr 25, 2024

Uh oh!

avivkeller left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mihir254 Apr 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

mihir254 commented Apr 23, 2024 •

edited

Loading

mihir254 Apr 23, 2024 •

edited

Loading

avivkeller Apr 25, 2024 •

edited

Loading

mihir254 Apr 26, 2024 •

edited

Loading