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

hpatel292-seneca · 2024-11-23T03:05:08Z

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:

nodejs-github-bot · 2024-11-23T03:05:14Z

Review requested:

@nodejs/test_runner

hpatel292-seneca · 2024-11-23T03:09:06Z

Hi @pmarchini, Please review and let me know if you want me to change anything.

pmarchini · 2024-11-23T03:36:31Z

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 · 2024-11-23T21:28:16Z

Codecov Report

Attention: Patch coverage is 81.25000% with 3 lines in your changes missing coverage. Please review.

Project coverage is 87.96%. Comparing base (e92499c) to head (33b961d).
Report is 37 commits behind head on main.

Files with missing lines	Patch %	Lines
lib/internal/test_runner/reporter/utils.js	77.77%	2 Missing ⚠️
lib/internal/test_runner/test.js	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #55964      +/-   ##
==========================================
- Coverage   87.99%   87.96%   -0.04%     
==========================================
  Files         653      656       +3     
  Lines      188091   188396     +305     
  Branches    35941    35975      +34     
==========================================
+ Hits       165516   165725     +209     
- Misses      15751    15832      +81     
- Partials     6824     6839      +15

Files with missing lines	Coverage Δ
lib/internal/test_runner/reporter/spec.js	`96.26% <100.00%> (+0.07%)`	⬆️
lib/internal/test_runner/tests_stream.js	`91.66% <100.00%> (+0.04%)`	⬆️
lib/internal/test_runner/test.js	`96.95% <0.00%> (-0.02%)`	⬇️
lib/internal/test_runner/reporter/utils.js	`95.00% <77.77%> (-1.71%)`	⬇️

... and 42 files with indirect coverage changes

cjihrig

Requesting changes since this already has an approval. This also needs docs and tests.

cjihrig · 2024-11-23T21:45:08Z

lib/internal/test_runner/reporter/utils.js

+  get 'debug'() {
+    return colors.gray;
+  },


Let's remove the debug level. The reporter stream is not a generic logger, and we have other ways (NODE_DEBUG) of adding debug output.

I will remove that. And I have a question so this CI https://github.com/nodejs/node/actions/runs/11983534043/job/33427085217?pr=55964 failed and it's for First commit message adheres to guidelines / lint-commit-message (pull_request) so should I change commit history??

test_runner: add level to diagnostics

So I am asking if I should change the commit history and force push.

Ah. Yes, please. You'll need to avoid merge commits too, as the tooling does not handle them well.

@pmarchini Ok so I am taking reference from this tests
https://github.com/nodejs/node/blob/main/test/fixtures/test-runner/output/coverage-width-80-color.mjs
https://github.com/nodejs/node/blob/main/test/fixtures/test-runner/output/coverage-width-80-color.snapshot

and based on that I wrote this test.
fixtures/test-runner/output/spec_reporter_diagnostic_levels.mjs

// Flags: --test-reporter=spec import { test } from 'node:test'; import { TestsStream } from '../../../lib/internal/test_runner/tests_stream.js'; process.env.FORCE_COLOR = '3'; const testsStream = new TestsStream(); test('Diagnostic Levels Color Output', () => { testsStream.diagnostic(1, {}, 'Info-level message', 'info'); testsStream.diagnostic(1, {}, 'Warning-level message', 'warn'); testsStream.diagnostic(1, {}, 'Error-level message', 'error'); });

and add this in test-runner-output.mjs

{ name: 'test-runner/output/spec_reporter_diagnostic_levels.mjs', transform: specTransform, tty: true, }

now I am running tools/test.py test/parallel/test-runner-output.mjs --snapshot but it's not creating snap for added test.
How I can create snaps??

I got this when I tried using test with release/node

I forgot you were working in a Windows environment. Considering this, I would suggest picking a different approach. You could add a test to node/test/parallel/test-runner-coverage-thresholds.js that forces the colors via the environment variable in the spawn, and check that the error message 'coverage does not meet...' is displayed in red.
Note: you need to set the reporter to spec.

This is also because test-runner-output.mjs is intended for 'e2e' testing of the test runner's output under specific circumstances.

@pmarchini @cjihrig Thank you so much for your help. It wouldn't be possible without your help.
I ran test cases and here is output
I printed raw output in test case and here is output

and here is test case:

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)); });

If it looks good to you I can commit it and you can review the whole PR.

Hi @pmarchini @cjihrig ,
Could we please land this PR if no change is required?

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: nodejs#55922

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: nodejs#55922

Enhanced to include colors for the following diagnostic levels: : blue - info : yellow - warn : red - error Refs: nodejs#55922

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: nodejs#55922

implemented requested change by removing debug from reporterColorMap Refs: nodejs#55964 (review)

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: nodejs#55922

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: nodejs#55922

hpatel292-seneca · 2024-11-27T17:16:47Z

Hi @pmarchini @cjihrig,
The PR is ready for review.

hpatel292-seneca · 2024-11-27T18:47:40Z

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

node/test/fixtures/test-runner/output/spec_reporter_cli.js

Line 9 in 585f7bc

// eslint-disable-next-line no-control-regex

Should I go ahead and send commit?

pmarchini · 2024-11-27T19:00:32Z

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

node/test/fixtures/test-runner/output/spec_reporter_cli.js

Line 9 in 585f7bc

// eslint-disable-next-line no-control-regex

Should I go ahead and send commit?

Sure 🚀

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: nodejs#55922

hpatel292-seneca · 2024-11-27T19:11:44Z

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

