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: run after hooks even if test is aborted #54151

Merged
merged 1 commit into from
Aug 4, 2024
Merged

test_runner: run after hooks even if test is aborted #54151

merged 1 commit into from
Aug 4, 2024
+40 −8

Conversation

cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Jul 31, 2024

If a test is run, but aborted, any after hooks should still be run, as they may need to perform cleanup.

@cjihrig
test_runner: run after hooks even if test is aborted
35fd985 
If a test is run, but aborted, any after hooks should still be
run, as they may need to perform cleanup.
@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 Jul 31, 2024
@cjihrig cjihrig mentioned this pull request Jul 31, 2024
Merged
@codecov Codecov
Copy link

codecov bot commented Jul 31, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.07%. Comparing base (67bc6a4) to head (35fd985).
Report is 19 commits behind head on main.

Additional details and impacted files 
@@           Coverage Diff           @@
##             main   #54151   +/-   ##
=======================================
  Coverage   87.07%   87.07%           
=======================================
  Files         643      643           
  Lines      181580   181574    -6     
  Branches    34894    34887    -7     
=======================================
+ Hits       158103   158110    +7     
+ Misses      16749    16747    -2     
+ Partials     6728     6717   -11
Files Coverage Δ
lib/internal/test_runner/test.js 98.17% <100.00%> (+0.07%) ⬆️

... and 24 files with indirect coverage changes

@atlowChemi atlowChemi added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 1, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 1, 2024
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@cjihrig
Copy link
Contributor Author

cjihrig commented Aug 2, 2024 

17:45:17 not ok 122 parallel/test-child-process-fork-exec-path
17:45:17   ---
17:45:17   duration_ms: 3418.98200
17:45:17   severity: fail
17:45:17   exitcode: 1
17:45:17   stack: |-
17:45:17     Can't clean tmpdir: d:\workspace\node-test-binary-windows-js-suites\node\test\.tmp.112
17:45:17     Files blocking: [ 'node-copy.exe' ]
17:45:17     
17:45:17     d:\workspace\node-test-binary-windows-js-suites\node\test\common\tmpdir.js:69
17:45:17         throw e;
17:45:17         ^
17:45:17     
17:45:17     Error: EPERM, Permission denied: \\?\d:\workspace\node-test-binary-windows-js-suites\node\test\.tmp.112 '\\?\d:\workspace\node-test-binary-windows-js-suites\node\test\.tmp.112'
17:45:17         at Object.rmSync (node:fs:1246:18)
17:45:17         at rmSync (d:\workspace\node-test-binary-windows-js-suites\node\test\common\tmpdir.js:20:8)

Seen on the most recent CI run on Windows. Linking to #53617 just in case.

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@jakecastelli
Copy link
Contributor

rerun failed

here 
17:23:23 not ok 142 node-api/test_general/test
17:23:23   ---
17:23:23   duration_ms: 883.16200
17:23:23   severity: fail
17:23:23   exitcode: 1
17:23:23   stack: |-
17:23:23     Can't clean tmpdir: d:\workspace\node-test-binary-windows-native-suites\node\test\.tmp.141
17:23:23     Files blocking: [ 'foo%#bar' ]
17:23:23     
17:23:23     node:fs:1246
17:23:23       return binding.rmSync(getValidatedPath(path), opts.maxRetries, opts.recursive, opts.retryDelay);

may relate to nodejs/build#3848, will re-request the CI.

@jakecastelli jakecastelli added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 4, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 4, 2024
@nodejs-github-bot
Copy link
Collaborator

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

mcollina
mcollina approved these changes Aug 4, 2024
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@nodejs-github-bot
Copy link
Collaborator

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

@rluvaton rluvaton added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue Add this label to land a pull request using GitHub Actions. labels Aug 4, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 4, 2024
@nodejs-github-bot nodejs-github-bot merged commit 67f7137 into nodejs:main Aug 4, 2024
69 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 67f7137

@cjihrig cjihrig deleted the abort-after branch August 5, 2024 14:36
targos pushed a commit that referenced this pull request Aug 14, 2024
@cjihrig @targos
test_runner: run after hooks even if test is aborted
20a01fc 
If a test is run, but aborted, any after hooks should still be
run, as they may need to perform cleanup.

PR-URL: #54151
Reviewed-By: Chemi Atlow <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Jake Yuesong Li <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Raz Luvaton <[email protected]>
@RafaelGSS RafaelGSS mentioned this pull request Aug 19, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mcollina mcollina mcollina approved these changes

@benjamingr benjamingr benjamingr approved these changes

@MoLow MoLow MoLow approved these changes

@rluvaton rluvaton rluvaton approved these changes

@jakecastelli jakecastelli jakecastelli approved these changes

@atlowChemi atlowChemi atlowChemi approved these changes

Assignees
No one assigned
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_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.
8 participants
@cjihrig @nodejs-github-bot @jakecastelli @mcollina @benjamingr @MoLow @rluvaton @atlowChemi