-
Notifications
You must be signed in to change notification settings - Fork 29.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
test_runner: display failed test stack trace with dot reporter #52655
Conversation
Review requested:
|
@@ -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`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use uppercase when possible
Failed tests:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I'll keep that in mind. I assumed it should be consistent with the spec reporter, which uses "failing tests:"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nodejs/test_runner WDYT about the formatting?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should I also make it Failed tests:
for spec reporter?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can be in a separate PR
if (type === 'test:pass') { | ||
yield '.'; | ||
} | ||
if (type === 'test:fail') { | ||
yield 'X'; | ||
failedTests.push(data); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the internals of Node.js, we use primordials, so try to use them when possible (ArrayPrototypePush
)
Also, please lint your source code and commit message. (You just need to the put the change in the commit message, E.G. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
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. |
@@ -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`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We might want to use util.styleText
, though I am not sure we want to, as it could be affected from user-land
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be worth to change it globally, but I think that’s an issue for a different PR.
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`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't we need to reset the output color after this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please add a unit test?
there are existing tests in place that need to be updated. see #52655 (review) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A lot of the test snapshots should not have changed
▶ test runner output (4860.751ms) AssertionError [ERR_ASSERTION]: Unexpected global(s) found: DT_AGENT_INJECTED I get this after regenerating the snapshots, any idea what this could be about? |
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) |
@@ -0,0 +1,15 @@ | |||
'use strict' | |||
|
|||
const symbols = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure how I feel about this name 🤔
WDYT about reporterUnicodeSymbolMap
etc?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could also export them directly, or would that be an issue with prototypes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
@mihir254 the submodule in the first commit is wrong, and in addition, the commit is 123 chars long, while the max allowed is 72. node/doc/contributing/pull-requests.md Lines 160 to 214 in 1681786
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
throw err; | ||
^ | ||
|
||
AssertionError [ERR_ASSERTION]: Unexpected global(s) found: __DT_AGENT_INJECTED__ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not quite sure what this global is, but I don't think it was here before
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can also create 2 different PRs, one for displaying mages in dot router and other for adding a stack trace to it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could rebase and squash some commits, if this is an issue (it would also allow fixing the first commit message)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
@@ -1 +1,11 @@ | |||
.X | |||
✖ Failed tests: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@mihir254 according to the CI, there may be a merge conflict in the dot reporter
|
b4c8223
to
dcf3e74
Compare
dcf3e74
to
c295eaf
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. the fix indeed made the tests pass - the failures are unrelated
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/.ncuhttps://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>
c295eaf
to
69f2ace
Compare
Landed in 69f2ace |
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>
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: 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>
Add failed test names and stack trace to dot reporter output
Fixes: #51769