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.

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

test: avoid left behind child processes #46276

Merged
merged 1 commit into from
Jan 22, 2023

Conversation

richardlau
Copy link
Member

@richardlau richardlau commented Jan 19, 2023

Extend the Linux logic in test-child-process-exec-abortcontroller-promisified to all POSIX platforms.

Fixes: nodejs/build#3154
Refs: #37572 (comment)
Refs: #37518


This unblocks the SmartOS CI for Node.js 14 and 16 that has been failing since we migrated the smartos machines at the end of last year. I'm opening this on main (which hasn't been failing) so I can cherry pick the same fix to both v16.x-staging and v14.x-staging. My main aim here is to unblock the CI.

To clarify, the test itself passes but is leaving behind child processes, which cause the CI run to fail as it checks for left over node processes at the end of the test run.

cc @nodejs/releasers @bahamat

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. test Issues and PRs related to the tests. labels Jan 19, 2023
@richardlau richardlau added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 19, 2023
Extend the Linux logic to all POSIX platforms in
test-child-process-exec-abortcontroller-promisified.
@richardlau richardlau removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 19, 2023
@nodejs-github-bot

This comment was marked as outdated.

@richardlau richardlau added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 19, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 19, 2023
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@richardlau richardlau added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 20, 2023
@lpinca lpinca added the commit-queue Add this label to land a pull request using GitHub Actions. label Jan 20, 2023
Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

@mhdawson mhdawson added the fast-track PRs that do not need to wait for 48 hours to land. label Jan 20, 2023
@nodejs-github-bot nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Jan 20, 2023
@nodejs-github-bot
Copy link
Collaborator

Commit Queue failed
- Loading data for nodejs/node/pull/46276
✔  Done loading data for nodejs/node/pull/46276
----------------------------------- PR info ------------------------------------
Title      test: avoid left behind child processes (#46276)
   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch     richardlau:cpexecac -> nodejs:main
Labels     test, fast-track, author ready, needs-ci, lts-watch-v14.x, lts-watch-v16.x
Commits    1
 - test: avoid left behind child processes
Committers 1
 - Richard Lau 
PR-URL: https://github.com/nodejs/node/pull/46276
Fixes: https://github.com/nodejs/build/issues/3154
Refs: https://github.com/nodejs/node/issues/37518
Reviewed-By: Moshe Atlow 
Reviewed-By: Beth Griggs 
Reviewed-By: Ruy Adorno 
Reviewed-By: Colin Ihrig 
Reviewed-By: Luigi Pinca 
Reviewed-By: Michaël Zasso 
Reviewed-By: Michael Dawson 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/46276
Fixes: https://github.com/nodejs/build/issues/3154
Refs: https://github.com/nodejs/node/issues/37518
Reviewed-By: Moshe Atlow 
Reviewed-By: Beth Griggs 
Reviewed-By: Ruy Adorno 
Reviewed-By: Colin Ihrig 
Reviewed-By: Luigi Pinca 
Reviewed-By: Michaël Zasso 
Reviewed-By: Michael Dawson 
--------------------------------------------------------------------------------
   ℹ  This PR was created on Thu, 19 Jan 2023 16:48:37 GMT
   ✔  Approvals: 7
   ✔  - Moshe Atlow (@MoLow): https://github.com/nodejs/node/pull/46276#pullrequestreview-1262194862
   ✔  - Beth Griggs (@BethGriggs) (TSC): https://github.com/nodejs/node/pull/46276#pullrequestreview-1262204146
   ✔  - Ruy Adorno (@ruyadorno): https://github.com/nodejs/node/pull/46276#pullrequestreview-1262207413
   ✔  - Colin Ihrig (@cjihrig) (TSC): https://github.com/nodejs/node/pull/46276#pullrequestreview-1262429139
   ✔  - Luigi Pinca (@lpinca): https://github.com/nodejs/node/pull/46276#pullrequestreview-1263218104
   ✔  - Michaël Zasso (@targos) (TSC): https://github.com/nodejs/node/pull/46276#pullrequestreview-1263219222
   ✔  - Michael Dawson (@mhdawson) (TSC): https://github.com/nodejs/node/pull/46276#pullrequestreview-1264226655
   ℹ  This PR is being fast-tracked
   ✖  The fast-track request requires at least one collaborator's approval (👍).
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2023-01-19T23:07:22Z: https://ci.nodejs.org/job/node-test-pull-request/49094/
- Querying data for job/node-test-pull-request/49094/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu
https://github.com/nodejs/node/actions/runs/3970755114

@aduh95 aduh95 added commit-queue Add this label to land a pull request using GitHub Actions. and removed fast-track PRs that do not need to wait for 48 hours to land. commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Jan 22, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jan 22, 2023
@nodejs-github-bot nodejs-github-bot merged commit 2df72fe into nodejs:main Jan 22, 2023
@nodejs-github-bot
Copy link
Collaborator

Landed in 2df72fe

richardlau added a commit to richardlau/node-1 that referenced this pull request Jan 22, 2023
Extend the Linux logic to all POSIX platforms in
test-child-process-exec-abortcontroller-promisified.

PR-URL: nodejs#46276
Fixes: nodejs/build#3154
Refs: nodejs#37518
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Beth Griggs <[email protected]>
Reviewed-By: Ruy Adorno <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: James M Snell <[email protected]>
richardlau added a commit that referenced this pull request Jan 22, 2023
Extend the Linux logic to all POSIX platforms in
test-child-process-exec-abortcontroller-promisified.

PR-URL: #46276
Fixes: nodejs/build#3154
Refs: #37518
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Beth Griggs <[email protected]>
Reviewed-By: Ruy Adorno <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@richardlau richardlau deleted the cpexecac branch January 30, 2023 12:21
ruyadorno pushed a commit that referenced this pull request Feb 1, 2023
Extend the Linux logic to all POSIX platforms in
test-child-process-exec-abortcontroller-promisified.

PR-URL: #46276
Fixes: nodejs/build#3154
Refs: #37518
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Beth Griggs <[email protected]>
Reviewed-By: Ruy Adorno <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@ruyadorno ruyadorno mentioned this pull request Feb 1, 2023
juanarbol pushed a commit that referenced this pull request Mar 3, 2023
Extend the Linux logic to all POSIX platforms in
test-child-process-exec-abortcontroller-promisified.

PR-URL: #46276
Fixes: nodejs/build#3154
Refs: #37518
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Beth Griggs <[email protected]>
Reviewed-By: Ruy Adorno <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@juanarbol juanarbol mentioned this pull request Mar 3, 2023
juanarbol pushed a commit that referenced this pull request Mar 5, 2023
Extend the Linux logic to all POSIX platforms in
test-child-process-exec-abortcontroller-promisified.

PR-URL: #46276
Fixes: nodejs/build#3154
Refs: #37518
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Beth Griggs <[email protected]>
Reviewed-By: Ruy Adorno <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: James M Snell <[email protected]>
oraluben pushed a commit to oraluben/node that referenced this pull request Mar 14, 2023
Extend the Linux logic to all POSIX platforms in
test-child-process-exec-abortcontroller-promisified.

PR-URL: nodejs/node#46276
Fixes: nodejs/build#3154
Refs: nodejs/node#37518
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Beth Griggs <[email protected]>
Reviewed-By: Ruy Adorno <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: James M Snell <[email protected]>
oraluben pushed a commit to oraluben/node that referenced this pull request Mar 17, 2023
Extend the Linux logic to all POSIX platforms in
test-child-process-exec-abortcontroller-promisified.

PR-URL: nodejs/node#46276
Fixes: nodejs/build#3154
Refs: nodejs/node#37518
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Beth Griggs <[email protected]>
Reviewed-By: Ruy Adorno <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. needs-ci PRs that need a full CI run. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

node-daily-v14.x-staging is failing on smartos