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: refactor child-process-fork-net #21095

Closed
wants to merge 3 commits into from
Closed

Conversation

Trott
Copy link
Member

@Trott Trott commented Jun 2, 2018

Split test-child-process-fork-net into
test-child-process-fork-net-server and
test-child-process-fork-net-socket. Rename
test-child-process-fork-net2.js to test-child-process-fork-net.js.

Refs: #21012

One of the files involved here is actively being worked on in an effort to make it not flaky. If the fix ends up being to the test (rather than a bug in Node.js itself), it will have a merge conflict with this. I'll rebase or close if that happens. (Just don't want the people working on the test to see this and get annoyed or worried or something along those lines.)

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

Split test-child-process-fork-net into
test-child-process-fork-net-server and
test-child-process-fork-net-socket. Rename
test-child-process-fork-net2.js to test-child-process-fork-net.js.

Refs: nodejs#21012
@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Jun 2, 2018
@Trott
Copy link
Member Author

Trott commented Jun 2, 2018

@Trott Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jun 2, 2018
@Trott
Copy link
Member Author

Trott commented Jun 2, 2018

@Trott
Copy link
Member Author

Trott commented Jun 3, 2018

Hmmm...both times, the test run for Windows was green. @targos Any chance you can pull down test-child-process-fork-net-socket from this PR and see if it fails for you on your Windows machine? While at it, maybe we can see if that socket.on('close',...) listener is running. (I imagine it can/should be removed?)

@Trott
Copy link
Member Author

Trott commented Jun 3, 2018

Stress test of the current test monolith on master: https://ci.nodejs.org/job/node-stress-single-test/1898/nodes=win2016-1p-vs2017/

Stress test of the previously (and perhaps still?) unreliable portion of that above test as refactored in this PR will be here once that one above finishes running: https://ci.nodejs.org/job/node-stress-single-test/1902/nodes=win2016-1p-vs2017/

I don't expect that the results will be different form each other, but if they are, then I guess that definitely points to side effects being an issue in the current test instability.

@targos
Copy link
Member

targos commented Jun 3, 2018

@Trott I'm compiling your branch just to be sure but yes, it still fails for me, more than 75% of the runs.

@targos
Copy link
Member

targos commented Jun 3, 2018

Failure is more difficult to obtain with this branch (maybe because of the V8 upgrade?) but it still happens!

@targos
Copy link
Member

targos commented Jun 5, 2018

Landed in 3c5b8b4

@targos targos closed this Jun 5, 2018
targos pushed a commit that referenced this pull request Jun 5, 2018
Split test-child-process-fork-net into
test-child-process-fork-net-server and
test-child-process-fork-net-socket. Rename
test-child-process-fork-net2.js to test-child-process-fork-net.js.

Refs: #21012

PR-URL: #21095
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@targos targos removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jun 5, 2018
MylesBorins pushed a commit that referenced this pull request Jun 6, 2018
Split test-child-process-fork-net into
test-child-process-fork-net-server and
test-child-process-fork-net-socket. Rename
test-child-process-fork-net2.js to test-child-process-fork-net.js.

Refs: #21012

PR-URL: #21095
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Jun 6, 2018
@Trott Trott deleted the split-fork-net branch January 13, 2022 22:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants