FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

test: add unexpected value to dgram socket type assertion by kcaulfield94 · Pull Request #18449 · nodejs/node · GitHub

/ node Public

test: add unexpected value to dgram socket type assertion - #18449

Closed
kcaulfield94 wants to merge 1 commit into
nodejs:masterfrom
kcaulfield94:dgram-test-assertion
ConversationCommits (1)ChecksFiles changed
Closed

test: add unexpected value to dgram socket type assertion#18449
kcaulfield94 wants to merge 1 commit into
nodejs:masterfrom
kcaulfield94:dgram-test-assertion

Conversation

kcaulfield94 commented Jan 30, 2018

Copy link
Copy Markdown
Contributor
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

test/dgram

nodejs-github-bot added the test Issues and PRs related to the tests. label Jan 30, 2018
lpinca reviewed Jan 30, 2018
View reviewed changes
Comment thread test/parallel/test-dgram-createSocket-type.js Outdated

lpinca Jan 30, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I'm a bit confused. Why the message says "1300 or 2600" when the value should be 10000 or 20000?

kcaulfield94 Jan 30, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@lpinca I will update accordingly, unless anyone knows why these values were in place to begin with. Seems to have come from #13623.

BridgeAR Feb 1, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Can you please add that change? :-) I guess otherwise the PR would be ready to land.

kcaulfield94 Feb 1, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Absolutely, will be added shortly! Thanks!

kcaulfield94 commented Feb 2, 2018

Copy link
Copy Markdown
Contributor Author

I apologise. Embarrassingly, something seems to have gone wrong with my first Node contribution. Any suggestions?

bnoordhuis commented Feb 2, 2018

Copy link
Copy Markdown
Member

@kcaulfield94 git fetch origin && git rebase origin/master, then force-push to your feature branch. After that you should have a branch that's master + your commit on top.

kcaulfield94 commented Feb 2, 2018

Copy link
Copy Markdown
Contributor Author

@bnoordhuis thanks for being so kind. Will update shortly.

bnoordhuis commented Feb 2, 2018 •
edited
Loading

Copy link
Copy Markdown
Member

@kcaulfield94 Alternative approach: git reset --hard origin/master && git cherry-pick <your commit's hash>, then force-push again. You should be able to find it with git reflog.

edit: I see it's the last commit here, kcaulfield94/node@f390eb4. If all else fails, I can probably fix it up for you.

edit2: I just tried, I can't. Did you disallow edits from collaborators? You can pick from https://github.com/bnoordhuis/io.js/tree/pr18449 if you want.

BridgeAR commented Feb 6, 2018

Copy link
Copy Markdown
Member

Ping @kcaulfield94

BridgeAR approved these changes Feb 10, 2018
View reviewed changes
Comment thread test/parallel/test-dgram-createSocket-type.js
socket.getRecvBufferSize() === 20000,
'SO_RCVBUF not 1300 or 2600');
'SO_RCVBUF not 10000 or 20000, ' +
`was ${socket.getRecvBufferSize()}`);

BridgeAR Feb 10, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Non blocking nit: please indent this code two more so it is possible to distinguish it from the other lines. The same applies to the part below.

BridgeAR added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Feb 10, 2018

BridgeAR commented Feb 10, 2018

Copy link
Copy Markdown
Member

Light-CI https://ci.nodejs.org/job/node-test-commit-light/253/

BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Feb 12, 2018
test: fix and improve error message
18e7284 
Add unexpected value to dgram socket type assertion and fix the size
entry.

PR-URL: nodejs#18449
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

BridgeAR commented Feb 12, 2018

Copy link
Copy Markdown
Member

Landed in 18e7284 🎉

@kcaulfield94 congratulation on your first commit to Node.js!

BridgeAR closed this Feb 12, 2018
MylesBorins pushed a commit that referenced this pull request Feb 21, 2018
test: fix and improve error message
d40e90a 
Add unexpected value to dgram socket type assertion and fix the size
entry.

PR-URL: #18449
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
MylesBorins pushed a commit that referenced this pull request Feb 21, 2018
test: fix and improve error message
bd2edd7 
Add unexpected value to dgram socket type assertion and fix the size
entry.

PR-URL: #18449
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
MylesBorins mentioned this pull request Feb 21, 2018
Merged
MylesBorins pushed a commit that referenced this pull request Feb 21, 2018
test: fix and improve error message
3362ae7 
Add unexpected value to dgram socket type assertion and fix the size
entry.

PR-URL: #18449
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
test: fix and improve error message
042213f 
Add unexpected value to dgram socket type assertion and fix the size
entry.

PR-URL: nodejs#18449
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
MylesBorins pushed a commit that referenced this pull request Aug 7, 2018
test: fix and improve error message
edccc9f 
Add unexpected value to dgram socket type assertion and fix the size
entry.

PR-URL: #18449
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
rvagg pushed a commit that referenced this pull request Aug 16, 2018
test: fix and improve error message
bbc7443 
Add unexpected value to dgram socket type assertion and fix the size
entry.

PR-URL: #18449
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
MylesBorins mentioned this pull request Aug 16, 2018
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

lpinca lpinca left review comments
addaleax addaleax approved these changes
cjihrig cjihrig approved these changes
BridgeAR BridgeAR approved these changes

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no outstanding review comments, and a CI started. test Issues and PRs related to the tests.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

Back | FazBrowse Home | New Git URL