test_runner: display failed test stack trace with dot reporter #52655

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

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

lib/internal/test_runner/reporter/dot.js

@@ -23,6 +29,14 @@ module.exports = async function* dot(source) {
    }
  }
  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

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

lib/internal/test_runner/reporter/dot.js

    if (type === 'test:pass') {
      yield '.';
    }
    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)

RedYetiDev · 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.

lib/internal/test_runner/reporter/dot.js

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

lib/internal/test_runner/reporter/dot.js

@@ -23,6 +29,14 @@ module.exports = async function* dot(source) {
    }
  }
  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

lib/internal/test_runner/reporter/dot.js

+  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?

lib/internal/test_runner/reporter/dot.js

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)

RedYetiDev

A lot of the test snapshots should not have changed

lib/internal/test_runner/reporter/spec.js

lib/internal/test_runner/reporter/utils.js

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

RedYetiDev · 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

lib/internal/test_runner/reporter/utils.js

@@ -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.

RedYetiDev

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.

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

test/fixtures/test-runner/output/describe_nested.snapshot

+  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)

lib/internal/test_runner/reporter/utils.js

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

test/fixtures/test-runner/output/eval_dot.snapshot

@@ -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/

RedYetiDev · 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-05T07:42:36Z

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

MoLow · 2024-05-05T08:26:18Z

