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

test_runner: add level-based diagnostic handling for reporter #55964

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
+58 −4
Open

test_runner: add level-based diagnostic handling for reporter #55964

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
7dda539
test_runner: add level parameter to reporter.diagnostic
hpatel292-seneca Nov 23, 2024
c618874
test_runner: enhance #handleEvent to handle levels
hpatel292-seneca Nov 23, 2024
2ac841c
test_runner: extend reporterColorMap for levels
hpatel292-seneca Nov 23, 2024
1ca0d45
test_runner: set level for coverage threshold
hpatel292-seneca Nov 23, 2024
038b0f8
test_runner: remove debug from reporterColorMap
hpatel292-seneca Nov 24, 2024
b490d9d
doc: update 'test:diagnostic' event to include level parameter
hpatel292-seneca Nov 26, 2024
3e4d5e6
test: add test for spec reporter red color output
hpatel292-seneca Nov 27, 2024
4048621
test: disable no-control-regex for color regex
hpatel292-seneca Nov 27, 2024
33b961d
doc: clarify diagnostic behavior in API docs
hpatel292-seneca Nov 28, 2024
b1c8033
test: validate diagnostic events in test runner
hpatel292-seneca Dec 13, 2024
c06e9e1
test: Fix Lint error
hpatel292-seneca Dec 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions doc/api/test.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2944,6 +2944,11 @@ defined. The corresponding declaration ordered event is `'test:start'`.
`undefined` if the test was run through the REPL.
* `message` {string} The diagnostic message.
* `nesting` {number} The nesting level of the test.
* `level` {string} The severity level of the diagnostic message.
Possible values are:
* `'info'`: Informational messages.
* `'warn'`: Warnings.
* `'error'`: Errors.

Emitted when [`context.diagnostic`][] is called.
This event is guaranteed to be emitted in the same order as the tests are
Expand Down
6 changes: 4 additions & 2 deletions lib/internal/test_runner/reporter/spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,8 +69,10 @@ class SpecReporter extends Transform {
case 'test:stderr':
case 'test:stdout':
return data.message;
case 'test:diagnostic':
return `${reporterColorMap[type]}${indent(data.nesting)}${reporterUnicodeSymbolMap[type]}${data.message}${colors.white}\n`;
case 'test:diagnostic':{
const diagnosticColor = reporterColorMap[data.level] || reporterColorMap['test:diagnostic'];
return `${diagnosticColor}${indent(data.nesting)}${reporterUnicodeSymbolMap[type]}${data.message}${colors.white}\n`;
}
case 'test:coverage':
return getCoverageReport(indent(data.nesting), data.summary,
reporterUnicodeSymbolMap['test:coverage'], colors.blue, true);
Expand Down
9 changes: 9 additions & 0 deletions lib/internal/test_runner/reporter/utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,15 @@ const reporterColorMap = {
get 'test:diagnostic'() {
return colors.blue;
},
get 'info'() {
return colors.blue;
},
get 'warn'() {
return colors.yellow;
},
get 'error'() {
return colors.red;
},
};

function indent(nesting) {
Expand Down
2 changes: 1 addition & 1 deletion lib/internal/test_runner/test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1087,7 +1087,7 @@ class Test extends AsyncResource {
if (actual < threshold) {
harness.success = false;
process.exitCode = kGenericUserError;
reporter.diagnostic(nesting, loc, `Error: ${NumberPrototypeToFixed(actual, 2)}% ${name} coverage does not meet threshold of ${threshold}%.`);
reporter.diagnostic(nesting, loc, `Error: ${NumberPrototypeToFixed(actual, 2)}% ${name} coverage does not meet threshold of ${threshold}%.`, 'error');
}
}

Expand Down
3 changes: 2 additions & 1 deletion lib/internal/test_runner/tests_stream.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,11 +114,12 @@ class TestsStream extends Readable {
});
}

