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.jsprocess.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(()=>{thrownewError('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#48913Fixes: nodejs#48867 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: Chemi Atlow <[email protected]>

PR-URL: #48913Fixes: #48867 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: Chemi Atlow <[email protected]>

PR-URL: nodejs#48913Fixes: nodejs#48867 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: Chemi Atlow <[email protected]>

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#48913Fixes: nodejs#48867 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: Chemi Atlow <[email protected]>

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

Backport PR:

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

PR-URL: nodejs#48913Fixes: nodejs#48867 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: Chemi Atlow <[email protected]>

PR-URL: #48913 Backport-PR-URL: #49225Fixes: #48867 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: Chemi Atlow <[email protected]>

PR-URL: nodejs/node#48913 Backport-PR-URL: nodejs/node#49225Fixes: nodejs/node#48867 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: Chemi Atlow <[email protected]>

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 Issues and PRs related to the test runner subsystem. 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 resolvedHide 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 resolvedHide 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 resolvedHide 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 3cf434dCompare 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-actionsbot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 26, 2023

github-actionsbot 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:mainAug 2, 2023

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

Uh oh!

test_runner: fix global after not failing the tests#48913

test_runner: fix global after not failing the tests #48913

Uh oh!

Conversation

rluvaton commented Jul 25, 2023• edited by atlowChemi Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Jul 25, 2023

Uh oh!

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!

cjihrigJul 25, 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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

MoLow left a comment

Choose a reason for hiding this comment

Uh oh!

rluvaton commented Jul 25, 2023

Uh oh!

nodejs-github-bot commented Jul 26, 2023

Uh oh!

MoLow commented Jul 27, 2023

Uh oh!

rluvaton commented Jul 27, 2023

Uh oh!

MoLow commented Jul 27, 2023

Uh oh!

rluvaton commented Jul 27, 2023

Uh oh!

nodejs-github-bot commented Aug 2, 2023

Uh oh!

RafaelGSS commented Aug 17, 2023

Uh oh!

rluvaton commented Aug 18, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

rluvaton commented Jul 25, 2023•
edited by atlowChemi
Loading

cjihrigJul 25, 2023•
edited
Loading