lib/internal/test_runner/reporter/spec.js


  constructor() {
    super({ __proto__: null, writableObjectMode: true });
    colors.refresh();
-    this.#inspectOptions = { __proto__: null, colors: colors.shouldColorize(process.stdout), breakLength: Infinity };
-    this.#colors = {


moving colors to utils breaks when there is no TTY, see https://ci.nodejs.org/job/node-test-commit-custom-suites-freestyle/35939/console:

colors cannot be deconstructed too early.

11:01:44 not ok 41 - test-runner/output/arbitrary-output-colored.js 11:01:44 --- 11:01:44 duration_ms: 5585.15492 11:01:44 location: '/home/iojs/build/workspace/node-test-commit-custom-suites-freestyle/test/parallel/test-runner-output.mjs:155:5' 11:01:44 failureType: 'testCodeFailure' 11:01:44 error: |- 11:01:44 Expected values to be strictly equal: 11:01:44 + actual - expected ... Lines skipped 11:01:44 11:01:44 "{ foo: [32m'bar'[39m }\n" + 11:01:44 '[33m1[39m\n' + 11:01:44 + '✔ passing test [90m(*ms)[39m[39m\n' + 11:01:44 + 'ℹ tests 1[39m\n' + 11:01:44 + 'ℹ suites 0[39m\n' + 11:01:44 + 'ℹ pass 1[39m\n' + 11:01:44 + 'ℹ fail 0[39m\n' + 11:01:44 + 'ℹ cancelled 0[39m\n' + 11:01:44 + 'ℹ skipped 0[39m\n' + 11:01:44 + 'ℹ todo 0[39m\n' + 11:01:44 + 'ℹ duration_ms *[39m\n' + 11:01:44 - '[32m✔ passing test [90m(*ms)[39m[39m\n' + 11:01:44 - '[34mℹ tests 1[39m\n' + 11:01:44 - '[34mℹ suites 0[39m\n' + 11:01:44 - '[34mℹ pass 1[39m\n' + 11:01:44 - '[34mℹ fail 0[39m\n' + 11:01:44 - '[34mℹ cancelled 0[39m\n' + 11:01:44 - '[34mℹ skipped 0[39m\n' + 11:01:44 - '[34mℹ todo 0[39m\n' + 11:01:44 - '[34mℹ duration_ms *[39m\n' + 11:01:44 'TAP version 13\n' + 11:01:44 ... 11:01:44 '# skipped 0\n' + 11:01:44 '# todo 0\n' + 11:01:44 '# duration_ms *\n'

changing reporterColorMap to a function might be the easiest fix

arbitrary-output-colored test passes on my machine. How can I reproduce the error?

changing reporterColorMap to a function might be the easiest fix

are you suggesting to replace the map with a function in utils.js or modify the constructor in spec.js?

test passes on my machine. How can I reproduce the error?

with FORCE_COLOR=0 NODE_REGENERATE_SNAPSHOTS=1 ./node test/parallel/test-runner-output.mjs

are you suggesting to replace the map with a function in utils.js or modify the constructor in spec.js?

either will work I think

this might be an easier & cleaner fix:

diff --git a/lib/internal/test_runner/reporter/utils.js b/lib/internal/test_runner/reporter/utils.js index caa35959ee..42c4ffa3cd 100644 --- a/lib/internal/test_runner/reporter/utils.js +++ b/lib/internal/test_runner/reporter/utils.js @@ -28,9 +28,15 @@ const reporterUnicodeSymbolMap = { const reporterColorMap = { '__proto__': null, - 'test:fail': colors.red, - 'test:pass': colors.green, - 'test:diagnostic': colors.blue, + get 'test:fail'() { + return colors.red; + }, + get 'test:pass'() { + return colors.green; + }, + get 'test:diagnostic'() { + return colors.blue; + }, };

test passes on my machine. How can I reproduce the error?

with FORCE_COLOR=0 NODE_REGENERATE_SNAPSHOTS=1 ./node test/parallel/test-runner-output.mjs

Still passes all tests :(

I have made the suggested changes, please let me know if I need to add anything else.

RedYetiDev · 2024-05-05T15:48:38Z

Your first commit was 48eecd379b73dc9be0959f2f919254a8c5f11fc8, so maybe try squashing everything after that commit?

Then you can more easier handle conflicts? (Gotten via git pull)

IIRC this correct, but take it with a grain of salt

mihir254 · 2024-05-06T18:52:41Z

There are some tests failing for other reasons:
example - node-test-commit-windows-fanned failed

How do I go about fixing these errors? Is there a specific file that I need to include? Thanks!

RedYetiDev · 2024-05-06T19:51:32Z

There are some tests failing for other reasons: example - node-test-commit-windows-fanned failed How do I go about fixing these errors? Is there a specific file that I need to include? Thanks!

IIRC you can ignore those, as it's a CI issue, not a PR issue.

mcollina

lgtm

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(mbmihirbhansali@gmail.com)
   ⚠  - commit 18eeb02527f3 is authored by moshe@atlow.co.il
   ⚠  - commit 748da16cb019 is authored by moshe@atlow.co.il
--------------------------------------------------------------------------------
   ✔  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 <matteo.collina@gmail.com> Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Chemi Atlow <chemi@atlow.co.il>

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

Landed in 69f2ace

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

Thanks for the contribution @mihir254 🎉

PR-URL: nodejs#52655 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Chemi Atlow <chemi@atlow.co.il>

PR-URL: #52655 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Chemi Atlow <chemi@atlow.co.il>

nodejs-github-bot added needs-ci PRs that need a full CI run. test_runner labels Apr 23, 2024

RedYetiDev suggested changes Apr 23, 2024

View reviewed changes

atlowChemi reviewed Apr 23, 2024

View reviewed changes

MoLow reviewed Apr 24, 2024

View reviewed changes

lib/internal/test_runner/reporter/dot.js Outdated Show resolved Hide resolved

mcollina requested changes Apr 24, 2024

View reviewed changes

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

RedYetiDev suggested changes Apr 24, 2024

View reviewed changes

RedYetiDev reviewed Apr 24, 2024

View reviewed changes

lib/internal/test_runner/reporter/spec.js Outdated Show resolved Hide resolved

lib/internal/test_runner/reporter/utils.js Show resolved Hide resolved

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 RedYetiDev April 25, 2024 21:26

RedYetiDev suggested changes Apr 25, 2024

View reviewed changes

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

MoLow reviewed Apr 26, 2024

View reviewed changes

lib/internal/test_runner/reporter/utils.js Outdated Show resolved Hide resolved

MoLow approved these changes Apr 26, 2024

View reviewed changes

mcollina reviewed Apr 26, 2024

View reviewed changes

RedYetiDev 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

MoLow approved these changes May 5, 2024

View reviewed changes

MoLow added the request-ci Add this label to start a Jenkins CI on a PR. label May 5, 2024

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

MoLow reviewed May 5, 2024

View reviewed changes

mihir254 force-pushed the modifying-dot-reporter branch from b4c8223 to dcf3e74 Compare May 6, 2024 19:47

mihir254 force-pushed the modifying-dot-reporter branch from dcf3e74 to c295eaf Compare May 6, 2024 19:54

mihir254 requested a review from MoLow May 7, 2024 06:02

mcollina approved these changes May 7, 2024

View reviewed changes

mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label May 7, 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 <matteo.collina@gmail.com> Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Chemi Atlow <chemi@atlow.co.il>

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
16 checks passed

	### `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.

test_runner: display failed test stack trace with dot reporter #52655

test_runner: display failed test stack trace with dot reporter #52655

Conversation

mihir254 commented Apr 23, 2024 • edited

nodejs-github-bot commented Apr 23, 2024

Choose a reason for hiding this comment

mihir254 Apr 23, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RedYetiDev commented Apr 23, 2024

atlowChemi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcollina left a comment

Choose a reason for hiding this comment

MoLow commented Apr 24, 2024

RedYetiDev left a comment

Choose a reason for hiding this comment

mihir254 commented Apr 24, 2024

RedYetiDev commented Apr 24, 2024

Choose a reason for hiding this comment

RedYetiDev Apr 25, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

atlowChemi commented Apr 25, 2024

RedYetiDev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mihir254 Apr 26, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nodejs-github-bot commented Apr 26, 2024

RedYetiDev commented Apr 26, 2024

nodejs-github-bot commented May 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RedYetiDev commented May 5, 2024 • edited

mihir254 commented May 6, 2024

RedYetiDev commented May 6, 2024

mcollina left a comment

Choose a reason for hiding this comment

nodejs-github-bot commented May 7, 2024

MoLow left a comment

Choose a reason for hiding this comment

nodejs-github-bot commented May 7, 2024

nodejs-github-bot commented May 7, 2024

MoLow commented May 7, 2024

MoLow commented May 7, 2024

mihir254 commented Apr 23, 2024 •

edited

mihir254 Apr 23, 2024 •

edited

RedYetiDev Apr 25, 2024 •

edited

mihir254 Apr 26, 2024 •

edited

RedYetiDev commented May 5, 2024 •

edited