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: try to stabilize test-child-process-fork-exec-path.js #27277

Closed
wants to merge 3 commits into from
Closed

test: try to stabilize test-child-process-fork-exec-path.js #27277

wants to merge 3 commits into from

Conversation

refack
Copy link
Contributor

@refack refack commented Apr 17, 2019

An attempt to de-flake test/parallel/test-child-process-fork-exec-path.js

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Apr 17, 2019
@nodejs-github-bot
Copy link
Collaborator

Lite-CI: https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/3283

@refack refack force-pushed the stablize-test-child-process-fork-exec-path branch from 3dda26d to 5ec9d3f Compare April 17, 2019 19:21
@refack refack added child_process Issues and PRs related to the child_process subsystem. flaky-test Issues and PRs related to the tests with unstable failures on the CI. windows Issues and PRs related to the Windows platform. labels Apr 17, 2019
@refack
Copy link
Contributor Author

refack commented Apr 17, 2019 •
edited
Loading

This reproduces differently using the stress job - https://ci.nodejs.org/job/node-stress-single-test/2204/

So I've started 3 Windows jobs
https://ci.nodejs.org/job/node-test-commit-windows-fanned/26603/ ❌
https://ci.nodejs.org/job/node-test-commit-windows-fanned/26604/ ❌
https://ci.nodejs.org/job/node-test-commit-windows-fanned/26605/ ❌
As Compared with status-quo:

image

@refack
Copy link
Contributor Author

refack commented Apr 17, 2019

Still no boeno

c:\workspace\node-test-binary-windows-2\test\common\tmpdir.js:27
      throw e;
      ^

Error: ENOTEMPTY: directory not empty, rmdir 'c:\workspace\node-test-binary-windows-2\test\.tmp.3'
    at Object.rmdirSync (fs.js:706:3)
    at rmdirSync (c:\workspace\node-test-binary-windows-2\test\common\tmpdir.js:48:10)
    at rimrafSync (c:\workspace\node-test-binary-windows-2\test\common\tmpdir.js:18:7)
    at Object.refresh (c:\workspace\node-test-binary-windows-2\test\common\tmpdir.js:66:3)
    at process.<anonymous> (c:\workspace\node-test-binary-windows-2\test\parallel\test-child-process-fork-exec-path.js:66:12)

@refack refack force-pushed the stablize-test-child-process-fork-exec-path branch from 5ec9d3f to 31bc7d0 Compare April 17, 2019 19:48
@refack

This comment has been minimized.

Sign in to view
@Trott
Copy link
Member

Trott commented Apr 17, 2019 •
edited
Loading

Is there a good reason we're using tmpdir at all? Why not just use the node executable where it is and don't move/copy/remove anything at all? EDIT: I guess to make sure it's not ignoring the execPath property? I should look more closely at things before writing comments. :-D

@refack
Copy link
Contributor Author

refack commented Apr 17, 2019

I guess to make sure it's not ignoring the execPath property?

70ad9bb

Worth documenting though

@refack refack force-pushed the stablize-test-child-process-fork-exec-path branch from 9a01a9a to 6bdc8d7 Compare April 17, 2019 20:22
@refack refack force-pushed the stablize-test-child-process-fork-exec-path branch from 6bdc8d7 to 907567e Compare April 17, 2019 21:06
@nodejs-github-bot
Copy link
Collaborator

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

@refack
Copy link
Contributor Author

refack commented Apr 17, 2019

https://ci.nodejs.org/job/node-test-commit-windows-fanned/26610/ ⚠️ (http2 flake)
https://ci.nodejs.org/job/node-test-commit-windows-fanned/26611/ ⚠️ (http2 flake)
https://ci.nodejs.org/job/node-test-commit-windows-fanned/26612/ ✔️

@refack
Copy link
Contributor Author

refack commented Apr 17, 2019

@nodejs/testing This flake has been failing fairly often in the last few days.
Please 👍 if you approve of fast tracking this.

@refack refack added the fast-track PRs that do not need to wait for 48 hours to land. label Apr 17, 2019
@danbev
Copy link
Contributor

danbev commented Apr 18, 2019

Landed in 4f8b497.

@danbev danbev closed this Apr 18, 2019
danbev pushed a commit that referenced this pull request Apr 18, 2019
@refack @danbev
test: try to stabalize test-child-process-fork-exec-path.js
4f8b497 
PR-URL: #27277
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@refack refack deleted the stablize-test-child-process-fork-exec-path branch April 18, 2019 11:48
@refack refack added the landed label Apr 18, 2019
This was referenced Apr 23, 2019
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
BethGriggs pushed a commit that referenced this pull request Feb 20, 2020
@refack @BethGriggs
test: try to stabalize test-child-process-fork-exec-path.js
2315270 
PR-URL: #27277
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@BethGriggs BethGriggs mentioned this pull request Feb 20, 2020
Closed
@BethGriggs BethGriggs mentioned this pull request Mar 12, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Trott Trott Trott left review comments

@joyeecheung joyeecheung joyeecheung approved these changes

@richardlau richardlau richardlau approved these changes

Assignees
No one assigned
Labels
child_process Issues and PRs related to the child_process subsystem. fast-track PRs that do not need to wait for 48 hours to land. flaky-test Issues and PRs related to the tests with unstable failures on the CI. test Issues and PRs related to the tests. windows Issues and PRs related to the Windows platform.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@refack @nodejs-github-bot @Trott @danbev @joyeecheung @richardlau