-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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: update test case in test-net-internal.js #24461
Conversation
84e860b
to
27578a5
Compare
27578a5
to
4d1179d
Compare
Re-run of failing node-test-commit-windows-fanned. |
4d1179d
to
27b0667
Compare
@danbev The testcase crashed on windows, so i skip it. |
Re-run of CI: https://ci.nodejs.org/job/node-test-pull-request/18898/. |
Add test code for `makeSyncWrite`, which improve `test/parallel/test-net-internal.js` test coverage to 100%
27b0667
to
ae69291
Compare
@danbev Travis CI failed due to
|
@leeight Would you be able to rebase this PR? Doing should take care of the issue we are seeing with Travis. Thanks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This tests internal APIs by directly accessing it. It should be possible to write the test with public facing APIs only. This is a Windows specific code, so it can only be triggered there and we do not run our regular coverage on Windows either. I don't think this should land as is.
ping @leeight - can you address the review comments? |
I will pick this up. |
#31851 removed the api |
8ae28ff
to
2935f72
Compare
Closing due to lack of continued activity. Can reopen if someone wishes to pick this up again |
Add test code for
makeSyncWrite
, which improvetest/parallel/test-net-internal.js
test coverage to 100%Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes