GV

GeezerVison

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

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — test: add and use expectSyncExitWithoutError() and expectSyncExit() utils by joyeecheung · Pull Request #49020 · nodejs/node

Conversation

@joyeecheung

@joyeecheung joyeecheung commented Aug 4, 2023

Copy link
Copy Markdown
Member

test: add expectSyncExitWithoutError() and expectSyncExit() utils

These can be used to check the state and the output of a child
process launched with spawnSync(). They log additional information
about the child process when the check fails to facilitate debugging
test failures.

test: use expectSyncExit{WithErrors} in snapshot tests

..and replace the similar code added for logging.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
bricss reacted with eyes emoji
All reactions
  • 👀 1 reaction
@nodejs-github-bot

nodejs-github-bot commented Aug 4, 2023

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/startup
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. test Issues and PRs related to Node.js core tests and test infrastructure. labels Aug 4, 2023
@joyeecheung

joyeecheung commented Aug 4, 2023 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member Author

Many of our tests that launch child processes only check the status of them without additional logging so when they fail, usually what you see from a assert.strictEqual(child.status, 0) is only:

Uncaught AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:

1 !== 0

and without changing the code to do additional logging it's usually impossible to understand exactly what's causing the failure. this is especially troublesome when debugging flaky tests in the CI. So my hope is that we can also gradually use these methods instead of doing assert.strictEqual(child.status, 0) right after launching child processes in the tests....
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
VoltrexKeyva
VoltrexKeyva reviewed Aug 5, 2023
View reviewed changes
Comment thread test/common/README.md Resolved

Uh oh!

There was an error while loading. Please reload this page.
lpinca
lpinca approved these changes Aug 8, 2023
View reviewed changes
@joyeecheung joyeecheung 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 Aug 14, 2023
@github-actions 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 Aug 14, 2023
@nodejs-github-bot

nodejs-github-bot commented Aug 14, 2023

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/53308/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@github-actions github-actions Bot mentioned this pull request Aug 15, 2023
CI Reliability 2023-08-15 nodejs/reliability#637
Open
22 tasks
joyeecheung added 2 commits August 15, 2023 20:45
@joyeecheung
test: add expectSyncExitWithoutError() and expectSyncExit() utils
7b106ed 
These can be used to check the state and the output of a child
process launched with `spawnSync()`. They log additional information
about the child process when the check fails to facilitate debugging
test failures.
@joyeecheung
test: use expectSyncExit{WithErrors} in snapshot tests
9303852 
..and replace the similar code added for logging.
@joyeecheung
joyeecheung force-pushed the cp-test branch from 1fa4cce to 9303852 Compare August 15, 2023 18:45
@github-actions github-actions Bot mentioned this pull request Aug 16, 2023
CI Reliability 2023-08-16 nodejs/reliability#638
Open
24 tasks
@joyeecheung joyeecheung 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 Aug 16, 2023
@github-actions 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 Aug 16, 2023
@nodejs-github-bot

nodejs-github-bot commented Aug 16, 2023

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/53369/
All reactions

Sorry, something went wrong.

Uh oh!

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

joyeecheung commented Aug 16, 2023

Copy link
Copy Markdown
Member Author

Landed in 6391b3b...a3f12e4
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@joyeecheung joyeecheung closed this Aug 16, 2023
joyeecheung added a commit that referenced this pull request Aug 16, 2023
@joyeecheung
test: add expectSyncExitWithoutError() and expectSyncExit() utils
034c801 
These can be used to check the state and the output of a child
process launched with `spawnSync()`. They log additional information
about the child process when the check fails to facilitate debugging
test failures.

PR-URL: #49020
Reviewed-By: Luigi Pinca <[email protected]>
joyeecheung added a commit that referenced this pull request Aug 16, 2023
@joyeecheung
test: use expectSyncExit{WithErrors} in snapshot tests
a3f12e4 
..and replace the similar code added for logging.

PR-URL: #49020
Reviewed-By: Luigi Pinca <[email protected]>
@github-actions github-actions Bot mentioned this pull request Aug 17, 2023
CI Reliability 2023-08-17 nodejs/reliability#639
Open
26 tasks
UlisesGascon pushed a commit that referenced this pull request Sep 10, 2023
@joyeecheung @UlisesGascon
test: add expectSyncExitWithoutError() and expectSyncExit() utils
c441f5a 
These can be used to check the state and the output of a child
process launched with `spawnSync()`. They log additional information
about the child process when the check fails to facilitate debugging
test failures.

PR-URL: #49020
Reviewed-By: Luigi Pinca <[email protected]>
UlisesGascon pushed a commit that referenced this pull request Sep 10, 2023
@joyeecheung @UlisesGascon
test: use expectSyncExit{WithErrors} in snapshot tests
47d24f1 
..and replace the similar code added for logging.

PR-URL: #49020
Reviewed-By: Luigi Pinca <[email protected]>
@UlisesGascon UlisesGascon mentioned this pull request Sep 10, 2023
v20.7.0 proposal #49592
Merged
targos pushed a commit that referenced this pull request Nov 27, 2023
@joyeecheung @targos
test: add expectSyncExitWithoutError() and expectSyncExit() utils
52d5dd8 
These can be used to check the state and the output of a child
process launched with `spawnSync()`. They log additional information
about the child process when the check fails to facilitate debugging
test failures.

PR-URL: #49020
Reviewed-By: Luigi Pinca <[email protected]>
@targos targos mentioned this pull request Nov 28, 2023
v18.19.0 release proposal #50953
Merged
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
@sercher
test: add expectSyncExitWithoutError() and expectSyncExit()
9458aee 
These can be used to check the state and the output of a child
process launched with `spawnSync()`. They log additional information
about the child process when the check fails to facilitate debugging
test failures.

PR-URL: nodejs/node#49020
Reviewed-By: Luigi Pinca <[email protected]>
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
@sercher
test: add expectSyncExitWithoutError() and expectSyncExit()
4c5e224 
These can be used to check the state and the output of a child
process launched with `spawnSync()`. They log additional information
about the child process when the check fails to facilitate debugging
test failures.

PR-URL: nodejs/node#49020
Reviewed-By: Luigi Pinca <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lpinca lpinca lpinca approved these changes
+1 more reviewer
@VoltrexKeyva VoltrexKeyva VoltrexKeyva left review comments
Reviewers whose approvals may not affect merge requirements

Uh oh!

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

Assignees

No one assigned

Labels

needs-ci PRs that need a full CI run. test Issues and PRs related to Node.js core tests and test infrastructure.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

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

4 participants

@joyeecheung @nodejs-github-bot @lpinca @VoltrexKeyva
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.