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

test: cleanup/update test-dgram-error-message-address.js by Goyapa · Pull Request #8938 · nodejs/node · GitHub

/ node Public

test: cleanup/update test-dgram-error-message-address.js - #8938

Closed
Goyapa wants to merge 1 commit into
nodejs:masterfrom
Goyapa:test-dgram-error-message
ConversationCommits (1)ChecksFiles changed
Closed

test: cleanup/update test-dgram-error-message-address.js#8938
Goyapa wants to merge 1 commit into
nodejs:masterfrom
Goyapa:test-dgram-error-message

Conversation

Goyapa commented Oct 5, 2016 •
edited by imyller
Loading

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

test
dgram

Description of change

test: cleanup/update test-dgram-error-message-address.js

* Changed some var to const
* This commit resolves #8925

nodejs-github-bot added the test Issues and PRs related to the tests. label Oct 5, 2016
Goyapa changed the title Test dgram error message test: cleanup/update test-dgram-error-message-address.js Oct 5, 2016
cjihrig approved these changes Oct 5, 2016
View reviewed changes

cjihrig left a comment

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.

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

LGTM, but there is some other cleanup that you could do while you're here. Like getting rid of assert.equal() and assert.notEqual().

mscdex added the dgram Issues and PRs related to the dgram subsystem / UDP. label Oct 5, 2016

Trott commented Oct 5, 2016 •
edited
Loading

Copy link
Copy Markdown
Member
  • Can you rebase/squash so that there aren't unrelated commits in the PR?

These changes would make this change more compelling:

  • assert.equal() -> assert.strictEqual()
  • assert.notEqual() -> assert.notStrictEqual()
  • The use of common.fail can be improved. Perhaps create a function that wraps it so that it prints a sensible message.

imyller mentioned this pull request Oct 7, 2016
Closed

imyller commented Oct 7, 2016

Copy link
Copy Markdown
Member

Fixes: #8925

lpinca commented Oct 12, 2016 •
edited
Loading

Copy link
Copy Markdown
Member

@Goyapa while you are at it can you please replace assert.equal() with assert.strictEqual() and assert.notEqual() with assert.notStrictEqual().

Also you can use the Fixes: metadata instead of This commit resolves #8925 as defined in CONTRIBUTING.md.

Goyapa commented Oct 13, 2016

Copy link
Copy Markdown
Contributor Author

@ipinca
My Laptop died, an i do not know when i am able to continue.
Sorry, how to process now?

lpinca commented Oct 13, 2016

Copy link
Copy Markdown
Member

@Goyapa we can use https://help.github.com/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork/ if it is not a problem for you.

addaleax commented Oct 13, 2016

Copy link
Copy Markdown
Member

You can also try to edit files on your branch using Github’s web interface, it’s a bit tricky but it’s nice when you don’t want to or can’t use a local git client.

Goyapa commented Oct 13, 2016

Copy link
Copy Markdown
Contributor Author

@lpinca
Allow edits from maintainers is checked v.

rvagg force-pushed the master branch 2 times, most recently from c133999 to 83c7a88 Compare October 18, 2016 17:02

Goyapa commented Nov 5, 2016

Copy link
Copy Markdown
Contributor Author

@lpinca
See that nothing has changed at this file on master.
I`m on it again!

test: cleanup/update test-dgram-error-message-address.js
9c8fb2f 
* Changes:

* var -> const
* assert.equal() -> assert.strictEqual()
* assert.notEqual() -> assert.notStrictEqual()

* Fixes: #8925

targos commented Nov 27, 2016

Copy link
Copy Markdown
Member

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

targos commented Nov 28, 2016

Copy link
Copy Markdown
Member

Landed in 6a73d61

targos closed this Nov 28, 2016
targos pushed a commit that referenced this pull request Nov 28, 2016
test: cleanup test-dgram-error-message-address
6a73d61 
* var -> const
* assert.equal() -> assert.strictEqual()
* assert.notEqual() -> assert.notStrictEqual()

Fixes: #8925
PR-URL: #8938
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
MylesBorins pushed a commit that referenced this pull request Nov 30, 2016
test: cleanup test-dgram-error-message-address
24fc1e2 
* var -> const
* assert.equal() -> assert.strictEqual()
* assert.notEqual() -> assert.notStrictEqual()

Fixes: #8925
PR-URL: #8938
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
MylesBorins pushed a commit that referenced this pull request Nov 30, 2016
test: cleanup test-dgram-error-message-address
9144d37 
* var -> const
* assert.equal() -> assert.strictEqual()
* assert.notEqual() -> assert.notStrictEqual()

Fixes: #8925
PR-URL: #8938
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
MylesBorins mentioned this pull request Dec 1, 2016
Merged
addaleax pushed a commit that referenced this pull request Dec 5, 2016
test: cleanup test-dgram-error-message-address
d06f010 
* var -> const
* assert.equal() -> assert.strictEqual()
* assert.notEqual() -> assert.notStrictEqual()

Fixes: #8925
PR-URL: #8938
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Fishrock123 mentioned this pull request Dec 5, 2016
Merged
2 tasks
MylesBorins mentioned this pull request Dec 6, 2016
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

jasnell jasnell approved these changes
cjihrig cjihrig approved these changes
+1 more reviewer
targos targos approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

dgram Issues and PRs related to the dgram subsystem / UDP. test Issues and PRs related to the tests.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Back | FazBrowse Home | New Git URL