test_runner: top-level diagnostics not ommited when running with --test #46441

pulkit-30 · 2023-01-31T10:35:11Z

Should not omit top-level diagnostics when running test with --test flag.

Changes

added a logic #checkNestedComment() to filter top-level diagnostics only in cases they are redundant and show them
test:

test('should not omit top-level diagnostics', () => setImmediate(() => done()));

output:

TAP version 13
# Subtest: *
    # Subtest: should not omit top-level diagnostics
    ok 1 - should not omit top-level diagnostics
      ---
      duration_ms: 2.072958
      ...
    1..1
    # Warning: Test "should not omit top-level diagnostics" generated asynchronous activity after the test ended. This activity created the error "ReferenceError: done is not defined" and would have caused the test to fail, but instead triggered an uncaughtException event.
not ok 1 - *
  ---
  duration_ms: 57.882541
  failureType: 'subtestsFailed'
  exitCode: 1
  error: 'test failed'
  code: 'ERR_TEST_FAILURE'
  ...
1..1
# tests 1
# pass 0
# fail 1
# cancelled 0
# skipped 0
# todo 0
# duration_ms 59.641875

I am not sure about test failing
when running test without --test flag, test output is:

TAP version 13
# Subtest: should not omit top-level diagnostics
ok 1 - should not omit top-level diagnostics
  ---
  duration_ms: 2.544125
  ...
1..1
# Warning: Test "should not omit top-level diagnostics" generated asynchronous activity after the test ended. This activity created the error "ReferenceError: done is not defined" and would have caused the test to fail, but instead triggered an uncaughtException event.
# tests 1
# pass 1
# fail 0
# cancelled 0
# skipped 0
# todo 0
# duration_ms 6.197042

nodejs-github-bot · 2023-01-31T10:35:15Z

Review requested:

@nodejs/test_runner

lib/internal/test_runner/runner.js

test/message/test_runner_output.js

lib/internal/test_runner/runner.js

cjihrig · 2023-01-31T14:58:39Z

lib/internal/test_runner/runner.js

