test_runner: report failing tests after summary #47164

HinataKah0 · 2023-03-19T15:03:02Z

Re-output failing tests after summary has been printed. This behavior follows other popular test runners (e.g. jest, mocha, etc...).

Updated SpecReporter:

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

Fixes: #47110

nodejs-github-bot · 2023-03-19T15:03:07Z

Review requested:

@nodejs/test_runner

lib/internal/test_runner/reporter/spec.js

MoLow · 2023-03-19T15:24:45Z

lib/internal/test_runner/test.js

@@ -655,6 +655,8 @@ class Test extends AsyncResource {
        this.reporter.coverage(this.nesting, kFilename, this.harness.coverage);
      }

+      this.reporter.endOfTests();


use _flush to spare this change

test/pseudo-tty/test_runner_default_reporter.out

MoLow · 2023-03-19T15:27:02Z

thanks for your contribution! I have left some comments

HinataKah0 · 2023-03-21T11:08:02Z

I've addressed the comments.

Thanks for the feedbacks, I learned a few things! 😄

And I found a minor issue, if duration_ms is not printed, the text color is not reset.

^ I tried the main branch

Shall we fix it in this PR? Or it is better to open another PR to address this specific issue?
The latter is tidier in workflow.

lib/internal/test_runner/reporter/spec.js

MoLow · 2023-03-21T12:17:47Z

Shall we fix it in this PR? Or it is better to open another PR to address this specific issue?

better to fix in a seperate PR

MoLow

LGTM, bet there seems to be lint issues

MoLow · 2023-03-21T20:46:40Z

lib/internal/test_runner/reporter/spec.js

+      color = gray;
+      symbol = symbols['hyphen:minus'];
+    }
+    return `${prefix}${indent}${color}${symbol}${title}${error}${white}`;


Suggested change

return `${prefix}${indent}${color}${symbol}${title}${error}${white}`;

return `${prefix}${indent}${color}${symbol}${title}${white}${error}`;

this is the fix for #47164 (comment)
since it is a small fix, I think it would be ok to include it in this PR

Shall we add a test for this as well? In test/pseudo-tty/test_runner_default_reporter.out.

test('parent', () => { test('should fail', () => { throw new Error('fail'); }); test('should pass but parent fail', () => {}); });

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 · 2023-03-22T11:29:31Z

Resolved lint error (extracted the code to handle test report event and called it in both test:pass and test:fail)
Rebased to resolve conflicts because of adding test

MoLow · 2023-03-22T16:23:33Z

test/pseudo-tty/test_runner_default_reporter.out

