Skip to content

test: update test case in test-net-internal.js#24461

Closed
leeight wants to merge 1 commit intonodejs:masterfrom
leeight:fix-test-net-internal
Closed

test: update test case in test-net-internal.js#24461
leeight wants to merge 1 commit intonodejs:masterfrom
leeight:fix-test-net-internal

Conversation

@leeight
Copy link
Contributor

@leeight leeight commented Nov 18, 2018 •
edited
Loading

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
@leeight leeight force-pushed the fix-test-net-internal branch from 84e860b to 27578a5 Compare November 19, 2018 04:19
@lpinca
Copy link
Member

lpinca commented Nov 19, 2018

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

@leeight leeight force-pushed the fix-test-net-internal branch from 27578a5 to 4d1179d Compare November 19, 2018 10:02
@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 leeight force-pushed the fix-test-net-internal branch from 4d1179d to 27b0667 Compare November 22, 2018 16:08
@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

Re-run of CI: https://ci.nodejs.org/job/node-test-pull-request/18898/.

@leeight
test: update test case in test-net-internal.js
ae69291 
Add test code for `makeSyncWrite`, which improve
`test/parallel/test-net-internal.js` test coverage to 100%
@leeight leeight force-pushed the fix-test-net-internal branch from 27b0667 to ae69291 Compare November 23, 2018 16:44
@leeight
Copy link
Contributor Author

leeight commented Nov 24, 2018 •
edited
Loading

@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

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

BridgeAR
BridgeAR requested changes Dec 20, 2019
View reviewed changes
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

gireeshpunathil commented Dec 28, 2019

ping @leeight - can you address the review comments?

@HarshithaKP
Copy link
Member

HarshithaKP commented Apr 12, 2020

I will pick this up.

@HarshithaKP
Copy link
Member

HarshithaKP commented Apr 13, 2020

#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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@BridgeAR BridgeAR BridgeAR requested changes

@jasnell jasnell jasnell approved these changes

@lpinca lpinca lpinca approved these changes

Assignees

No one assigned

Labels

stalled Issues and PRs that are stalled. test Issues and PRs related to the tests.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@leeight @lpinca @danbev @Trott @gireeshpunathil @HarshithaKP @jasnell @BridgeAR @nodejs-github-bot

Comments