Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

test_runner: catch reporter errors #49646

Closed
wants to merge 3 commits into from
Closed

test_runner: catch reporter errors #49646

wants to merge 3 commits into from

Conversation

MoLow
Copy link
Member

@MoLow MoLow commented Sep 14, 2023

Fixes: #48937

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/test_runner

@nodejs-github-bot 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 Sep 14, 2023
ljharb
ljharb suggested changes Sep 14, 2023
View reviewed changes
lib/internal/test_runner/utils.js Outdated Show resolved Hide resolved
ljharb
ljharb approved these changes Sep 14, 2023
View reviewed changes
Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

would be good to have a test of something that does process.exitCode = 42 or whatever, and then throws, and asserts that the exitCode remains 42 afterwards

@MoLow MoLow added the commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. label Sep 14, 2023
atlowChemi
atlowChemi approved these changes Sep 14, 2023
View reviewed changes
lib/internal/test_runner/utils.js Outdated Show resolved Hide resolved
benjamingr
benjamingr requested changes Sep 14, 2023
View reviewed changes
Copy link
Member

@benjamingr benjamingr left a comment

Choose a reason for hiding this comment

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

I was convinced think @cjihrig is right and we should also deal with the case the error is async given this is the test runner.

@MoLow MoLow removed the commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. label Sep 21, 2023
@MoLow 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 Sep 21, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 21, 2023
@nodejs-github-bot
Copy link
Collaborator

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

@MoLow MoLow added the commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. label Sep 21, 2023
cjihrig
cjihrig approved these changes Sep 21, 2023
View reviewed changes
Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

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

LGTM once the CI passes.

test/fixtures/test-runner/custom_reporters/throwing-async.js Outdated Show resolved Hide resolved
test/fixtures/test-runner/custom_reporters/throwing.js Outdated Show resolved Hide resolved
@MoLow MoLow added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 21, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 21, 2023
@nodejs-github-bot
Copy link
Collaborator

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

MoLow added a commit to MoLow/node that referenced this pull request Sep 21, 2023
@MoLow
test_runner: catch reporter errors
a4c7f81 
PR-URL: nodejs#49646
Fixes: nodejs#48937
Reviewed-By: Chemi Atlow <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@MoLow
Copy link
Member Author

MoLow commented Sep 21, 2023

Landed in a4c7f81

@MoLow MoLow closed this Sep 21, 2023
@MoLow MoLow deleted the print-reporter-errors branch September 21, 2023 20:26
This was referenced Sep 22, 2023
Open
Open
Open
@ruyadorno
Copy link
Member

This commit does not land cleanly on v20.x-staging and will need manual backport in case we want it in v20.

@ruyadorno ruyadorno added the backport-requested-v20.x PRs awaiting manual backport to the v20.x-staging branch. label Sep 28, 2023
@MoLow MoLow mentioned this pull request Sep 28, 2023
Closed
@MoLow MoLow 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 Sep 28, 2023
MoLow added a commit to MoLow/node that referenced this pull request Sep 28, 2023
@MoLow
test_runner: catch reporter errors
0e27a7a 
PR-URL: nodejs#49646
Fixes: nodejs#48937
Reviewed-By: Chemi Atlow <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@RafaelGSS RafaelGSS mentioned this pull request Oct 9, 2023
Merged
alexfernandez pushed a commit to alexfernandez/node that referenced this pull request Nov 1, 2023
@MoLow @alexfernandez
test_runner: catch reporter errors
15ebb7e 
PR-URL: nodejs#49646
Fixes: nodejs#48937
Reviewed-By: Chemi Atlow <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
targos pushed a commit that referenced this pull request Nov 11, 2023
@MoLow @targos
test_runner: catch reporter errors
27558c4 
PR-URL: #49646
Fixes: #48937
Reviewed-By: Chemi Atlow <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@targos targos mentioned this pull request Nov 12, 2023
Merged
@targos targos removed the backport-open-v20.x Indicate that the PR has an open backport label Nov 12, 2023
targos pushed a commit that referenced this pull request Nov 27, 2023
@MoLow @targos
test_runner: catch reporter errors
103c4a0 
PR-URL: #49646
Fixes: #48937
Reviewed-By: Chemi Atlow <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@targos targos mentioned this pull request Nov 28, 2023
Merged
@juanarbol juanarbol mentioned this pull request Dec 6, 2023
Merged
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
@sercher
test_runner: catch reporter errors
9e54ca0 
PR-URL: nodejs/node#49646
Fixes: nodejs/node#48937
Reviewed-By: Chemi Atlow <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
@sercher
test_runner: catch reporter errors
5f38e70 
PR-URL: nodejs/node#49646
Fixes: nodejs/node#48937
Reviewed-By: Chemi Atlow <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@atlowChemi atlowChemi atlowChemi approved these changes

@cjihrig cjihrig cjihrig approved these changes

@benjamingr benjamingr benjamingr approved these changes

@ljharb ljharb Awaiting requested review from ljharb

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. needs-ci PRs that need a full CI run. test_runner Issues and PRs related to the test runner subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Throwing an exception on test reporters silently error the test runner
8 participants
@MoLow @nodejs-github-bot @ruyadorno @ljharb @benjamingr @cjihrig @atlowChemi @targos