Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Feature request: report failing tests at the bottom of the spec reporter #47110

Closed
MoLow opened this issue Mar 15, 2023 · 4 comments · Fixed by #47164
Closed

Feature request: report failing tests at the bottom of the spec reporter #47110

MoLow opened this issue Mar 15, 2023 · 4 comments · Fixed by #47164
Labels
feature request Issues that request new features to be added to Node.js. good first issue Issues that are suitable for first-time contributors. test_runner Issues and PRs related to the test runner subsystem.

Comments

@MoLow
Copy link
Member

MoLow commented Mar 15, 2023 •
edited
Loading

What is the problem this feature will solve?

when test runs fail, it is currently needed to scroll up and manually search for the failing tests,

What is the feature you are proposing to solve the problem?

this behavior exists in some popular test runners (mocha, jest, playwright, etc),
re-printing the list of failing tests after the summary can be very useful

What alternatives have you considered?

scrolling to find the failing tests manually
@MoLow MoLow added good first issue Issues that are suitable for first-time contributors. feature request Issues that request new features to be added to Node.js. test_runner Issues and PRs related to the test runner subsystem. labels Mar 15, 2023
@MoLow MoLow changed the title report failing tests at the bottom of the spec reporter Feature request: report failing tests at the bottom of the spec reporter Mar 15, 2023
@itsAfnanAlam
Copy link

Hi, can I take this?

@HinataKah0
Copy link
Contributor

HinataKah0 commented Mar 18, 2023 •
edited
Loading

Hi,

I am interested to work on the changes.

Based on my understanding:

  • We need to output the failing tests report in root Test (lib/internal/test_runner/test.js).
  • Since #handleReportItem (lib/internal/test_runner/runner.js) immediately emits the test output to stream, we need to store the error output somewhere.
  • Eventually, all children Tests need to propagate the error outputs to their parents.
  • ArrayPrototypePush is concurrency safe.

Please correct me if I am wrong.
Draft commit here

I need suggestions about the output format.
I feel that if we output the whole diagnostics (including stack trace) again, it will make the output too cluttered since too many duplicates. Maybe we can omit the stack trace before the summary?
And I am worried about the memory usage, should we allow this feature to be disabled by choice (using flag)?

Also may I know how to properly re-generate test_runner_default_reporter.out?
Thanks!

HinataKah0 added a commit to HinataKah0/node that referenced this issue Mar 18, 2023
@HinataKah0
test_runner: report failing tests after summary
2dc1ab3 
Re-output failing tests after summary has been printed.
This behavior follows other popular test runners (e.g. jest, mocha, etc...).

Fixes: nodejs#47110
@MoLow
Copy link
Member Author

MoLow commented Mar 18, 2023

@HinataKah0 I would expect the change to only affect the spec reporter
whenever it receives a test:fail event, it can store the failed test, and when completed the reporting, it can print the failutes in the same format, at the end of the stream and with no indentation

@HinataKah0
Copy link
Contributor

HinataKah0 commented Mar 19, 2023 •
edited
Loading

Hi @MoLow ,

Thanks for the kind reply! Now the implementation is quite clear for me.
However, I noticed that we need to introduce a new event to mark the end of tests (e.g. test:eot).
Because test:diagnostic is sent a few times so it doesn't signify the ending. Actually I can maintain a counter but I don't think it's clean.

To illustrate what I am talking about here

Does this match to what you have in mind?
Screenshot_2023-03-19_at_7 12 19_PM
I think adding file path will be very useful.

It took me a while to understand how nodejs' build testing works (and to understand why I broke some tests). 🤣

Thanks!

HinataKah0 added a commit to HinataKah0/node that referenced this issue Mar 19, 2023
@HinataKah0
test_runner: report failing tests after summary
98fa308 
Re-output failing tests after summary has been printed.
This behavior follows other popular test runners (e.g. jest, mocha, etc...).

Updates SpecReporter:
1. When there is a 'test:fail' event, the failed test will be stored.
2. Introduce a new 'test:eot' event in which all the failed tests will be dumped.

Fixes: nodejs#47110
@HinataKah0 HinataKah0 mentioned this issue Mar 19, 2023
Merged
HinataKah0 added a commit to HinataKah0/node that referenced this issue Mar 21, 2023
@HinataKah0
test_runner: report failing tests after summary
abd6ef9 
Re-output failing tests after summary has been printed.
This behavior follows other popular test runners
(e.g. jest, mocha, etc...).

Updated SpecReporter:
1. When there is a 'test:fail' event, the test will be
stored.
2. After no more input, all the failed tests will be
flushed.
3. Extract the logic for formatting a test report
into a re-usable function.