node/test/fixtures/test-runner/output/spec_reporter_cli.js

Line 9 in 585f7bc

// eslint-disable-next-line no-control-regex

Should I go ahead and send commit?

Sure 🚀

@pmarchini I did. Thanks

hpatel292-seneca · 2024-11-28T13:19:57Z

Hi @pmarchini @cjihrig,
PR is ready for review and all CI passed. Please let me know if anything needs to be changed.
Thanks.

pmarchini

Except the documentation: LGTM

pmarchini · 2024-11-28T14:01:44Z

doc/api/test.md

+  * `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.


Suggested change

* `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.

* `level` {string} The severity level of the diagnostic message.

Possible values are:

* `'info'`: Informational messages.

* `'warn'`: Warnings.

* `'error'`: Errors.

I'm adding this suggestion since the color is a "side-effect" and happens only in the spec reporter.
This information could help users while creating a new custom reporter

I will update and commit it. Thanks.

Hi @pmarchini, Docs is updated.

@pmarchini Please let me know if more changes is required.

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: nodejs#55922

nodejs-github-bot added needs-ci PRs that need a full CI run. test_runner Issues and PRs related to the test runner subsystem. labels Nov 23, 2024

hpatel292-seneca marked this pull request as ready for review November 23, 2024 03:05

hpatel292-seneca marked this pull request as draft November 23, 2024 03:33

pmarchini requested review from cjihrig and MoLow November 23, 2024 03:34

hpatel292-seneca marked this pull request as ready for review November 23, 2024 03:39

pmarchini mentioned this pull request Nov 23, 2024

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

Closed

jasnell approved these changes Nov 23, 2024

View reviewed changes

cjihrig requested changes Nov 23, 2024

View reviewed changes

hpatel292-seneca added 5 commits November 23, 2024 18:57

test_runner: extend reporterColorMap for levels

2ac841c

Enhanced to include colors for the following diagnostic levels: : blue - info : yellow - warn : red - error Refs: nodejs#55922

test_runner: set level for coverage threshold

1ca0d45

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: nodejs#55922

test_runner: remove debug from reporterColorMap

038b0f8

implemented requested change by removing debug from reporterColorMap Refs: nodejs#55964 (review)

hpatel292-seneca force-pushed the issue-55922-b branch from 188f357 to 038b0f8 Compare November 24, 2024 00:16

hpatel292-seneca requested a review from cjihrig November 24, 2024 04:04

doc: update 'test:diagnostic' event to include level parameter

b490d9d

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: nodejs#55922

hpatel292-seneca requested a review from pmarchini November 27, 2024 16:35

test: add test for spec reporter red color output

3e4d5e6

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: nodejs#55922

test: disable no-control-regex for color regex

4048621

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: nodejs#55922

pmarchini reviewed Nov 28, 2024

View reviewed changes

doc: clarify diagnostic behavior in API docs

33b961d

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: nodejs#55922

hpatel292-seneca requested a review from pmarchini November 28, 2024 14:52

pmarchini approved these changes Nov 29, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

hpatel292-seneca commented Nov 23, 2024 •

edited

Loading

nodejs-github-bot commented Nov 23, 2024

hpatel292-seneca commented Nov 23, 2024

pmarchini commented Nov 23, 2024

codecov bot commented Nov 23, 2024 •

edited

Loading

cjihrig left a comment

cjihrig Nov 23, 2024

hpatel292-seneca Nov 23, 2024 •

edited

Loading

cjihrig Nov 23, 2024

hpatel292-seneca Nov 23, 2024

cjihrig Nov 23, 2024

hpatel292-seneca Nov 26, 2024 •

edited

Loading

hpatel292-seneca Nov 27, 2024

pmarchini Nov 27, 2024

hpatel292-seneca Nov 27, 2024 •

edited

Loading

hpatel292-seneca Dec 1, 2024

hpatel292-seneca commented Nov 27, 2024

hpatel292-seneca commented Nov 27, 2024 •

edited

Loading

pmarchini commented Nov 27, 2024

hpatel292-seneca commented Nov 27, 2024

hpatel292-seneca commented Nov 28, 2024

pmarchini left a comment

pmarchini Nov 28, 2024

hpatel292-seneca Nov 28, 2024

hpatel292-seneca Nov 28, 2024

hpatel292-seneca Nov 29, 2024

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

Are you sure you want to change the base?

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

Conversation

hpatel292-seneca commented Nov 23, 2024 • edited Loading

Change summary

nodejs-github-bot commented Nov 23, 2024

hpatel292-seneca commented Nov 23, 2024

pmarchini commented Nov 23, 2024

codecov bot commented Nov 23, 2024 • edited Loading

Codecov Report

cjihrig left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hpatel292-seneca Nov 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hpatel292-seneca Nov 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hpatel292-seneca Nov 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hpatel292-seneca commented Nov 27, 2024

hpatel292-seneca commented Nov 27, 2024 • edited Loading

pmarchini commented Nov 27, 2024

hpatel292-seneca commented Nov 27, 2024

hpatel292-seneca commented Nov 28, 2024

pmarchini left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hpatel292-seneca commented Nov 23, 2024 •

edited

Loading

codecov bot commented Nov 23, 2024 •

edited

Loading

hpatel292-seneca Nov 23, 2024 •

edited

Loading

hpatel292-seneca Nov 26, 2024 •

edited

Loading

hpatel292-seneca Nov 27, 2024 •

edited

Loading

hpatel292-seneca commented Nov 27, 2024 •

edited

Loading