diagnostic(nesting, loc, message) {
diagnostic(nesting, loc, message, level = 'info') {
this[kEmitMessage]('test:diagnostic', {
__proto__: null,
nesting,
message,
level,
...loc,
});
}
Expand Down
19 changes: 19 additions & 0 deletions test/parallel/test-runner-coverage-thresholds.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,25 @@ for (const coverage of coverages) {
assert(!findCoverageFileForPid(result.pid));
});

test(`test failing ${coverage.flag} with red color`, () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hey @hpatel292-seneca, it seems this test is failing in the CI.
Could you please take a look at it?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @pmarchini, I tried reading logs on failed tests and it seems like this regex
image
is not matching with the output. But as we are not logging anything I am not sure what we are getting output here.

But this test is running as expected and passing locally and in GitHub Action CI.
Github Action run: https://github.com/nodejs/node/actions/runs/12320594158/job/34394971556
Below is the SS of the locally run test
image

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://ci.nodejs.org/job/node-test-commit-osx/nodes=osx11-x64/62632/testReport/junit/(root)/parallel/test_runner_coverage_thresholds/ <-- here one CI failure

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I saw this CI failure but I am not sure what is causing the failure as this test is running as expected on my local machine. I tried reading logs on CI but I can't find anything. Is there any way I can debug those CI runs??

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @hpatel292-seneca, if you follow the link I've shared, you will find the description of the error.
It seems that the expected red error does not match any part of the output.
I've restarted many times, and the error systematically appears across different OS.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @pmarchini, I got your point but the test is running as expected with red color on my machine.
For example: I am printing output in the test like this

  test(`test failing ${coverage.flag} with red color`, () => {
    const result = spawnSync(process.execPath, [
      '--test',
      '--experimental-test-coverage',
      `${coverage.flag}=99`,
      '--test-reporter', 'spec',
      fixture,
    ], {
      env: { ...process.env, FORCE_COLOR: '3' },
    });

    const stdout = result.stdout.toString();
    console.log(stdout);  \\ Here I am printing output
    // eslint-disable-next-line no-control-regex
    const redColorRegex = /\u001b\[31mℹ Error: \d{2}\.\d{2}% \w+ coverage does not meet threshold of 99%/;
    assert.match(stdout, redColorRegex, 'Expected red color code not found in diagnostic message');
    assert.strictEqual(result.status, 1);
    assert(!findCoverageFileForPid(result.pid));
  });

And this is the output I am getting:
image

And here is the logs of failed pipeline run

actual: 'invalid tap output
✔ a test (3.3287ms)

ℹ tests 1
ℹ suites 0
ℹ pass 1
ℹ fail 0
ℹ cancelled 0
ℹ skipped 0
ℹ todo 0
ℹ duration_ms 304.826895',

expected: 
/\u001b[31mℹ Error: \d{2}\.\d{2}% \w+ coverage does not meet threshold of 99%/,

So basically, it is not even printing the coverage error which I think it shoud print if I am using this flags --experimental-test-coverage, ${coverage.flag}=99, and --test-reporter', 'spec which I am using in the test.

Do you think it is a environment or configuration issue??

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would say yes but in the CI it's also failing under Windows OS

const result = spawnSync(process.execPath, [
'--test',
'--experimental-test-coverage',
`${coverage.flag}=99`,
'--test-reporter', 'spec',
fixture,
], {
env: { ...process.env, FORCE_COLOR: '3' },
});

const stdout = result.stdout.toString();
// eslint-disable-next-line no-control-regex
const redColorRegex = /\u001b\[31mℹ Error: \d{2}\.\d{2}% \w+ coverage does not meet threshold of 99%/;
assert.match(stdout, redColorRegex, 'Expected red color code not found in diagnostic message');
assert.strictEqual(result.status, 1);
assert(!findCoverageFileForPid(result.pid));
});

test(`test failing ${coverage.flag}`, () => {
const result = spawnSync(process.execPath, [
'--test',
Expand Down
Loading