test_runner: fix global after not failing the tests #48913

rluvaton · 2023-07-25T08:17:04Z

Fix: #48867

I'm not sure about the solution, WDYT? 🤔

nodejs-github-bot · 2023-07-25T08:17:08Z

Review requested:

@nodejs/test_runner

lib/internal/test_runner/test.js

MoLow · 2023-07-25T18:54:37Z

test/fixtures/test-runner/output/global-after-should-fail-the-test.snapshot

+  ---
+  duration_ms: *
+  ...
+not ok 0 - <root>


I wonder if we should change this name. @cjihrig WDYT?

I don't think the root test should even show up in the output like this because users will have no clue what it is (the root test is basically an implementation detail). For failed global hooks, I think we should try to do something similar to what we do when we run a test file with no tests that exits with non-zero.

In case it's not clear what I mean:

// foo.js process.exit(1);

$ node --test foo.js ✖ /private/tmp/foo.js (23.390208ms) 'test failed' ℹ tests 1 ℹ suites 0 ℹ pass 0 ℹ fail 1 ℹ cancelled 0 ℹ skipped 0 ℹ todo 0 ℹ duration_ms 27.586916 ✖ failing tests: ✖ /private/tmp/foo.js (23.390208ms) 'test failed'

updated, can you take a look?

Sorry, I should have been a bit more explicit. When I said "something similar" I meant specify the name of the hook, not the filename. For example, "global after hook" or something alone those lines.

When the before failed we have:

'use strict'; const { it, before } = require('node:test'); before(() => { throw new Error('this should fail the test') }); it('this is a test', () => { console.log('this is a test') });

✖ this is a test (0.679959ms) Error: this should fail the test at TestContext.<anonymous> (/Users/rluvaton/dev/open-source/node/node-fork/test/fixtures/test-runner/output/global_after_should_fail_the_test.js:5:9) at TestHook.runInAsyncScope (node:async_hooks:206:9) at TestHook.run (node:internal/test_runner/test:581:25) at TestHook.run (node:internal/test_runner/test:774:18) at TestHook.run (node:internal/util:500:12) at node:internal/test_runner/test:517:20 at async Test.runHook (node:internal/test_runner/test:515:7) at async Test.run (node:internal/test_runner/test:554:9) at async startSubtest (node:internal/test_runner/harness:204:3) ℹ tests 1 ℹ suites 0 ℹ pass 0 ℹ fail 1 ℹ cancelled 0 ℹ skipped 0 ℹ todo 0 ℹ duration_ms 116.542583 ✖ failing tests: ✖ this is a test (0.679959ms) Error: this should fail the test at TestContext.<anonymous> (/Users/rluvaton/dev/open-source/node/node-fork/test/fixtures/test-runner/output/global_after_should_fail_the_test.js:5:9) at TestHook.runInAsyncScope (node:async_hooks:206:9) at TestHook.run (node:internal/test_runner/test:581:25) at TestHook.run (node:internal/test_runner/test:774:18) at TestHook.run (node:internal/util:500:12) at node:internal/test_runner/test:517:20 at async Test.runHook (node:internal/test_runner/test:515:7) at async Test.run (node:internal/test_runner/test:554:9) at async startSubtest (node:internal/test_runner/harness:204:3)

With Node 20.5.0, a failing global before() fails the test run with the same error shown in #48913 (comment). I pulled down this PR locally, and verified there is no difference.

With Node 20.5.0, a failing global after() does not fail the test run, and is a bug. This PR does address that, but the failure should be more accurate IMO:

this is a test ✔ this is a test (1.356584ms) ✖ /tmp/foo.js (0.22ms) Error: this should fail the test at TestContext.<anonymous> (/private/tmp/foo.js:5:9) at TestHook.runInAsyncScope (node:async_hooks:206:9) at TestHook.run (node:internal/test_runner/test:581:25) at TestHook.run (node:internal/test_runner/test:774:18) at TestHook.run (node:internal/util:500:12) at node:internal/test_runner/test:517:20 at async Test.runHook (node:internal/test_runner/test:515:7) at async after (node:internal/test_runner/test:543:9) at async Test.run (node:internal/test_runner/test:591:7) at async process.exitHandler (node:internal/test_runner/harness:144:5) ℹ tests 1 ℹ suites 0 ℹ pass 1 ℹ fail 0 ℹ cancelled 0 ℹ skipped 0 ℹ todo 0 ℹ duration_ms 0.22 ✖ failing tests: ✖ /tmp/foo.js (0.22ms) Error: this should fail the test at TestContext.<anonymous> (/private/tmp/foo.js:5:9) at TestHook.runInAsyncScope (node:async_hooks:206:9) at TestHook.run (node:internal/test_runner/test:581:25) at TestHook.run (node:internal/test_runner/test:774:18) at TestHook.run (node:internal/util:500:12) at node:internal/test_runner/test:517:20 at async Test.runHook (node:internal/test_runner/test:515:7) at async after (node:internal/test_runner/test:543:9) at async Test.run (node:internal/test_runner/test:591:7) at async process.exitHandler (node:internal/test_runner/harness:144:5)

