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

Add a level parameter to test runner diagnostics #55922

Open
RedYetiDev opened this issue Nov 19, 2024 · 12 comments · May be fixed by #55964
Open

Add a level parameter to test runner diagnostics #55922

RedYetiDev opened this issue Nov 19, 2024 · 12 comments · May be fixed by #55964
Labels
feature request Issues that request new features to be added to Node.js. test_runner Issues and PRs related to the test runner subsystem.

Comments

@RedYetiDev
Copy link
Member

maybe we should add a level parameter in diagnostics (i.e debug/info/warn/error) so reporters can implement coloring or other things

Originally posted by @MoLow in #55911 (comment)

@RedYetiDev RedYetiDev added feature request Issues that request new features to be added to Node.js. test_runner Issues and PRs related to the test runner subsystem. labels Nov 19, 2024
@github-project-automation github-project-automation bot moved this to Awaiting Triage in Node.js feature requests Nov 19, 2024
@hpatel292-seneca
Copy link

Hi @RedYetiDev @MoLow, I would love to contribute to this.

@RedYetiDev
Copy link
Member Author

Feel free to open a PR

@hpatel292-seneca
Copy link

Hi @RedYetiDev, Here is the Summary of Planned changes, please take a look and give feedback

Summary of Planned Changes

To address the problem of coverage threshold errors not being visually distinct, I propose the following enhancements:

  1. Introduce a level Parameter in Diagnostics:

    • Add a level parameter (e.g., debug, info, warn, error) to diagnostic messages.
    • This parameter will categorize the severity of the message and allow reporters to handle formatting based on the message type.
  2. Update Diagnostic Emission:

    • Modify the diagnostic function to include the level parameter along with the message and location.
    • Example:
      reporter.diagnostic(nesting, loc, {
        message: `Error: ${actual}% ${name} coverage does not meet threshold of ${threshold}%.`,
        level: 'error'
      });
  3. Delegate Formatting to Reporters:

    • Ensure reporters use the level parameter to apply appropriate formatting (e.g., red for errors, yellow for warnings).
    • This will keep presentation logic within reporters, maintaining the separation of concerns.

@pmarchini
Copy link
Member

pmarchini commented Nov 22, 2024

Hey @hpatel292-seneca, thanks for contributing! 😊
Regarding the planned changes: IMHO, it looks okay🚀 I think you could start working on the PR!

Only one thing: atm, the message is a string parameter.
Changing it from a string to an object could require more work and attention, potentially causing breaking changes
I would suggest considering the idea of simply adding a new parameter to the function instead.

Just a trivial reminder: please ensure tests are added to cover this feature.

@hpatel292-seneca
Copy link

Understood. Thanks @pmarchini

@hpatel292-seneca
Copy link

hpatel292-seneca commented Nov 22, 2024

Hi @pmarchini,

I just wanted to confirm one thing,
so I 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() {
    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

so my question is I will 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
            );
          }

right?

@pmarchini
Copy link
Member

Hey @hpatel292-seneca, I would suggest moving this discussion to a draft PR!

Also, please include at least some tests (even in the draft) to verify the behavior 🚀

@hpatel292-seneca
Copy link

hpatel292-seneca commented Nov 22, 2024

Hi @pmarchini, I am confused, I read Building.md but not sure what option is best for me to build Node. I have windows. Could you please let me know what is best option to build Node locally for Windows.

Thanks

@pmarchini
Copy link
Member

Hi @pmarchini, I am confused, I read Building.md but not sure what option is best for me to build Node. I have windows. Could you please let me know what is best option to build Node locally for Windows.

Thanks

I would suggest going with option 3: Boxstarter 🚀

@hpatel292-seneca
Copy link

@pmarchini How much time do you think the build will take?
Because my building has been running for 1 hour and 10 minutes and still hasn't been completed.

@pmarchini
Copy link
Member

@hpatel292-seneca it greatly depends on your machine.
On my Windows laptop it takes circa 25 minutes

@hpatel292-seneca
Copy link

@pmarchini I am using this machine
image

I will cancel the build for now and will try tomorrow.

hpatel292-seneca added a commit to hpatel292-seneca/node that referenced this issue Nov 22, 2024
Added a  parameter to  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
hpatel292-seneca added a commit to hpatel292-seneca/node that referenced this issue Nov 22, 2024
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
hpatel292-seneca added a commit to hpatel292-seneca/node that referenced this issue Nov 22, 2024
Enhanced  to include colors for the following
diagnostic levels:
 : blue
 : gray
 : yellow
 : red

Ensures consistency in color-coding across the reporter.

Refs: nodejs#55922
hpatel292-seneca added a commit to hpatel292-seneca/node that referenced this issue Nov 22, 2024
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
hpatel292-seneca added a commit to hpatel292-seneca/node that referenced this issue Nov 23, 2024
Added a  parameter to  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
hpatel292-seneca added a commit to hpatel292-seneca/node that referenced this issue Nov 23, 2024
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
hpatel292-seneca added a commit to hpatel292-seneca/node that referenced this issue Nov 23, 2024
Enhanced  to include colors for the following
diagnostic levels:
 : blue
 : gray
 : yellow
 : red

Ensures consistency in color-coding across the reporter.

Refs: nodejs#55922
hpatel292-seneca added a commit to hpatel292-seneca/node that referenced this issue Nov 23, 2024
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
hpatel292-seneca added a commit to hpatel292-seneca/node that referenced this issue Nov 24, 2024
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
hpatel292-seneca added a commit to hpatel292-seneca/node that referenced this issue Nov 24, 2024
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
hpatel292-seneca added a commit to hpatel292-seneca/node that referenced this issue Nov 24, 2024
Enhanced  to include colors for the following
diagnostic levels:
 : blue - info
 : yellow - warn
 : red - error

Refs: nodejs#55922
hpatel292-seneca added a commit to hpatel292-seneca/node that referenced this issue Nov 24, 2024
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Issues that request new features to be added to Node.js. test_runner Issues and PRs related to the test runner subsystem.
Projects
Status: Awaiting Triage
3 participants