@@ -16,6 +16,7 @@ const {
  SafeMap,
  SafeSet,
  StringPrototypeStartsWith,
+  StringPrototypeSplit,


Please move this up a line (sorted).

cjihrig · 2023-01-31T14:58:39Z

lib/internal/test_runner/runner.js

@@ -51,6 +52,7 @@ const {

 const kFilterArgs = ['--test', '--experimental-test-coverage', '--watch'];
 const kFilterArgValues = ['--test-reporter', '--test-reporter-destination'];
+const diagnosticsFilterArgs = ['tests', 'pass', 'fail', 'cancelled', 'skipped', 'todo', 'duration_ms'];


Suggested change

const diagnosticsFilterArgs = ['tests', 'pass', 'fail', 'cancelled', 'skipped', 'todo', 'duration_ms'];

const kDiagnosticsFilterArgs = ['tests', 'pass', 'fail', 'cancelled', 'skipped', 'todo', 'duration_ms'];

ljharb · 2023-01-31T15:13:20Z

lib/internal/test_runner/runner.js

@@ -130,6 +132,10 @@ function getRunArgs({ path, inspectPort }) {

 class FileTest extends Test {
  #buffer = [];
+  #checkNestedComment(node) {
+    const commentArgs = StringPrototypeSplit(node.comment, ' ');


can node.comment ever be a non-string?

I don't think there would be any case where comment is not type string.

can that be asserted here, since passing an object into StringPrototypeSplit isn't safe?

FWIW StringPrototypeSplit is always "unsafe", there's no way of using it without either accepting prototype pollution or suffer very bad perf. Ideally we would not use it at all, but sometimes there is just no other way around.
In this case though, it seems it wouldn't be very difficult to use something else, since we don't even use the split values but are just counting the spaces.

const firstSpaceIndex = StringPrototypeIndexOf(comment.node, ' '); const secondSpaceIndex = StringPrototypeIndexOf(comment.node, ' ', firstSpaceIndex); return secondSpaceIndex === -1 && ArrayPrototypeIncludes(kDiagnosticsFilterArgs, StringPrototypeSlice(comment.node, 0, firstSpaceIndex));

Thanks for review
committing required changes 🚀

aduh95 · 2023-01-31T18:30:30Z

lib/internal/test_runner/runner.js

@@ -130,6 +133,12 @@ function getRunArgs({ path, inspectPort }) {

 class FileTest extends Test {
  #buffer = [];
+  #checkNestedComment({ comment }) {
+    const firstSpaceIndex = StringPrototypeIndexOf(comment, ' ');


I forgot about the case where node.comment does not contain any spaces.

Suggested change

const firstSpaceIndex = StringPrototypeIndexOf(comment, ' ');

const firstSpaceIndex = StringPrototypeIndexOf(comment, ' ');

if (firstSpaceIndex === -1) return false;

test_runner: top-level diagnostics not ommited when running with --test

de70efa

nodejs-github-bot added dont-land-on-v14.x PRs that should not land on the v14.x-staging branch and should not be released in v14.x. needs-ci PRs that need a full CI run. test_runner labels Jan 31, 2023

MoLow reviewed Jan 31, 2023

View changes

lib/internal/test_runner/runner.js Outdated Show resolved Hide resolved

MoLow reviewed Jan 31, 2023

View changes

test/message/test_runner_output.js Outdated Show resolved Hide resolved

remove redundant test

bb83008

MoLow reviewed Jan 31, 2023

View changes

lib/internal/test_runner/runner.js Outdated Show resolved Hide resolved

used StringPrototypeSplit primordials and added more conditions

d3ab0fc

MoLow approved these changes Jan 31, 2023

View changes

tony-go approved these changes Jan 31, 2023

View changes

cjihrig reviewed Jan 31, 2023

View changes

move StringPrototypeSplit up a line and rename variable

754e8d3

ljharb reviewed Jan 31, 2023

View changes

used StringPrototypeIndexOf in place of StringPrototypeSplit primordials

7719a4b

MoLow approved these changes Jan 31, 2023

View changes

benjamingr approved these changes Jan 31, 2023

View changes

aduh95 reviewed Jan 31, 2023

View changes

test_runner: top-level diagnostics not ommited when running with --test #46441

test_runner: top-level diagnostics not ommited when running with --test #46441

pulkit-30 commented Jan 31, 2023 •

edited

nodejs-github-bot commented Jan 31, 2023

cjihrig Jan 31, 2023

cjihrig Jan 31, 2023

ljharb Jan 31, 2023

pulkit-30 Jan 31, 2023

ljharb Jan 31, 2023

aduh95 Jan 31, 2023 •

edited

pulkit-30 Jan 31, 2023

aduh95 Jan 31, 2023

	const diagnosticsFilterArgs = ['tests', 'pass', 'fail', 'cancelled', 'skipped', 'todo', 'duration_ms'];
	const kDiagnosticsFilterArgs = ['tests', 'pass', 'fail', 'cancelled', 'skipped', 'todo', 'duration_ms'];

	const firstSpaceIndex = StringPrototypeIndexOf(comment, ' ');
	const firstSpaceIndex = StringPrototypeIndexOf(comment, ' ');
	if (firstSpaceIndex === -1) return false;

test_runner: top-level diagnostics not ommited when running with --test #46441

Are you sure you want to change the base?

test_runner: top-level diagnostics not ommited when running with --test #46441

Conversation

pulkit-30 commented Jan 31, 2023 • edited

nodejs-github-bot commented Jan 31, 2023

cjihrig Jan 31, 2023

Choose a reason for hiding this comment

cjihrig Jan 31, 2023

Choose a reason for hiding this comment

ljharb Jan 31, 2023

Choose a reason for hiding this comment

pulkit-30 Jan 31, 2023

Choose a reason for hiding this comment

ljharb Jan 31, 2023

Choose a reason for hiding this comment

aduh95 Jan 31, 2023 • edited

Choose a reason for hiding this comment

pulkit-30 Jan 31, 2023

Choose a reason for hiding this comment

aduh95 Jan 31, 2023

Choose a reason for hiding this comment

pulkit-30 commented Jan 31, 2023 •

edited

aduh95 Jan 31, 2023 •

edited