Just looking at that output, I would have no idea that the failure is because the after hook failed. That is important information for users. That's what I'm saying we need to change in #48913 (comment).

EDIT: More specifically, it would be good if ✖ /tmp/foo.js (0.22ms) in this example said ✖ global after hook (0.22ms)

More specifically, it would be good if ✖ /tmp/foo.js (0.22ms) in this example said ✖ global after hook (0.22ms)

it has a failureType: 'hookFailed' , wich is printed in case of the tap reporter, but in spec we seem to omit it.
still - there is a stack trace, isnt that good enough?

I mean it's better than the existing bug, but seems like something we could improve 🤷

ideally, I would prefer each hook that failed to tell that it failed from that hook

I think it should be in a different PR, and in that PR I would also align the before to match the right behavior...

lib/internal/test_runner/test.js

test/fixtures/test-runner/output/global_after_should_fail_the_test.snapshot

MoLow

LGTM

rluvaton · 2023-07-25T22:17:09Z

The test test/parallel/test-runner-cli.js is failing as it picked up new test files, #48787 would have fixed that

nodejs-github-bot · 2023-07-26T20:39:30Z

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

MoLow · 2023-07-27T06:42:31Z

@rluvaton the new tests seem to fail on windows https://ci.nodejs.org/job/node-test-binary-windows-js-suites/22188/RUN_SUBSET=1,nodes=win2012r2-COMPILED_BY-vs2019-x86/console

00:29:13         not ok 7 - test-runner/output/global_after_should_fail_the_test.js
00:29:13           ---
00:29:13           duration_ms: 780.562048
00:29:13           failureType: 'testCodeFailure'
00:29:13           error: |-
00:29:13             Expected values to be strictly equal:
00:29:13             + actual - expected ... Lines skipped
00:29:13             
00:29:13               'this is a test\n' +
00:29:13                 'TAP version 13\n' +
00:29:13             ...
00:29:13                 '  duration_ms: *\n' +
00:29:13                 '  ...\n' +
00:29:13             +   'not ok 2 - C:\\\\workspace\\\\node-test-binary-windows-js-suites\\\\node\\\\test\\\\fixtures\\\\test-runner\\\\output\\\\global_after_should_fail_the_test.js\n' +
00:29:13             -   'not ok 2 - /test/fixtures/test-runner/output/global_after_should_fail_the_test.js\n' +
00:29:13                 '  ---\n' +
00:29:13             ...
00:29:13                 '# skipped 0\n' +
00:29:13                 '# todo 0\n' +
00:29:13                 '# duration_ms *\n'

rluvaton · 2023-07-27T07:06:01Z

I pushed a fix, hope this would work

MoLow · 2023-07-27T07:42:15Z

@rluvaton commits like this dont actually trigger the CI

rluvaton · 2023-07-27T07:45:58Z

What would you do to trigger a CI?

nodejs-github-bot · 2023-08-02T20:06:27Z

Landed in c58e8fc

PR-URL: nodejs#48913 Fixes: nodejs#48867 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Chemi Atlow <chemi@atlow.co.il>

PR-URL: #48913 Fixes: #48867 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Chemi Atlow <chemi@atlow.co.il>

PR-URL: nodejs#48913 Fixes: nodejs#48867 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Chemi Atlow <chemi@atlow.co.il>

RafaelGSS · 2023-08-17T19:11:32Z

Due to fact, #48877 didn't land cleanly on v20.x-staging. This PR somehow, depends on it. So we'll need a manual backport.

Reference: https://github.com/nodejs/node/blob/main/doc/contributing/backporting-to-release-lines.md

PR-URL: nodejs#48913 Fixes: nodejs#48867 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Chemi Atlow <chemi@atlow.co.il>

rluvaton · 2023-08-18T10:08:05Z

Backport PR:

[v20.x backport] backport multiple test runner related prs #49225

PR-URL: nodejs#48913 Fixes: nodejs#48867 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Chemi Atlow <chemi@atlow.co.il>

PR-URL: #48913 Backport-PR-URL: #49225 Fixes: #48867 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Chemi Atlow <chemi@atlow.co.il>

PR-URL: nodejs/node#48913 Backport-PR-URL: nodejs/node#49225 Fixes: nodejs/node#48867 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Chemi Atlow <chemi@atlow.co.il>

rluvaton marked this pull request as ready for review July 25, 2023 08:17

