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: update test case in test-net-internal.js #24461

Closed
wants to merge 1 commit into from

Conversation

leeight
Copy link
Contributor

@leeight leeight commented Nov 18, 2018

Add test code for makeSyncWrite, which improve
test/parallel/test-net-internal.js test coverage to 100%

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

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Nov 18, 2018
@lpinca
Copy link
Member

lpinca commented Nov 19, 2018

@danbev
Copy link
Contributor

danbev commented Nov 21, 2018

Re-run of failing node-test-commit-windows-fanned.
Re-run of failing node-test-commit-custom-suites-freestyle

@leeight
Copy link
Contributor Author

leeight commented Nov 22, 2018

@danbev The testcase crashed on windows, so i skip it.

@danbev
Copy link
Contributor

danbev commented Nov 23, 2018

Add test code for `makeSyncWrite`, which improve
`test/parallel/test-net-internal.js` test coverage to 100%
@leeight
Copy link
Contributor Author

leeight commented Nov 24, 2018

@danbev Travis CI failed due to

https://api.github.com/repos/nodejs/node/commits/ae69291115e6f8641956346b094d4cbc0cdea134
/home/travis/.npm/_npx/4528/lib/node_modules/core-validate-commit/bin/cmd.js:145
      if (err) throw err
               ^
Error: Invalid api format
    at new Parser (/home/travis/.npm/_npx/4528/lib/node_modules/core-validate-commit/node_modules/gitlint-parser-base/index.js:23:15)
    at new Parser (/home/travis/.npm/_npx/4528/lib/node_modules/core-validate-commit/node_modules/gitlint-parser-node/index.js:15:5)
    at ValidateCommit.lint (/home/travis/.npm/_npx/4528/lib/node_modules/core-validate-commit/lib/index.js:49:22)
    at load (/home/travis/.npm/_npx/4528/lib/node_modules/core-validate-commit/bin/cmd.js:146:9)
    at IncomingMessage.res.on (/home/travis/.npm/_npx/4528/lib/node_modules/core-validate-commit/bin/cmd.js:77:9)
    at IncomingMessage.emit (events.js:187:15)
    at endReadableNT (_stream_readable.js:1098:12)
    at process.internalTickCallback (internal/process/next_tick.js:72:19)
The command "if [ "${TRAVIS_PULL_REQUEST}" != "false" ]; then bash -x tools/lint-pr-commit-message.sh ${TRAVIS_PULL_REQUEST}; fi" exited with 1.

@danbev
Copy link
Contributor

danbev commented Nov 27, 2018

@leeight Would you be able to rebase this PR? Doing should take care of the issue we are seeing with Travis. Thanks

@Trott
Copy link
Member

Trott commented Dec 1, 2018

Copy link
Member

@BridgeAR BridgeAR left a 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.

@gireeshpunathil
Copy link
Member

ping @leeight - can you address the review comments?

@HarshithaKP
Copy link
Member

I will pick this up.

@HarshithaKP
Copy link
Member

#31851 removed the api isLegalPort. Part of that, the test was removed it looks like - because the test was testing that API.

@BridgeAR BridgeAR force-pushed the master branch 2 times, most recently from 8ae28ff to 2935f72 Compare May 31, 2020 12:19
@jasnell jasnell added the stalled Issues and PRs that are stalled. label Jul 7, 2020
@jasnell
Copy link
Member

jasnell commented Jul 7, 2020

Closing due to lack of continued activity. Can reopen if someone wishes to pick this up again

@jasnell jasnell closed this Jul 7, 2020
@jasnell jasnell added this to Would need investigation in Futures Jul 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stalled Issues and PRs that are stalled. test Issues and PRs related to the tests.
Projects
Futures
Would need investigation
Development

Successfully merging this pull request may close these issues.

None yet

9 participants