test_runner: fix spec skip detection by MoLow · Pull Request #47537 · nodejs/node

MoLow · 2023-04-13T10:18:10Z

nodejs-github-bot · 2023-04-13T10:18:16Z

Review requested:

@nodejs/test_runner

MoLow · 2023-04-13T10:24:00Z

@nodejs/test_runner even after this PR there are discrepancies when running with and without --test using the spec reporter, mainly in the area of generating errors correctly:

diff --git a/test/fixtures/test-runner/output/spec_reporter.js b/test/fixtures/test-runner/output/spec_reporter.js
index 6a7c2d655f..20ef4f140c 100644
--- a/test/fixtures/test-runner/output/spec_reporter.js
+++ b/test/fixtures/test-runner/output/spec_reporter.js
@@ -5,7 +5,7 @@ const fixtures = require('../../../common/fixtures');
 const spawn = require('node:child_process').spawn;
 
 const child = spawn(process.execPath,
-                    ['--no-warnings', '--test-reporter', 'spec', fixtures.path('test-runner/output/output.js')],
+                    ['--no-warnings',  '--test', '--test-reporter', 'spec', fixtures.path('test-runner/output/output.js')],
                     { stdio: 'pipe' });

diff --git a/test/fixtures/test-runner/output/spec_reporter.snapshot b/test/fixtures/test-runner/output/spec_reporter.snapshot
index ad0c88c26f..8f5cf3131b 100644
--- a/test/fixtures/test-runner/output/spec_reporter.snapshot
+++ b/test/fixtures/test-runner/output/spec_reporter.snapshot
@@ -57,7 +57,7 @@
       *
 
  async assertion fail (*ms)
-  AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:
+  AssertionError: Expected values to be strictly equal:
   
   true !== false
   
