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.

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 5 commits into
base: main
Choose a base branch
from

Conversation

hpatel292-seneca
Copy link

@hpatel292-seneca hpatel292-seneca commented Nov 23, 2024

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:
image

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/test_runner

@nodejs-github-bot 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 hpatel292-seneca marked this pull request as ready for review November 23, 2024 03:05
@hpatel292-seneca
Copy link
Author

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

@hpatel292-seneca hpatel292-seneca marked this pull request as draft November 23, 2024 03:33
@pmarchini
Copy link
Member

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.

Copy link

codecov bot commented Nov 23, 2024

Codecov Report

Attention: Patch coverage is 78.94737% with 4 lines in your changes missing coverage. Please review.

Project coverage is 87.99%. Comparing base (f270462) to head (188f357).
Report is 20 commits behind head on main.

Files with missing lines Patch % Lines
lib/internal/test_runner/reporter/utils.js 75.00% 3 Missing ⚠️
lib/internal/test_runner/test.js 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #55964      +/-   ##
==========================================
- Coverage   88.49%   87.99%   -0.51%     
==========================================
  Files         653      653              
  Lines      187728   187867     +139     
  Branches    36182    35889     -293     
==========================================
- Hits       166136   165314     -822     
- Misses      14814    15718     +904     
- Partials     6778     6835      +57     
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.97% <0.00%> (ø)
lib/internal/test_runner/reporter/utils.js 94.17% <75.00%> (-2.53%) ⬇️

... and 90 files with indirect coverage changes

---- 🚨 Try these New Features:

Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

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

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

Comment on lines 43 to 45
get 'debug'() {
return colors.gray;
},
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Author

@hpatel292-seneca hpatel292-seneca Nov 23, 2024

Choose a reason for hiding this comment

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

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??

Copy link
Contributor

Choose a reason for hiding this comment

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

test_runner: add level to diagnostics

Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

Hi @cjihrig, I pushed the change you requested and the commit history to follow the commit guidelines.

Copy link
Author

Choose a reason for hiding this comment

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

And for docs and tests could you please give more details about it? Like where the doc goes and the same for the test

Copy link
Contributor

Choose a reason for hiding this comment

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

The test runner docs are located in doc/api/test.md. The tests are located in test/parallel/test-runner-*.

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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-ci PRs that need a full CI run. test_runner Issues and PRs related to the test runner subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a level parameter to test runner diagnostics
5 participants