Skip to content
Closed
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
test_runner: extend reporterColorMap for levels
Enhanced  to include colors for the following
diagnostic levels:
 : blue - info
 : yellow - warn
 : red - error

Refs: #55922
  • Loading branch information
hpatel292-seneca committed Nov 24, 2024
commit 2ac841c625a57b9c677388e7fb2bb71a403a9df3
12 changes: 12 additions & 0 deletions lib/internal/test_runner/reporter/utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,18 @@ const reporterColorMap = {
get 'test:diagnostic'() {
return colors.blue;
},
get 'info'() {
return colors.blue;
},
get 'debug'() {
return colors.gray;
},

Copy link
Copy Markdown
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.

@hpatel292-seneca hpatel292-seneca Nov 23, 2024

Copy link
Copy Markdown
Author

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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.

@hpatel292-seneca hpatel292-seneca Nov 26, 2024

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

@hpatel292-seneca hpatel292-seneca Nov 27, 2024

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

@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
image
and here is test case:
image

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.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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

get 'warn'() {
return colors.yellow;
},
get 'error'() {
return colors.red;
},
};

function indent(nesting) {
Expand Down