nodejs-github-bot added needs-ci PRs that need a full CI run. test_runner labels Jul 25, 2023

rluvaton marked this pull request as draft July 25, 2023 08:17

rluvaton marked this pull request as ready for review July 25, 2023 08:23

rluvaton marked this pull request as draft July 25, 2023 08:24

atlowChemi reviewed Jul 25, 2023

View reviewed changes

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

rluvaton marked this pull request as ready for review July 25, 2023 18:12

MoLow reviewed Jul 25, 2023

View reviewed changes

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

MoLow requested a review from cjihrig July 25, 2023 18:55

benjamingr approved these changes Jul 25, 2023

View reviewed changes

MoLow reviewed Jul 25, 2023

View reviewed changes

test/fixtures/test-runner/output/global_after_should_fail_the_test.snapshot Outdated Show resolved Hide resolved

MoLow approved these changes Jul 25, 2023

View reviewed changes

rluvaton mentioned this pull request Jul 25, 2023

test: relocate test files fixtures for better understanding #48787

Merged

rluvaton force-pushed the fix-after-not-failing-the-root-test branch from dc7d333 to 3cf434d Compare July 26, 2023 18:31

MoLow approved these changes Jul 26, 2023

View reviewed changes

rluvaton mentioned this pull request Jul 26, 2023

test_runner: propagate only to test ancestors #48932

Closed

5 tasks

MoLow added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Jul 26, 2023

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

github-actions bot mentioned this pull request Jul 27, 2023

CI Reliability 2023-07-27 nodejs/reliability#625

Open

18 tasks

atlowChemi approved these changes Jul 27, 2023

View reviewed changes

nodejs-github-bot merged commit c58e8fc into nodejs:main Aug 2, 2023
58 checks passed

rluvaton deleted the fix-after-not-failing-the-root-test branch August 2, 2023 20:06

This was referenced Aug 3, 2023

CI Reliability 2023-08-03 nodejs/reliability#632

Open

CI Reliability 2023-08-04 nodejs/reliability#633

Open

UlisesGascon mentioned this pull request Aug 15, 2023

v20.6.0 proposal #49185

Merged

RafaelGSS added the backport-requested-v20.x PRs awaiting manual backport to the v20.x-staging branch. label Aug 17, 2023

rluvaton mentioned this pull request Aug 18, 2023

[v20.x backport] backport multiple test runner related prs #49225

Closed

rluvaton added backport-open-v20.x Indicate that the PR has an open backport and removed backport-requested-v20.x PRs awaiting manual backport to the v20.x-staging branch. labels Aug 18, 2023

UlisesGascon mentioned this pull request Sep 10, 2023

v20.7.0 proposal #49592

Merged

targos added backported-to-v20.x PRs backported to the v20.x-staging branch. and removed backport-open-v20.x Indicate that the PR has an open backport labels Nov 12, 2023

targos mentioned this pull request Nov 28, 2023

v18.19.0 release proposal #50953

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test_runner: fix global after not failing the tests #48913

test_runner: fix global after not failing the tests #48913

rluvaton commented Jul 25, 2023 •

edited by atlowChemi

nodejs-github-bot commented Jul 25, 2023

MoLow Jul 25, 2023

cjihrig Jul 25, 2023

rluvaton Jul 25, 2023

cjihrig Jul 25, 2023

rluvaton Jul 25, 2023

cjihrig Jul 25, 2023 •

edited

MoLow Jul 25, 2023

cjihrig Jul 25, 2023

rluvaton Jul 25, 2023

rluvaton Jul 25, 2023

MoLow left a comment

rluvaton commented Jul 25, 2023

nodejs-github-bot commented Jul 26, 2023

MoLow commented Jul 27, 2023

rluvaton commented Jul 27, 2023

MoLow commented Jul 27, 2023

rluvaton commented Jul 27, 2023

nodejs-github-bot commented Aug 2, 2023

RafaelGSS commented Aug 17, 2023

rluvaton commented Aug 18, 2023

test_runner: fix global after not failing the tests #48913

test_runner: fix global after not failing the tests #48913

Conversation

rluvaton commented Jul 25, 2023 • edited by atlowChemi

nodejs-github-bot commented Jul 25, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjihrig Jul 25, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MoLow left a comment

Choose a reason for hiding this comment

rluvaton commented Jul 25, 2023

nodejs-github-bot commented Jul 26, 2023

MoLow commented Jul 27, 2023

rluvaton commented Jul 27, 2023

MoLow commented Jul 27, 2023

rluvaton commented Jul 27, 2023

nodejs-github-bot commented Aug 2, 2023

RafaelGSS commented Aug 17, 2023

rluvaton commented Aug 18, 2023

rluvaton commented Jul 25, 2023 •

edited by atlowChemi

cjihrig Jul 25, 2023 •

edited