@@ -68,7 +68,7 @@
       *
       *
       * {
-    generatedMessage: true,
+    generatedMessage: false,
     code: 'ERR_ASSERTION',
     actual: true,
     expected: false,
@@ -109,7 +109,8 @@
  subtest sync throw fail (*ms)
 
  sync throw non-error fail (*ms)
-  Symbol(thrown symbol from sync throw non-error fail)
+  [Error: Symbol(thrown symbol from sync throw non-error fail)
+  ]
 
  level 0a
    level 1a (*ms)
@@ -120,7 +121,8 @@
 
  top level
    +long running (*ms)
-    'test did not finish before its parent and was cancelled'
+    [Error: test did not finish before its parent and was cancelled
+    ]
 
    +short running
      ++short running (*ms)
@@ -149,8 +151,7 @@
  <anonymous> (*ms) # SKIP
  test with a name and options provided (*ms) # SKIP
  functionAndOptions (*ms) # SKIP
- escaped description \ # \#\ 
- 	  � �  (*ms)
+ escaped description \ # \#\ \n \t \f \v \b \r (*ms)
  escaped skip message (*ms) # SKIP
  escaped todo message (*ms)
  escaped diagnostic (*ms)
@@ -165,7 +166,8 @@
  async t is this in test (*ms)
  callback t is this in test (*ms)
  callback also returns a Promise (*ms)
-  'passed a callback but also returned a Promise'
+  [Error: passed a callback but also returned a Promise
+  ]
 
  callback throw (*ms)
   Error: thrown from callback throw
@@ -178,16 +180,14 @@
       *
 
  callback called twice (*ms)
-  'callback invoked multiple times'
+  Error: callback invoked multiple times
+      *
+      *
 
  callback called twice in different ticks (*ms)
  callback called twice in future tick (*ms)
-  Error [ERR_TEST_FAILURE]: callback invoked multiple times
-      * {
-    failureType: 'multipleCallbackInvocations',
-    cause: 'callback invoked multiple times',
-    code: 'ERR_TEST_FAILURE'
-  }
+  Error: callback invoked multiple times
+      *
 
  callback async throw (*ms)
   Error: thrown from callback async throw
@@ -206,10 +206,15 @@
 
  'only' and 'runOnly' require the --test-only command-line option.
  custom inspect symbol fail (*ms)
-  customized
+  [Error: customized
+  ]
 
  custom inspect symbol that throws fail (*ms)
-  { foo: 1, [Symbol(nodejs.util.inspect.custom)]: [Function: [nodejs.util.inspect.custom]] }
+  [Error: {
+    foo: 1,
+    [Symbol(nodejs.util.inspect.custom)]: [Function: [nodejs.util.inspect.custom]]
+  }
+  ]
 
  subtest sync throw fails
    sync throw fails at first (*ms)
@@ -241,16 +246,19 @@
  subtest sync throw fails (*ms)
 
  timed out async test (*ms)
-  'test timed out after *ms'
+  [Error: test timed out after *ms
+  ]
 
  timed out callback test (*ms)
-  'test timed out after *ms'
+  [Error: test timed out after *ms
+  ]
 
  large timeout async test is ok (*ms)
  large timeout callback test is ok (*ms)
  successful thenable (*ms)
  rejected thenable (*ms)
-  'custom error'
+  [Error: custom error
+  ]
 
  unfinished test with uncaughtException (*ms)
   Error: foo
@@ -265,7 +273,7 @@
       *
 
  assertion errors display actual and expected properly (*ms)
-  AssertionError [ERR_ASSERTION]: Expected values to be loosely deep-equal:
+  AssertionError: Expected values to be loosely deep-equal:
   
   {
     bar: 1,
@@ -279,7 +287,7 @@
     c: [Circular *1]
   }
       * {
-    generatedMessage: true,
+    generatedMessage: false,
     code: 'ERR_ASSERTION',
     actual: [Object],
     expected: [Object],
@@ -287,7 +295,8 @@
   }
 
  invalid subtest fail (*ms)
-  'test could not be started because its parent finished'
+  Error: test could not be started because its parent finished
+      *
 
  Warning: Test "unhandled rejection - passes but warns" generated asynchronous activity after the test ended. This activity created the error "Error: rejected from unhandled rejection fail" and would have caused the test to fail, but instead triggered an unhandledRejection event.
  Warning: Test "async unhandled rejection - passes but warns" generated asynchronous activity after the test ended. This activity created the error "Error: rejected from async unhandled rejection fail" and would have caused the test to fail, but instead triggered an unhandledRejection event.
@@ -358,7 +367,7 @@
       *
 
  async assertion fail (*ms)
-  AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:
+  AssertionError: Expected values to be strictly equal:
   
   true !== false
   
@@ -369,7 +378,7 @@
       *
       *
       * {
-    generatedMessage: true,
+    generatedMessage: false,
     code: 'ERR_ASSERTION',
     actual: true,
     expected: false,
@@ -400,16 +409,20 @@
       *
 
  subtest sync throw fail (*ms)
-  '1 subtest failed'
+  [Error: 1 subtest failed
+  ]
 
  sync throw non-error fail (*ms)
-  Symbol(thrown symbol from sync throw non-error fail)
+  [Error: Symbol(thrown symbol from sync throw non-error fail)
+  ]
 
  +long running (*ms)
-  'test did not finish before its parent and was cancelled'
+  [Error: test did not finish before its parent and was cancelled
+  ]
 
  top level (*ms)
-  '1 subtest failed'
+  [Error: 1 subtest failed
+  ]
 
  sync skip option is false fail (*ms)
   Error: this should be executed
@@ -427,7 +440,8 @@
       *
 
  callback also returns a Promise (*ms)
-  'passed a callback but also returned a Promise'
+  [Error: passed a callback but also returned a Promise
+  ]
 
  callback throw (*ms)
   Error: thrown from callback throw
@@ -440,15 +454,13 @@
       *
 
  callback called twice (*ms)
-  'callback invoked multiple times'
+  Error: callback invoked multiple times
+      *
+      *
 
  callback called twice in future tick (*ms)
-  Error [ERR_TEST_FAILURE]: callback invoked multiple times
-      * {
-    failureType: 'multipleCallbackInvocations',
-    cause: 'callback invoked multiple times',
-    code: 'ERR_TEST_FAILURE'
-  }
+  Error: callback invoked multiple times
+      *
 
  callback async throw (*ms)
   Error: thrown from callback async throw
@@ -456,10 +468,15 @@
       *
 
  custom inspect symbol fail (*ms)
-  customized
+  [Error: customized
+  ]
 
  custom inspect symbol that throws fail (*ms)
-  { foo: 1, [Symbol(nodejs.util.inspect.custom)]: [Function: [nodejs.util.inspect.custom]] }
+  [Error: {
+    foo: 1,
+    [Symbol(nodejs.util.inspect.custom)]: [Function: [nodejs.util.inspect.custom]]
+  }
+  ]
 
  sync throw fails at first (*ms)
   Error: thrown from subtest sync throw fails at first
@@ -488,16 +505,20 @@
       *
 
  subtest sync throw fails (*ms)
-  '2 subtests failed'
+  [Error: 2 subtests failed
+  ]
 
  timed out async test (*ms)
-  'test timed out after *ms'
+  [Error: test timed out after *ms
+  ]
 
  timed out callback test (*ms)
-  'test timed out after *ms'
+  [Error: test timed out after *ms
+  ]
 
  rejected thenable (*ms)
-  'custom error'
+  [Error: custom error
+  ]
 
  unfinished test with uncaughtException (*ms)
   Error: foo
@@ -512,7 +533,7 @@
       *
 
  assertion errors display actual and expected properly (*ms)
-  AssertionError [ERR_ASSERTION]: Expected values to be loosely deep-equal:
+  AssertionError: Expected values to be loosely deep-equal:
   
   {
     bar: 1,
@@ -526,7 +547,7 @@
     c: [Circular *1]
   }
       * {
-    generatedMessage: true,
+    generatedMessage: false,
     code: 'ERR_ASSERTION',
     actual: [Object],
     expected: [Object],
@@ -534,4 +555,5 @@
   }
 
  invalid subtest fail (*ms)
-  'test could not be started because its parent finished'
+  Error: test could not be started because its parent finished
+      *

MoLow · 2023-04-13T10:25:10Z

CC @ErickWendel

ljharb · 2023-04-13T17:23:08Z

lib/internal/test_runner/reporter/spec.js

      hasChildren = true;
    }
-    const skippedSubtest = subtest && data.skip && data.skip !== undefined;
+    const skippedSubtest = subtest && 'skip' in data;


i don't think this is right - skip has to be truthy, otherwise skip: false will skip a test.

skip can be an empty string or undefined, but Il fix it to support false

in every test runner i'm aware of, a falsyskip - including undefined and an empty string - does not skip the test. The purpose is so you can dynamically skip something, and it would be incredibly surprising and confusing to users if a falsy value meant "true".

yeah I guess you are right it seems like the TAP parser makes skip an empty string

cjihrig · 2023-04-13T18:21:08Z

lib/internal/test_runner/tap_parser.js

-        test.reason = reason;
-      }
-
+    if (todoOrSkipToken === 'todo' || todoOrSkipToken === 'skip') {


Nothing that needs to change in this PR, but this whole block (lines 745-761 with the changes in this PR) looks like it could be optimized a bit.

cjihrig · 2023-04-13T18:30:02Z

test/parallel/test-runner-tap-parser-stream.js

          id: '2',
          description: 'test',
-          reason: '',
+          reason: true,


The changes in this file, and the first two changes in test/parallel/test-runner-tap-parser.js don't look correct. I would think the boolean would be status.skip, which it looks like it is on line 321. The reason should be a string.

ljharb · 2023-04-14T06:31:53Z

lib/internal/test_runner/runner.js

+          directive = this.reporter.getSkip(node.reason || true);
        } else if (todo) {
-          directive = this.reporter.getTodo(node.reason);
+          directive = this.reporter.getTodo(node.reason || true);


so, always?

inside this code branch? yes

then why the node.reason || at all, if it'll literally always be true? or is it for a skip message

It is a string including a description for the skip, it is not necesaraly a boolean

So in lib/internal/test_runner/tests_stream.js we are doing this. Should we change that to || instead of using || in this file and then ?? again there?

no, since in tests_stream we do not want to convert falsy values to true, but here it is ok since we are inside an if detecting we are in a skipped test

cjihrig

LGTM, but left a small comment: #47537 (comment).

nodejs-github-bot · 2023-04-15T17:21:59Z

CI: https://ci.nodejs.org/job/node-test-pull-request/51279/

nodejs-github-bot · 2023-04-15T19:30:36Z

CI: https://ci.nodejs.org/job/node-test-pull-request/51284/

nodejs-github-bot · 2023-04-15T20:50:59Z

CI: https://ci.nodejs.org/job/node-test-pull-request/51287/

nodejs-github-bot · 2023-04-15T22:13:05Z

CI: https://ci.nodejs.org/job/node-test-pull-request/51288/

nodejs-github-bot · 2023-04-16T06:05:15Z

CI: https://ci.nodejs.org/job/node-test-pull-request/51292/

nodejs-github-bot · 2023-04-16T07:01:56Z

CI: https://ci.nodejs.org/job/node-test-pull-request/51293/

nodejs-github-bot · 2023-04-16T07:26:39Z

Landed in 46a3cff

PR-URL: #47537 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

PR-URL: nodejs#47537 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

nodejs-github-bot added dont-land-on-v14.x needs-ci PRs that need a full CI run. test_runner Issues and PRs related to the test runner subsystem. labels Apr 13, 2023

MoLow mentioned this pull request Apr 13, 2023

test_runner: fix todo and skip messages on spec-reporter #47525

Closed

MoLow force-pushed the fix-spec-skip branch from 54c2c00 to 591847c Compare April 13, 2023 15:52

MoLow requested review from ErickWendel, benjamingr and cjihrig April 13, 2023 15:57

ljharb reviewed Apr 13, 2023

View reviewed changes

MoLow force-pushed the fix-spec-skip branch 2 times, most recently from 4971d18 to e7e9868 Compare April 13, 2023 18:06

cjihrig reviewed Apr 13, 2023

View reviewed changes

test_runner: fix spec skip detection

f09efef

MoLow requested a review from cjihrig April 14, 2023 05:35

MoLow force-pushed the fix-spec-skip branch from e7e9868 to f09efef Compare April 14, 2023 05:35

ljharb reviewed Apr 14, 2023

View reviewed changes

cjihrig approved these changes Apr 14, 2023

View reviewed changes

MoLow added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 15, 2023

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 15, 2023

benjamingr approved these changes Apr 16, 2023

View reviewed changes

MoLow added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 16, 2023

MoLow added the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 16, 2023

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 16, 2023

nodejs-github-bot merged commit 46a3cff into nodejs:main Apr 16, 2023

MoLow deleted the fix-spec-skip branch April 16, 2023 07:26

github-actions bot mentioned this pull request Apr 18, 2023

CI Reliability 2023-04-18 nodejs/reliability#544

Open

35 tasks

targos pushed a commit that referenced this pull request May 2, 2023

test_runner: fix spec skip detection

4203057

PR-URL: #47537 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

targos mentioned this pull request May 2, 2023

v20.1.0 release proposal #47820

Merged

danielleadams pushed a commit that referenced this pull request Jul 6, 2023

test_runner: fix spec skip detection

b3ec4db

PR-URL: #47537 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

MoLow added a commit to MoLow/node that referenced this pull request Jul 6, 2023

test_runner: fix spec skip detection

688078b

PR-URL: nodejs#47537 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

danielleadams mentioned this pull request Jul 10, 2023

v18.17.0 release proposal #48694

Merged

vassudanagunta mentioned this pull request Feb 14, 2026

test_runner: todo/skip/expectFailure are truthy per docs, but implementation is otherwise #61815

Open

Uh oh!

Conversation

MoLow commented Apr 13, 2023

Uh oh!

nodejs-github-bot commented Apr 13, 2023

Uh oh!

MoLow commented Apr 13, 2023

Uh oh!

MoLow commented Apr 13, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ljharb Apr 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cjihrig left a comment

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Apr 15, 2023

Uh oh!

nodejs-github-bot commented Apr 15, 2023

Uh oh!

nodejs-github-bot commented Apr 15, 2023

Uh oh!

nodejs-github-bot commented Apr 15, 2023

Uh oh!

nodejs-github-bot commented Apr 16, 2023

Uh oh!

nodejs-github-bot commented Apr 16, 2023

Uh oh!

nodejs-github-bot commented Apr 16, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ljharb Apr 14, 2023 •

edited

Loading