Skip to content

test: add dgram.Socket.prototype.sendto's test - #10901

Closed
hiroppy wants to merge 1 commit into
nodejs:masterfrom
hiroppy:feature/add-test-dgram-sendto
Closed

test: add dgram.Socket.prototype.sendto's test#10901
hiroppy wants to merge 1 commit into
nodejs:masterfrom
hiroppy:feature/add-test-dgram-sendto

Conversation

@hiroppy

Copy link
Copy Markdown
Member

sendto: https://github.com/nodejs/node/blob/master/lib/dgram.js#L234

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

test

@nodejs-github-botnodejs-github-bot added test Issues and PRs related to the tests. lts-watch-v6.x labels Jan 19, 2017
Comment threadtest/parallel/test-dgram-sendto.js Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can you store this regular expression in a variable for reuse.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Updated.

@hiroppy
hiroppyforce-pushed the feature/add-test-dgram-sendto branch from 0dc8717 to df5c0faCompareJanuary 19, 2017 18:50
@mscdexmscdex added the dgram Issues and PRs related to the dgram subsystem / UDP. label Jan 19, 2017
Comment threadtest/parallel/test-dgram-sendto.js Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

No need for the RegExp() constructor.

Comment threadtest/parallel/test-dgram-sendto.js Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If all the sendto() calls are supposed to throw before actually sending, and we're verifying the error messages, I don't think binding, listening, and closing the socket is necessary.

@Trott

Copy link
Copy Markdown
Member

LGTM if CI is green.

I'd leave a nit suggesting being more specific with the identifier name than just errorMessage but I'm having a hard time coming up with something significantly better that isn't probably too long, so ¯\(ツ)/¯.

@targos

Copy link
Copy Markdown
Member

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

Tip for the link reference you put in the commit message: You can press Y on your keyboard to get an absolute link to the current revision. That way the line will remain correct even after the file is changed on master.

@hiroppy

Copy link
Copy Markdown
MemberAuthor

@targos Thanks :) I modified this commit message.

@targos

Copy link
Copy Markdown
Member

Landed in 320108c

@targostargos closed this Jan 24, 2017
targos pushed a commit that referenced this pull request Jan 24, 2017
Refs: https://github.com/nodejs/node/blob/09ebdf14005cc948529b3f193ad550d5dfded26a/lib/dgram.js#L234
PR-URL: #10901
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 25, 2017
Refs: https://github.com/nodejs/node/blob/09ebdf14005cc948529b3f193ad550d5dfded26a/lib/dgram.js#L234
PR-URL: nodejs#10901
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 27, 2017
Refs: https://github.com/nodejs/node/blob/09ebdf14005cc948529b3f193ad550d5dfded26a/lib/dgram.js#L234
PR-URL: nodejs#10901
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@italoacasasitaloacasas mentioned this pull request Jan 29, 2017
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 30, 2017
Refs: https://github.com/nodejs/node/blob/09ebdf14005cc948529b3f193ad550d5dfded26a/lib/dgram.js#L234
PR-URL: nodejs#10901
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 30, 2017
Refs: https://github.com/nodejs/node/blob/09ebdf14005cc948529b3f193ad550d5dfded26a/lib/dgram.js#L234
PR-URL: nodejs#10901
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@hiroppy
hiroppy deleted the feature/add-test-dgram-sendto branch February 7, 2017 00:23
MylesBorins pushed a commit that referenced this pull request Mar 8, 2017
Refs: https://github.com/nodejs/node/blob/09ebdf14005cc948529b3f193ad550d5dfded26a/lib/dgram.js#L234
PR-URL: #10901
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@MylesBorins

Copy link
Copy Markdown
Contributor

failures on v4.x, did not land

=== release test-dgram-sendto ===
Path: parallel/test-dgram-sendto
assert.js:361
throw actual;
^
Error: send takes offset and length as args 2 and 3
at Socket.sendto (dgram.js:237:11)
at /Users/mborins/code/node/v4.x/test/parallel/test-dgram-sendto.js:11:10
at _tryBlock (assert.js:320:5)
at _throws (assert.js:339:12)
at Function.assert.throws (assert.js:369:3)
at Object.<anonymous> (/Users/mborins/code/node/v4.x/test/parallel/test-dgram-sendto.js:10:8)
at Module._compile (module.js:409:26)
at Object.Module._extensions..js (module.js:416:10)
at Module.load (module.js:343:32)
at Function.Module._load (module.js:300:12)

MylesBorins pushed a commit that referenced this pull request Mar 9, 2017
Refs: https://github.com/nodejs/node/blob/09ebdf14005cc948529b3f193ad550d5dfded26a/lib/dgram.js#L234
PR-URL: #10901
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@MylesBorinsMylesBorins mentioned this pull request Mar 9, 2017
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

dgramIssues and PRs related to the dgram subsystem / UDP.testIssues and PRs related to the tests.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants

@hiroppy@Trott@targos@MylesBorins@jasnell@cjihrig@mscdex@nodejs-github-bot