Add a level parameter to test runner diagnostics

[avivkeller]

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)

[hpatel292-seneca]

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

[avivkeller]

Feel free to open a PR

[hpatel292-seneca]

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]

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]

Understood. Thanks @pmarchini

[hpatel292-seneca]

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]

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]

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]

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]

@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]

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

[hpatel292-seneca]

@pmarchini I am using this machine

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