+    [90m    at *[39m
+    [90m    at *[39m
+
+  [31m✖ should pass but parent fail [90m(*ms)[39m[39m


Suggested change

[31m✖ should pass but parent fail [90m(*ms)[39m[39m

[31m* should pass but parent fail [90m(*ms)[39m[39m

Ah...

Thanks for catching this, I totally missed it. 😨
Let me verify again.

Upon 'test:fail', text color is not reset correctly if duration is not printed.

nodejs-github-bot · 2023-03-23T13:40:11Z

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

nodejs-github-bot · 2023-03-23T18:45:55Z

Landed in 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>

This commit updates a test runner fixture that includes a failing child test. However, the nested test is created using the top level test() function instead t.test(). This commit updates the fixture to use t.test(), while preserving the expected failure. Refs: nodejs#47164

This commit updates a test runner fixture that includes a failing child test. However, the nested test is created using the top level test() function instead t.test(). This commit updates the fixture to use t.test(), while preserving the expected failure. Refs: #47164 PR-URL: #52185 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Chemi Atlow <chemi@atlow.co.il>

Prior to this commit, each top level test awaited the same global promise for setting up test reporters. This commit updates the logic to only await the promise the first time it is encountered. PR-URL: #52185 Refs: #47164 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Chemi Atlow <chemi@atlow.co.il>

This commit updates a test runner fixture that includes a failing child test. However, the nested test is created using the top level test() function instead t.test(). This commit updates the fixture to use t.test(), while preserving the expected failure. Refs: nodejs#47164 PR-URL: nodejs#52185 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Chemi Atlow <chemi@atlow.co.il>

Prior to this commit, each top level test awaited the same global promise for setting up test reporters. This commit updates the logic to only await the promise the first time it is encountered. PR-URL: nodejs#52185 Refs: nodejs#47164 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Chemi Atlow <chemi@atlow.co.il>

This commit updates a test runner fixture that includes a failing child test. However, the nested test is created using the top level test() function instead t.test(). This commit updates the fixture to use t.test(), while preserving the expected failure. Refs: nodejs#47164 PR-URL: nodejs#52185 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Chemi Atlow <chemi@atlow.co.il>

Prior to this commit, each top level test awaited the same global promise for setting up test reporters. This commit updates the logic to only await the promise the first time it is encountered. PR-URL: nodejs#52185 Refs: nodejs#47164 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Chemi Atlow <chemi@atlow.co.il>

This commit updates a test runner fixture that includes a failing child test. However, the nested test is created using the top level test() function instead t.test(). This commit updates the fixture to use t.test(), while preserving the expected failure. Refs: #47164 PR-URL: #52185 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Chemi Atlow <chemi@atlow.co.il>

Prior to this commit, each top level test awaited the same global promise for setting up test reporters. This commit updates the logic to only await the promise the first time it is encountered. PR-URL: #52185 Refs: #47164 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Chemi Atlow <chemi@atlow.co.il>

This commit updates a test runner fixture that includes a failing child test. However, the nested test is created using the top level test() function instead t.test(). This commit updates the fixture to use t.test(), while preserving the expected failure. Refs: #47164 PR-URL: #52185 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Chemi Atlow <chemi@atlow.co.il>

Prior to this commit, each top level test awaited the same global promise for setting up test reporters. This commit updates the logic to only await the promise the first time it is encountered. PR-URL: #52185 Refs: #47164 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Chemi Atlow <chemi@atlow.co.il>

nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Mar 19, 2023

MoLow reviewed Mar 19, 2023

View reviewed changes

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

MoLow reviewed Mar 19, 2023

View reviewed changes

test/pseudo-tty/test_runner_default_reporter.out Outdated Show resolved Hide resolved

HinataKah0 force-pushed the report-failing-tests branch from 98fa308 to abd6ef9 Compare March 21, 2023 10:42

MoLow reviewed Mar 21, 2023

View reviewed changes

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

MoLow reviewed Mar 21, 2023

View reviewed changes

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

HinataKah0 force-pushed the report-failing-tests branch 2 times, most recently from 62f9aa5 to 55a4b28 Compare March 21, 2023 15:37

MoLow reviewed Mar 21, 2023

View reviewed changes

HinataKah0 force-pushed the report-failing-tests branch from 55a4b28 to 0b7bf9a Compare March 22, 2023 11:03

HinataKah0 force-pushed the report-failing-tests branch from 0b7bf9a to c22d1e9 Compare March 22, 2023 11:27

MoLow reviewed Mar 22, 2023

View reviewed changes

MoLow added commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. test_runner Issues and PRs related to the test runner subsystem. labels Mar 22, 2023

test_runner: fix improper text color reset

c4fd2c7

Upon 'test:fail', text color is not reset correctly if duration is not printed.

HinataKah0 force-pushed the report-failing-tests branch from c22d1e9 to c4fd2c7 Compare March 22, 2023 16:54

MoLow added the request-ci Add this label to start a Jenkins CI on a PR. label Mar 23, 2023

MoLow approved these changes Mar 23, 2023

View reviewed changes

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 23, 2023

MoLow added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 23, 2023

cjihrig approved these changes Mar 23, 2023

View reviewed changes

MoLow added the commit-queue Add this label to land a pull request using GitHub Actions. label Mar 23, 2023

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Mar 23, 2023

nodejs-github-bot merged commit 2e8f8eb into nodejs:main Mar 23, 2023

RafaelGSS mentioned this pull request Apr 3, 2023

2023-04-18, Version 20.0.0 (Current) #47381

Merged

RafaelGSS mentioned this pull request Apr 6, 2023

v19.9.0 proposal #47441

Merged

HinataKah0 deleted the report-failing-tests branch May 14, 2023 09:33

danielleadams mentioned this pull request Jul 10, 2023

v18.17.0 release proposal #48694

Merged

MoLow mentioned this pull request Jul 24, 2023

.cause of errors innode:test test cases not available to custom reporter #48900

Closed

cjihrig mentioned this pull request Mar 22, 2024

test_runner: don't await the same promise for each test #52185

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test_runner: report failing tests after summary #47164

test_runner: report failing tests after summary #47164

HinataKah0 commented Mar 19, 2023 •

edited

Loading

nodejs-github-bot commented Mar 19, 2023

MoLow Mar 19, 2023 •

edited

Loading

MoLow commented Mar 19, 2023

HinataKah0 commented Mar 21, 2023

MoLow commented Mar 21, 2023

MoLow left a comment

MoLow Mar 21, 2023

HinataKah0 Mar 22, 2023 •

edited

Loading

HinataKah0 commented Mar 22, 2023 •

edited

Loading

MoLow Mar 22, 2023

HinataKah0 Mar 22, 2023

nodejs-github-bot commented Mar 23, 2023

nodejs-github-bot commented Mar 23, 2023

	return `${prefix}${indent}${color}${symbol}${title}${error}${white}`;
	return `${prefix}${indent}${color}${symbol}${title}${white}${error}`;

	[31m✖ should pass but parent fail [90m(*ms)[39m[39m
	[31m* should pass but parent fail [90m(*ms)[39m[39m

test_runner: report failing tests after summary #47164

test_runner: report failing tests after summary #47164

Conversation

HinataKah0 commented Mar 19, 2023 • edited Loading

nodejs-github-bot commented Mar 19, 2023

MoLow Mar 19, 2023 • edited Loading

Choose a reason for hiding this comment

MoLow commented Mar 19, 2023

HinataKah0 commented Mar 21, 2023

MoLow commented Mar 21, 2023

MoLow left a comment

Choose a reason for hiding this comment

MoLow Mar 21, 2023

Choose a reason for hiding this comment

HinataKah0 Mar 22, 2023 • edited Loading

Choose a reason for hiding this comment

HinataKah0 commented Mar 22, 2023 • edited Loading

MoLow Mar 22, 2023

Choose a reason for hiding this comment

HinataKah0 Mar 22, 2023

Choose a reason for hiding this comment

nodejs-github-bot commented Mar 23, 2023

nodejs-github-bot commented Mar 23, 2023

HinataKah0 commented Mar 19, 2023 •

edited

Loading

MoLow Mar 19, 2023 •

edited

Loading

HinataKah0 Mar 22, 2023 •

edited

Loading

HinataKah0 commented Mar 22, 2023 •

edited

Loading