Fixes: nodejs#47110
HinataKah0 added a commit to HinataKah0/node that referenced this issue Mar 21, 2023
@HinataKah0
test_runner: report failing tests after summary
62f9aa5 
Re-output failing tests after summary has been printed.
This behavior follows other popular test runners
(e.g. jest, mocha, etc...).

Updated SpecReporter:
1. When there is a 'test:fail' event, the test will be
stored.
2. After no more input, all the failed tests will be
flushed.
3. Extract the logic for formatting a test report
into a re-usable function.

Fixes: nodejs#47110
HinataKah0 added a commit to HinataKah0/node that referenced this issue Mar 21, 2023
@HinataKah0
test_runner: report failing tests after summary
55a4b28 
Re-output failing tests after summary has been printed.
This behavior follows other popular test runners
(e.g. jest, mocha, etc...).

Updated SpecReporter:
1. When there is a 'test:fail' event, the test will be
stored.
2. After no more input, all the failed tests will be
flushed.
3. Extract the logic for formatting a test report
into a re-usable function.

Fixes: nodejs#47110
HinataKah0 added a commit to HinataKah0/node that referenced this issue Mar 22, 2023
@HinataKah0
test_runner: report failing tests after summary
63b1965 
Re-output failing tests after summary has been printed.
This behavior follows other popular test runners
(e.g. jest, mocha, etc...).

Updated SpecReporter:
1. When there is a 'test:fail' event, the test will be
stored.
2. After no more input, all the failed tests will be
flushed.
3. Extract the logic for formatting a test report
into a re-usable function.

Fixes: nodejs#47110
HinataKah0 added a commit to HinataKah0/node that referenced this issue Mar 22, 2023
@HinataKah0
test_runner: report failing tests after summary
57a35b3 
Re-output failing tests after summary has been printed.
This behavior follows other popular test runners
(e.g. jest, mocha, etc...).

Updated SpecReporter:
1. When there is a 'test:fail' event, the test will be
stored.
2. After no more input, all the failed tests will be
flushed.
3. Extract the logic for formatting a test report
into a re-usable function.

Fixes: nodejs#47110
nodejs-github-bot pushed a commit that referenced this issue Mar 23, 2023
@HinataKah0
test_runner: report failing tests after summary
2e8f8eb 
Re-output failing tests after summary has been printed.
This behavior follows other popular test runners
(e.g. jest, mocha, etc...).

Updated SpecReporter:
1. When there is a 'test:fail' event, the test will be
stored.
2. After no more input, all the failed tests will be
flushed.
3. Extract the logic for formatting a test report
into a re-usable function.

Fixes: #47110
PR-URL: #47164
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
RafaelGSS pushed a commit that referenced this issue Apr 5, 2023
@HinataKah0 @RafaelGSS
test_runner: report failing tests after summary
90a8910 
Re-output failing tests after summary has been printed.
This behavior follows other popular test runners
(e.g. jest, mocha, etc...).

Updated SpecReporter:
1. When there is a 'test:fail' event, the test will be
stored.
2. After no more input, all the failed tests will be
flushed.
3. Extract the logic for formatting a test report
into a re-usable function.

Fixes: #47110
PR-URL: #47164
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
RafaelGSS pushed a commit that referenced this issue Apr 7, 2023
@HinataKah0 @RafaelGSS
test_runner: report failing tests after summary
3e7f8e8 
Re-output failing tests after summary has been printed.
This behavior follows other popular test runners
(e.g. jest, mocha, etc...).

Updated SpecReporter:
1. When there is a 'test:fail' event, the test will be
stored.
2. After no more input, all the failed tests will be
flushed.
3. Extract the logic for formatting a test report
into a re-usable function.

Fixes: #47110
PR-URL: #47164
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
danielleadams pushed a commit that referenced this issue Jul 6, 2023
@HinataKah0 @danielleadams
test_runner: report failing tests after summary
9cbf897 
Re-output failing tests after summary has been printed.
This behavior follows other popular test runners
(e.g. jest, mocha, etc...).

Updated SpecReporter:
1. When there is a 'test:fail' event, the test will be
stored.
2. After no more input, all the failed tests will be
flushed.
3. Extract the logic for formatting a test report
into a re-usable function.

Fixes: #47110
PR-URL: #47164
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
feature request Issues that request new features to be added to Node.js. good first issue Issues that are suitable for first-time contributors. test_runner Issues and PRs related to the test runner subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

test_runner: report failing tests after summary HinataKah0/node
3 participants
@MoLow @itsAfnanAlam @HinataKah0