From 7dda5392cc1ef60e55689056d5ed6c05d218cc9f Mon Sep 17 00:00:00 2001 From: Harshil Patel <26harshilpatel11@gmail.com> Date: Fri, 22 Nov 2024 21:58:23 -0500 Subject: [PATCH 01/11] test_runner: add level parameter to reporter.diagnostic Added a parameter to allow severity-based formatting for diagnostic messages. Defaults to 'info'. This update enables better control over message presentation (e.g., coloring) based on severity levels such as 'info', 'warn', and 'error'. Refs: https://github.com/nodejs/node/issues/55922 --- lib/internal/test_runner/tests_stream.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/internal/test_runner/tests_stream.js b/lib/internal/test_runner/tests_stream.js index ecbc407e01f318..2ae1ce923982ea 100644 --- a/lib/internal/test_runner/tests_stream.js +++ b/lib/internal/test_runner/tests_stream.js @@ -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, }); } From c6188743fccd9ef629f1ba58935c61323b0a6021 Mon Sep 17 00:00:00 2001 From: Harshil Patel <26harshilpatel11@gmail.com> Date: Fri, 22 Nov 2024 22:01:05 -0500 Subject: [PATCH 02/11] test_runner: enhance #handleEvent to handle levels Updated to process the parameter for events. Messages are now formatted with colors based on the (e.g., 'info', 'warn', 'error'). This change ensures diagnostic messages are visually distinct, improving clarity and reducing debugging effort during test runs. Refs: https://github.com/nodejs/node/issues/55922 --- lib/internal/test_runner/reporter/spec.js | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/lib/internal/test_runner/reporter/spec.js b/lib/internal/test_runner/reporter/spec.js index 2092d22e3fe77f..e45351a79f560c 100644 --- a/lib/internal/test_runner/reporter/spec.js +++ b/lib/internal/test_runner/reporter/spec.js @@ -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); From 2ac841c625a57b9c677388e7fb2bb71a403a9df3 Mon Sep 17 00:00:00 2001 From: Harshil Patel <26harshilpatel11@gmail.com> Date: Fri, 22 Nov 2024 22:02:28 -0500 Subject: [PATCH 03/11] test_runner: extend reporterColorMap for levels Enhanced to include colors for the following diagnostic levels: : blue - info : yellow - warn : red - error Refs: https://github.com/nodejs/node/issues/55922 --- lib/internal/test_runner/reporter/utils.js | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/lib/internal/test_runner/reporter/utils.js b/lib/internal/test_runner/reporter/utils.js index 9b6cc96a185a9a..c3fe8d116347aa 100644 --- a/lib/internal/test_runner/reporter/utils.js +++ b/lib/internal/test_runner/reporter/utils.js @@ -37,6 +37,18 @@ const reporterColorMap = { get 'test:diagnostic'() { return colors.blue; }, + get 'info'() { + return colors.blue; + }, + get 'debug'() { + return colors.gray; + }, + get 'warn'() { + return colors.yellow; + }, + get 'error'() { + return colors.red; + }, }; function indent(nesting) { From 1ca0d45f06f49e1af392aae7c0024fa3bed2bf46 Mon Sep 17 00:00:00 2001 From: Harshil Patel <26harshilpatel11@gmail.com> Date: Fri, 22 Nov 2024 22:03:30 -0500 Subject: [PATCH 04/11] test_runner: set level for coverage threshold Updated coverage threshold checks in to use the parameter when calling. Errors now use the 'error' level for red-colored formatting. This ensures coverage errors are highlighted effectively in the output. Fixes: https://github.com/nodejs/node/issues/55922 --- lib/internal/test_runner/test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/internal/test_runner/test.js b/lib/internal/test_runner/test.js index 34e1900979960f..d24e19c358a448 100644 --- a/lib/internal/test_runner/test.js +++ b/lib/internal/test_runner/test.js @@ -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'); } } From 038b0f8eca0b395944deb44e83d7d186ad5f35a1 Mon Sep 17 00:00:00 2001 From: Harshil Patel <26harshilpatel11@gmail.com> Date: Sat, 23 Nov 2024 19:14:46 -0500 Subject: [PATCH 05/11] test_runner: remove debug from reporterColorMap implemented requested change by removing debug from reporterColorMap Refs: https://github.com/nodejs/node/pull/55964#pullrequestreview-2456483116 --- lib/internal/test_runner/reporter/utils.js | 3 --- 1 file changed, 3 deletions(-) diff --git a/lib/internal/test_runner/reporter/utils.js b/lib/internal/test_runner/reporter/utils.js index c3fe8d116347aa..8bb881a0ca934f 100644 --- a/lib/internal/test_runner/reporter/utils.js +++ b/lib/internal/test_runner/reporter/utils.js @@ -40,9 +40,6 @@ const reporterColorMap = { get 'info'() { return colors.blue; }, - get 'debug'() { - return colors.gray; - }, get 'warn'() { return colors.yellow; }, From b490d9d52c1f83415faa4e50681165d2f736a7c5 Mon Sep 17 00:00:00 2001 From: Harshil Patel <26harshilpatel11@gmail.com> Date: Mon, 25 Nov 2024 22:52:36 -0500 Subject: [PATCH 06/11] doc: update 'test:diagnostic' event to include level parameter updated the documentation for the 'test:diagnostic' event to include the new level parameter. clarified its purpose, default value, and possible severity levels ('info', 'warn', 'error'). Fixes: https://github.com/nodejs/node/issues/55922 --- doc/api/test.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/doc/api/test.md b/doc/api/test.md index f88d3542f954bf..4dcbc2e8022fc9 100644 --- a/doc/api/test.md +++ b/doc/api/test.md @@ -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, which determines its output color in the reporter. + 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 From 3e4d5e6884ffa2c5ad264c97bb5f5a1aebffaeff Mon Sep 17 00:00:00 2001 From: Harshil Patel <26harshilpatel11@gmail.com> Date: Wed, 27 Nov 2024 12:08:23 -0500 Subject: [PATCH 07/11] test: add test for spec reporter red color output Add a test in to verify that the diagnostic error messages about unmet coverage thresholds are displayed in red when using the spec reporter. Fixes: https://github.com/nodejs/node/issues/55922 --- .../test-runner-coverage-thresholds.js | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/test/parallel/test-runner-coverage-thresholds.js b/test/parallel/test-runner-coverage-thresholds.js index c1b64cb06a83ff..cf585c4cee0b30 100644 --- a/test/parallel/test-runner-coverage-thresholds.js +++ b/test/parallel/test-runner-coverage-thresholds.js @@ -88,6 +88,24 @@ for (const coverage of coverages) { assert(!findCoverageFileForPid(result.pid)); }); + 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(); + 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', From 4048621e264b9d9148b303d326f6209dbc9b35d9 Mon Sep 17 00:00:00 2001 From: Harshil Patel <26harshilpatel11@gmail.com> Date: Wed, 27 Nov 2024 14:10:41 -0500 Subject: [PATCH 08/11] test: disable no-control-regex for color regex Added eslint-disable comment to bypass no-control-regex. This allows testing ANSI escape sequences for red color in error messages without triggering lint errors. Fixes: https://github.com/nodejs/node/issues/55922 --- test/parallel/test-runner-coverage-thresholds.js | 1 + 1 file changed, 1 insertion(+) diff --git a/test/parallel/test-runner-coverage-thresholds.js b/test/parallel/test-runner-coverage-thresholds.js index cf585c4cee0b30..38b329054fb681 100644 --- a/test/parallel/test-runner-coverage-thresholds.js +++ b/test/parallel/test-runner-coverage-thresholds.js @@ -100,6 +100,7 @@ for (const coverage of coverages) { }); 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); From 33b961d93a5cd689f8508069b5c543da0570e9fe Mon Sep 17 00:00:00 2001 From: Harshil Patel <26harshilpatel11@gmail.com> Date: Thu, 28 Nov 2024 09:51:28 -0500 Subject: [PATCH 09/11] doc: clarify diagnostic behavior in API docs Updated the description of the parameter to note that color output is specific to the spec reporter. This helps users understand its behavior and create custom reporters with accurate expectations. Fixes: https://github.com/nodejs/node/issues/55922 --- doc/api/test.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/api/test.md b/doc/api/test.md index 4dcbc2e8022fc9..1f031807dd830b 100644 --- a/doc/api/test.md +++ b/doc/api/test.md @@ -2944,7 +2944,7 @@ 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, which determines its output color in the reporter. + * `level` {string} The severity level of the diagnostic message. Possible values are: * `'info'`: Informational messages. * `'warn'`: Warnings. From b1c80331fec4d6bde034cf6eed95e2e928015585 Mon Sep 17 00:00:00 2001 From: Harshil Patel <26harshilpatel11@gmail.com> Date: Fri, 13 Dec 2024 11:29:35 -0500 Subject: [PATCH 10/11] test: validate diagnostic events in test runner add a test to ensure that diagnostic events emitted by the test runner contain level parameter. Refs: https://github.com/nodejs/node/pull/55964 --- test/parallel/test-runner-run.mjs | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/test/parallel/test-runner-run.mjs b/test/parallel/test-runner-run.mjs index 9f13e1a1e56420..309f10c35d170d 100644 --- a/test/parallel/test-runner-run.mjs +++ b/test/parallel/test-runner-run.mjs @@ -33,6 +33,24 @@ describe('require(\'node:test\').run', { concurrency: true }, () => { for await (const _ of stream); }); + it('should emit diagnostic events with level parameter', async () => { + const diagnosticEvents = []; + + const stream = run({ + files: [join(testFixtures, 'coverage.js')], + reporter: 'spec', + }); + + stream.on('test:diagnostic', (event) => { + diagnosticEvents.push(event); + }); + + for await (const _ of stream); + assert(diagnosticEvents.length > 0, 'No diagnostic events were emitted'); + const infoEvent = diagnosticEvents.find((e) => e.level === 'info'); + assert(infoEvent, 'No diagnostic events with level "info" were emitted'); + }); + const argPrintingFile = join(testFixtures, 'print-arguments.js'); it('should allow custom arguments via execArgv', async () => { const result = await run({ files: [argPrintingFile], execArgv: ['-p', '"Printed"'] }).compose(spec).toArray(); From c06e9e180d476f59398fc8f326be77773fd16a40 Mon Sep 17 00:00:00 2001 From: Harshil Patel <26harshilpatel11@gmail.com> Date: Fri, 13 Dec 2024 12:55:12 -0500 Subject: [PATCH 11/11] test: Fix Lint error Added eslint-disable-next-line to bypass no-unused-vars check ref: https://github.com/nodejs/node/pull/55964 --- test/parallel/test-runner-run.mjs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/parallel/test-runner-run.mjs b/test/parallel/test-runner-run.mjs index 309f10c35d170d..6521fec00b96dd 100644 --- a/test/parallel/test-runner-run.mjs +++ b/test/parallel/test-runner-run.mjs @@ -44,7 +44,7 @@ describe('require(\'node:test\').run', { concurrency: true }, () => { stream.on('test:diagnostic', (event) => { diagnosticEvents.push(event); }); - + // eslint-disable-next-line no-unused-vars for await (const _ of stream); assert(diagnosticEvents.length > 0, 'No diagnostic events were emitted'); const infoEvent = diagnosticEvents.find((e) => e.level === 'info');