GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — test_runner: catch reporter errors by MoLow · Pull Request #49646 · nodejs/node

Conversation

MoLow commented Sep 14, 2023

Copy link

Copy Markdown

Member

Fixes: #48937

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bricss and tonivj5 reacted with thumbs up emoji

All reactions

👍 2 reactions

nodejs-github-bot commented Sep 14, 2023

Copy link

Copy Markdown

Collaborator

Review requested:

@nodejs/test_runner

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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 requested changes

Sep 14, 2023

View reviewed changes

Comment thread

lib/internal/test_runner/utils.js Resolved

Uh oh!

There was an error while loading. Please reload this page.

ljharb approved these changes

Sep 14, 2023

View reviewed changes

ljharb left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this 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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

atlowChemi reacted with thumbs up emoji

All reactions

👍 1 reaction

MoLow added the commit-queue-squash PRs the Commit Queue should land as one squashed commit. label

Sep 14, 2023

atlowChemi approved these changes

Sep 14, 2023

View reviewed changes

Comment thread

lib/internal/test_runner/utils.js Resolved

Uh oh!

There was an error while loading. Please reload this page.

cjihrig reviewed

Sep 14, 2023

View reviewed changes

Comment thread

lib/internal/test_runner/utils.js Resolved

Uh oh!

There was an error while loading. Please reload this page.

benjamingr reviewed

Sep 14, 2023

View reviewed changes

Comment thread

lib/internal/test_runner/utils.js Resolved

Uh oh!

There was an error while loading. Please reload this page.

benjamingr reviewed

Sep 14, 2023

View reviewed changes

Comment thread

test/fixtures/test-runner/custom_reporters/throwing.js Resolved

Uh oh!

There was an error while loading. Please reload this page.

benjamingr approved these changes

Sep 14, 2023

View reviewed changes

benjamingr requested changes

Sep 14, 2023

View reviewed changes

benjamingr left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this 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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

MoLow and ljharb reacted with thumbs up emoji

All reactions

👍 2 reactions

MoLow force-pushed the print-reporter-errors branch from 81853a5 to c40d76d Compare

September 21, 2023 06:17

MoLow requested review from atlowChemi, benjamingr, cjihrig and ljharb

September 21, 2023 06:17

atlowChemi reviewed

Sep 21, 2023

View reviewed changes

Comment thread

lib/internal/test_runner/harness.js Resolved

Uh oh!

There was an error while loading. Please reload this page.

MoLow removed the commit-queue-squash PRs the Commit Queue should land as one squashed commit. label

Sep 21, 2023


          test_runner: catch reporter errors

38aed2c

MoLow force-pushed the print-reporter-errors branch from c40d76d to 38aed2c Compare

September 21, 2023 07:00

benjamingr approved these changes

Sep 21, 2023

View reviewed changes

MoLow added author ready

PRs with CI started, the required approvals, and no outstanding review comments.

request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. labels

Sep 21, 2023

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label

Sep 21, 2023

nodejs-github-bot commented Sep 21, 2023

Copy link

Copy Markdown

Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

fix

a76e97e

MoLow added the commit-queue-squash PRs the Commit Queue should land as one squashed commit. label

Sep 21, 2023

cjihrig approved these changes

Sep 21, 2023

View reviewed changes

cjihrig left a comment

Copy link

Copy Markdown

Contributor

There was a problem hiding this 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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

test/fixtures/test-runner/custom_reporters/throwing-async.js Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

test/fixtures/test-runner/custom_reporters/throwing.js Resolved

Uh oh!

There was an error while loading. Please reload this page.


          Apply suggestions from code review

aebf089

Co-authored-by: Colin Ihrig <[email protected]>

MoLow added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label

Sep 21, 2023

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label

Sep 21, 2023

nodejs-github-bot commented Sep 21, 2023

Copy link

Copy Markdown

Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

MoLow added a commit to MoLow/node that referenced this pull request

Sep 21, 2023


          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 commented Sep 21, 2023

Copy link

Copy Markdown

Member Author

Landed in a4c7f81

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

MoLow closed this

Sep 21, 2023

MoLow deleted the print-reporter-errors branch

September 21, 2023 20:26

This was referenced Sep 22, 2023

CI Reliability 2023-09-22 nodejs/reliability#673

Open

CI Reliability 2023-09-23 nodejs/reliability#674

Open

CI Reliability 2023-09-24 nodejs/reliability#675

Open

ruyadorno commented Sep 28, 2023

Copy link

Copy Markdown

Member

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

ruyadorno added the backport-requested-v20.x label

Sep 28, 2023

MoLow mentioned this pull request

Sep 28, 2023

[v20.x backport] test_runner: catch reporter errors #49941

Closed

MoLow added backport-open-v20.x and removed backport-requested-v20.x labels

Sep 28, 2023

MoLow added a commit to MoLow/node that referenced this pull request

Sep 28, 2023


          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 mentioned this pull request

Oct 9, 2023

2023-10-17, Version 21.0.0 (Current) #49870

Merged

targos pushed a commit that referenced this pull request

Nov 11, 2023


          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 mentioned this pull request

Nov 12, 2023

v20.10.0 release proposal #50682

Merged

targos removed the backport-open-v20.x label

Nov 12, 2023

targos pushed a commit that referenced this pull request

Nov 27, 2023


          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 mentioned this pull request

Nov 28, 2023

v18.19.0 release proposal #50953

Merged

juanarbol mentioned this pull request

Dec 6, 2023

2023-12-06, N|Solid v5.0.1 Iron Release nodesource/nsolid#41

Merged

sercher added a commit to sercher/graaljs that referenced this pull request

Apr 25, 2024


          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


          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]>

Reviewers

atlowChemi

atlowChemi approved these changes

benjamingr

benjamingr approved these changes

cjihrig

cjihrig approved these changes

ljharb

Awaiting requested review from ljharb

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

author ready

PRs with CI started, the required approvals, and no outstanding review comments.

commit-queue-squash

PRs the Commit Queue should land as one squashed commit.

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

Uh oh!

There was an error while loading. Please reload this page.

8 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.