test_runner: display failed test stack trace with dot reporter

[mihir254]

Add failed test names and stack trace to dot reporter output

Fixes: https://github.com/nodejs/node/issues/51769

[nodejs-github-bot]

Review requested:

• [ ] @nodejs/test_runner

[redyetidev]

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)

[MoLow]

Can you please add a unit test?

there are existing tests in place that need to be updated. see https://github.com/nodejs/node/pull/52655#pullrequestreview-2018141409

[mihir254]

▶ 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]

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]

@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 https://github.com/nodejs/node/blob/168178658e1ae980345e148d154d6e5516f93fa0/doc/contributing/pull-requests.md#L160-L214

[nodejs-github-bot]

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

[redyetidev]

@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

[mihir254]

@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

How do you suggest I go about fixing this? Sync fork, resolve conflict, add new changes and commit again?

[redyetidev]

@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

How do you suggest I go about fixing this? Sync fork, resolve conflict, add new changes and commit again?

Basically yes.

[github-actions[bot]]

Failed to start CI

   ⚠  Something was pushed to the Pull Request branch since the last approving review.
   ✘  Refusing to run CI on potentially unsafe PR

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

[nodejs-github-bot]

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

[aduh95]

It looks like the linter is complaining about the indentation, can you please run make lint-js-fix and commit the results?

[mihir254]

Is there a windows version of make lint-js-fix? I tried .\vcbuild lint-js-fix but it is not a valid command line option. Also, .\vcbuild lint only lints the markdown files on windows, but it is mentioned in the BUILDING.md file that it lints JavaScript too.

[redyetidev]

AFAIK lint-js (make) works, so maybe try that with vcbuild? You can also run eslint manually

[benjamingr]

It looks okay, but I don't want to approve/request changes until a CI is ran.

You're welcome to review code changes (as in make recommendations) but note your approval/request-changes currently carries no weight process wise. (As in, approving doesn't mean stuff can land and requesting changes doesn't block). Please be careful about how it's perceived and keep contributing until you earn review priviledges :)

[redyetidev]

Yes, sorry. I just meant that the last CI had a merge issue, and I didn't want to note that it looks good to me until that was resolved

Sorry if I overstepped

[mihir254]

running lint-js

Oops! Something went wrong! :(

ESLint: 8.57.0

C:\Users\........\node\tools\node_modules\eslint\node_modules\eslint:1
..
^

SyntaxError: Unexpected token '.'
    at wrapSafe (node:internal/modules/cjs/loader:1378:20)
    at Module._compile (node:internal/modules/cjs/loader:1435:41)
    at Module._extensions..js (node:internal/modules/cjs/loader:1555:10)
    at Module.load (node:internal/modules/cjs/loader:1288:32)
    at Module._load (node:internal/modules/cjs/loader:1104:12)
    at Module.require (node:internal/modules/cjs/loader:1311:19)
    at require (node:internal/modules/helpers:179:18)
    at Object.<anonymous> (C:\Users\........\node\tools\node_modules\eslint\node_modules\eslint-plugin-jsdoc\dist\rules\checkExamples.cjs:8:39)
    at Module._compile (node:internal/modules/cjs/loader:1476:14)
    at Module._extensions..js (node:internal/modules/cjs/loader:1555:10)

I am having an issue with the lint.

[mihir254]

Are some of the tests (like test-macOS, etc.) failing because they are expecting the original dot-reporter response? How can I go about fixing them? Also, still no luck with linting. I have manually fixed indentation errors, but I was still hoping to run a lint test before committing, just to be sure.

[redyetidev]

Are some of the tests (like test-macOS, etc.) failing because they are expecting the original dot-reporter response? How can I go about fixing them?

Also, still no luck with linting. I have manually fixed indentation errors, but I was still hoping to run a lint test before committing, just to be sure.

My recommendation is to figure out what you want to see, and make sure that is what the tests are printing. If yes, then that may be the issue, and if not, it may be something else.

[MoLow]

@mihir254 I am not sure what is wrong with running linter by you, but if you are having trouble, I can fix lint issues and push to this branch

[mihir254]

@MoLow that'll be great, thank you!

[mihir254]

I see that test-runner-reporters keep failing as it expects to see .XX. instead of the updated dot-reporter output. The expected values are hard-coded in the test. How should I go about updating this test?

[MoLow]

@mihir254 I fixed lint & tests

[mihir254]

I think the parallel/test-runner-reporters still needs to be fixed.

It does not expect to see the failing tests (test/parallel/test-runner-reporters.js line 84). Any directions on that?

[MoLow]

It does not expect to see the failing tests (test/parallel/test-runner-reporters.js line 84). Any directions on that?

you should probably copy what was done in the spec reporter and adjust

[MoLow]

@mihir254 please squash/remove the merge commit. if you need help with that or you want me to perform that, LMK

[mcollina]

linting is failing too: https://github.com/nodejs/node/actions/runs/8898858755/job/24459540180?pr=52655#step:5:10.

[nodejs-github-bot]

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

[MoLow]

I am afraid the merge commits will prevent landing this PR, can you please remove this before we trigger the CI?