Skip to content

test_runner: catch reporter errors#49646

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
Closed
MoLow wants to merge 3 commits into from
Closed

test_runner: catch reporter errors #49646

MoLow wants to merge 3 commits into from

Conversation

@MoLow
Copy link
Member

@MoLowMoLow commented Sep 14, 2023

Fixes: #48937

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Sep 14, 2023

Review requested:

  • @nodejs/test_runner

@nodejs-github-botnodejs-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 requested changes Sep 14, 2023
View reviewed changes
ljharb
ljharb approved these changes Sep 14, 2023
View reviewed changes
Copy link
Member

@ljharbljharb 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

@MoLowMoLow 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
cjihrig
cjihrig reviewed Sep 14, 2023
View reviewed changes
benjamingr
benjamingr reviewed Sep 14, 2023
View reviewed changes
benjamingr
benjamingr requested changes Sep 14, 2023
View reviewed changes
Copy link
Member

@benjamingrbenjamingr 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.

@MoLowMoLowforce-pushed the print-reporter-errors branch from 81853a5 to c40d76dCompareSeptember 21, 2023 06:17
atlowChemi
atlowChemi reviewed Sep 21, 2023
View reviewed changes
@MoLowMoLow 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
@MoLowMoLowforce-pushed the print-reporter-errors branch from c40d76d to 38aed2cCompareSeptember 21, 2023 07:00
@MoLowMoLow 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-actionsgithub-actionsbot 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

nodejs-github-bot commented Sep 21, 2023

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

@MoLowMoLow 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

@cjihrigcjihrig 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.

@MoLowMoLow added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 21, 2023
@github-actionsgithub-actionsbot 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

nodejs-github-bot commented Sep 21, 2023

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#49646Fixes: nodejs#48937 Reviewed-By: Chemi Atlow <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
@MoLow
Copy link
MemberAuthor

MoLow commented Sep 21, 2023

Landed in a4c7f81

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

ruyadorno commented Sep 28, 2023

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

@ruyadornoruyadorno added the backport-requested-v20.x PRs awaiting manual backport to the v20.x-staging branch. label Sep 28, 2023
@MoLowMoLow mentioned this pull request Sep 28, 2023
Closed
@MoLowMoLow 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#49646Fixes: nodejs#48937 Reviewed-By: Chemi Atlow <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
@RafaelGSSRafaelGSS 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#49646Fixes: 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: #49646Fixes: #48937 Reviewed-By: Chemi Atlow <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
@targostargos mentioned this pull request Nov 12, 2023
Merged
@targostargos 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: #49646Fixes: #48937 Reviewed-By: Chemi Atlow <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
@targostargos mentioned this pull request Nov 28, 2023
Merged
@juanarboljuanarbol 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#49646Fixes: 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#49646Fixes: nodejs/node#48937 Reviewed-By: Chemi Atlow <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@atlowChemiatlowChemiatlowChemi approved these changes

@benjamingrbenjamingrbenjamingr approved these changes

@cjihrigcjihrigcjihrig approved these changes

@ljharbljharbAwaiting requested review from ljharb

Assignees

No one assigned

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.commit-queue-squashAdd this label to instruct the Commit Queue to squash all the PR commits into the first one.needs-ciPRs that need a full CI run.test_runnerIssues 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