nodejs
test_runner: add level-based diagnostic handling for reporter
This fixes #55922
Change summary
Updated the reporter.diagnostic to accept level parameter like this
diagnostic(nesting, loc, message, level = 'info') {
this[kEmitMessage]('test:diagnostic', {
__proto__: null,
nesting,
message,
level,
...loc,
});
}
Then I updated #handleEvent like this
#handleEvent({ type, data }) {
switch (type) {
case 'test:fail':
if (data.details?.error?.failureType !== kSubtestsFailed) {
ArrayPrototypePush(this.#failedTests, data);
}
return this.#handleTestReportEvent(type, data);
case 'test:pass':
return this.#handleTestReportEvent(type, data);
case 'test:start':
ArrayPrototypeUnshift(this.#stack, { __proto__: null, data, type });
break;
case 'test:stderr':
case 'test:stdout':
return data.message;
case 'test:diagnostic': // Here I added logic
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
);
}
}
And I am Updated reporterColorMap like this
const reporterColorMap = {
__proto__: null,
get 'test:fail'() {
return colors.red;
},
get 'test:pass'() {
return colors.green;
},
get 'test:diagnostic'() {
return colors.blue;
},
get info() { // Here I added logic
return colors.blue;
},
get debug() {
return colors.gray;
},
get warn() {
return colors.yellow;
},
get error() {
return colors.red;
},
};
and color already contain logic for this colors
I also set the reporter.diagnostic call from test.js like this (level="Error")
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}%.`,
'error' // Level is set to error for red color
);
}
Here is Demo output:
Hi @pmarchini, Please review and let me know if you want me to change anything.
Hey @hpatel292-seneca, I won't be able to review until Monday. In the meantime, I suggest you take a look at the test runner output tests; we definitely need to add tests there.
I've also requested other reviews in the meantime.
Codecov Report
Attention: Patch coverage is 81.25000% with 3 lines in your changes missing coverage. Please review.
Project coverage is 88.54%. Comparing base (
e92499c) to head (c06e9e1). Report is 1578 commits behind head on main.
| Files with missing lines | Patch % | Lines |
|---|---|---|
| lib/internal/test_runner/reporter/utils.js | 77.77% | 2 Missing :warning: |
| lib/internal/test_runner/test.js | 0.00% | 1 Missing :warning: |
Additional details and impacted files
@@ Coverage Diff @@
## main #55964 +/- ##
==========================================
+ Coverage 87.99% 88.54% +0.54%
==========================================
Files 653 657 +4
Lines 188091 190215 +2124
Branches 35941 36529 +588
==========================================
+ Hits 165516 168431 +2915
+ Misses 15751 14976 -775
+ Partials 6824 6808 -16
| Files with missing lines | Coverage Δ | |
|---|---|---|
| lib/internal/test_runner/reporter/spec.js | 96.26% <100.00%> (+0.07%) |
:arrow_up: |
| lib/internal/test_runner/tests_stream.js | 91.66% <100.00%> (+0.04%) |
:arrow_up: |
| lib/internal/test_runner/test.js | 96.95% <0.00%> (-0.02%) |
:arrow_down: |
| lib/internal/test_runner/reporter/utils.js | 95.00% <77.77%> (-1.71%) |
:arrow_down: |
:rocket: New features to boost your workflow:
- :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
- :package: JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
![codecov[bot] avatar](https://avatars.githubusercontent.com/in/254?v=4 "Published by a pub.dev verified publisher"){kind=small}
Hi @pmarchini @cjihrig, The PR is ready for review.
Hi, @pmarchini @cjihrig I will add one more commit for fixing the lint error.
Fix:
I must add // eslint-disable-next-line no-control-regex for that line as I need that character for \u001b[31m for checking the response if red color.
It is also used at other places like
https://github.com/nodejs/node/blob/585f7bc9528f663afed3ebd4907a54d7d7bf9ada/test/fixtures/test-runner/output/spec_reporter_cli.js#L9
Should I go ahead and send commit?
Hi @pmarchini @cjihrig, The PR is ready for review.
Hi, @pmarchini @cjihrig I will add one more commit for fixing the lint error. Fix: I must add
// eslint-disable-next-line no-control-regexfor that line as I need that character for\u001b[31mfor checking the response if red color. It is also used at other places like https://github.com/nodejs/node/blob/585f7bc9528f663afed3ebd4907a54d7d7bf9ada/test/fixtures/test-runner/output/spec_reporter_cli.js#L9Should I go ahead and send commit?
Sure 🚀
Hi @pmarchini @cjihrig, The PR is ready for review.
Hi, @pmarchini @cjihrig I will add one more commit for fixing the lint error. Fix: I must add
// eslint-disable-next-line no-control-regexfor that line as I need that character for\u001b[31mfor checking the response if red color. It is also used at other places like https://github.com/nodejs/node/blob/585f7bc9528f663afed3ebd4907a54d7d7bf9ada/test/fixtures/test-runner/output/spec_reporter_cli.js#L9Should I go ahead and send commit?
Sure 🚀
@pmarchini I did. Thanks
Hi @pmarchini @cjihrig, PR is ready for review and all CI passed. Please let me know if anything needs to be changed. Thanks.
Is it possible to add or update a test that verifies the data added to the event. I'm not sure that validating red text coming from the spec reporter is thorough enough.
Is it possible to add or update a test that verifies the data added to the event. I'm not sure that validating red text coming from the spec reporter is thorough enough.
How I can test that? Could you please elaborate??
The run() API returns the stream that emits these events. You could call run() and perform some assertions on the diagnostic events coming out of the stream.
Hi @cjihrig, Based on the this tests: https://github.com/nodejs/node/blob/96cd2a6ec32917f430444904ab13239564480b1a/test/parallel/test-runner-run.mjs#L28 I figured out that I should test like this
it('should emit diagnostic events with correct level and message', async () => {
const diagnosticEvents = [];
const stream = run({
files: ['test-file.js'], // I am confused here!!
reporter: 'spec',
});
stream.on('test:diagnostic', (event) => {
diagnosticEvents.push(event.data);
});
for await (const _ of stream);
assert(diagnosticEvents.length > 0, 'No diagnostic events were emitted');
const errorEvent = diagnosticEvents.find((e) => e.level === 'error');
assert(errorEvent, 'No error-level diagnostic events found');
assert.match(
errorEvent.message,
"Error Message",
'Diagnostic message format mismatch'
);
});
I am not sure how to write that test-file.js which I want to run in this test.
I am not sure how to write that test-file.js which I want to run in this test.
I think you'd want to do something like this. That will let you run the same test fixture that you're running in your test from test-runner-coverage-thresholds.js.
I am not sure how to write that test-file.js which I want to run in this test.
I think you'd want to do something like this. That will let you run the same test fixture that you're running in your test from
test-runner-coverage-thresholds.js.
So I tried that like this
it('should emit diagnostic events with correct level and message', async () => {
const diagnosticEvents = [];
// Run the test suite and capture events
const stream = run({
files: [join(testFixtures, 'coverage.js')],
reporter: 'spec',
});
stream.on('test:diagnostic', (event) => {
diagnosticEvents.push(event);
});
for await (const _ of stream);
console.log(diagnosticEvents) // here I am printing the events
// Assertions on diagnostic events
assert(diagnosticEvents.length > 0, 'No diagnostic events were emitted');
const errorEvent = diagnosticEvents.find((e) => e.level === 'error');
});
So I think I am able to capture test:diagnostic events and as you can see from above test I am also printing the events and here how it looks
[
[Object: null prototype] {
nesting: 0,
message: 'tests 1',
level: 'info'
},
[Object: null prototype] {
nesting: 0,
message: 'suites 0',
level: 'info'
},
[Object: null prototype] {
nesting: 0,
message: 'pass 1',
level: 'info'
},
[Object: null prototype] {
nesting: 0,
message: 'fail 0',
level: 'info'
},
[Object: null prototype] {
nesting: 0,
message: 'cancelled 0',
level: 'info'
},
[Object: null prototype] {
nesting: 0,
message: 'skipped 0',
level: 'info'
},
[Object: null prototype] {
nesting: 0,
message: 'todo 0',
level: 'info'
},
[Object: null prototype] {
nesting: 0,
message: 'duration_ms 1277.1178',
level: 'info'
}
]
As you can see from the output diagnostic events contain the level parameter which is good but it's not emitting level: error may be because I am passing the flags like I did in my test in test-runner-coverage-thresholds.js. This flags:
const result = spawnSync(process.execPath, [
'--test',
'--experimental-test-coverage',
`${coverage.flag}=99`,
'--test-reporter', 'spec',
fixture,
], {
env: { ...process.env, FORCE_COLOR: '3' },
});
```
I tried adding flags in run api call but it's not working.
I think that's fine. We don't need to make sure the level is set to 'error'. I think 'info' is good enough. We mostly just want to test the event to make sure that the red text coming from the reporter is not due to something else.
Hi @cjihrig, I have added the test. Please review and let me know if any changes are required. Thanks
