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

test: favor `===` over in `==` in http test by jun-oka · Pull Request #8471 · nodejs/node · GitHub

/ node Public

test: favor === over in == in http test - #8471

Closed
jun-oka wants to merge 4 commits into
nodejs:masterfrom
jun-oka:test-http-eqeqeq
ConversationCommits (4)ChecksFiles changed
Closed

test: favor === over in == in http test#8471
jun-oka wants to merge 4 commits into
nodejs:masterfrom
jun-oka:test-http-eqeqeq

Conversation

jun-oka commented Sep 9, 2016

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

test http

Description of change

nodejs-github-bot added the test Issues and PRs related to the tests. label Sep 9, 2016

addaleax commented Sep 9, 2016

Copy link
Copy Markdown
Member

LGTM, thank you! :) By the way, your author name in this commit is given as “jun-oka”. Is that intended or do you prefer to be listed (changelog, git log, AUTHORS file) with some other name? People typically prefer their full name, but ultimately it’s up to you.

cjihrig commented Sep 9, 2016

Copy link
Copy Markdown
Contributor

LGTM

jun-oka commented Sep 9, 2016 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@addaleax Thank you for that info! In that case, I prefer to use "Junshu Okamoto".

addaleax commented Sep 9, 2016

Copy link
Copy Markdown
Member

@jun-oka I guess @Trott is around somewhere and can help you, but usually, you can update the commit here yourself. It goes something like this:

git config --global user.name 'Junshu Okamoto' # (leave out the --global if you only want this for Node)
git commit --amend --reset-author
git push --force-with-lease

(git magic! 😄)

Trott reviewed Sep 9, 2016
View reviewed changes
Comment thread test/parallel/test-http.js Outdated

Trott Sep 9, 2016

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 update assert.equal() here and elsewhere to use assert.strictEqual() instead? Might as well get all our minor refactoring in at once. :-D

jun-oka commented Sep 9, 2016

Copy link
Copy Markdown
Contributor Author

@addaleax Yes! @Trott is next to me.
@Trott OK. Thank you.

Trott commented Sep 9, 2016

Copy link
Copy Markdown
Member

LGTM if CI doesn't reveal any surprises.

Trott commented Sep 9, 2016

Copy link
Copy Markdown
Member

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

jun-oka commented Sep 9, 2016

Copy link
Copy Markdown
Contributor Author

It seems unrelated error, I hope.

lpinca commented Sep 10, 2016

Copy link
Copy Markdown
Member

The ubuntu1404-32 failure is unrelated and I think the arm one is expected.
LGTM.

thefourtheye reviewed Sep 10, 2016
View reviewed changes
Comment thread test/parallel/test-http.js Outdated

thefourtheye Sep 10, 2016

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

Now that you are doing this, why don't you change the requires as well.

jun-oka Sep 10, 2016

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

@thefourtheye That's right. Let me do that.

jun-oka Sep 12, 2016

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

@thefourtheye I will finish it today.

jasnell commented Sep 12, 2016

Copy link
Copy Markdown
Member

LGTM

jun-oka commented Sep 13, 2016

Copy link
Copy Markdown
Contributor Author

I updated :)

Trott commented Sep 13, 2016

Copy link
Copy Markdown
Member

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

jun-oka commented Sep 13, 2016

Copy link
Copy Markdown
Contributor Author

Hmm error again.

Trott commented Sep 13, 2016

Copy link
Copy Markdown
Member

Errors in CI are unrelated build issues and one known flaky.

Trott pushed a commit to Trott/io.js that referenced this pull request Sep 13, 2016
test: refector parallel/test-http.js
7f2c9ba 
* favor ’===’ over in ’==’
* favor ’assert.strictEqual’ over ’assert.equal’
* favor ’const’ over ’var’

PR-URL: nodejs#8471
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

Trott commented Sep 13, 2016

Copy link
Copy Markdown
Member

Landed in 7f2c9ba

Trott closed this Sep 13, 2016

jun-oka commented Sep 13, 2016

Copy link
Copy Markdown
Contributor Author

Thank you Trott

Fishrock123 pushed a commit that referenced this pull request Sep 14, 2016
test: refector parallel/test-http.js
3358861 
* favor ’===’ over in ’==’
* favor ’assert.strictEqual’ over ’assert.equal’
* favor ’const’ over ’var’

PR-URL: #8471
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Oct 6, 2016
test: refector parallel/test-http.js
7b58f32 
* favor ’===’ over in ’==’
* favor ’assert.strictEqual’ over ’assert.equal’
* favor ’const’ over ’var’

PR-URL: #8471
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Oct 10, 2016
test: refector parallel/test-http.js
5186253 
* favor ’===’ over in ’==’
* favor ’assert.strictEqual’ over ’assert.equal’
* favor ’const’ over ’var’

PR-URL: #8471
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
rvagg pushed a commit that referenced this pull request Oct 18, 2016
test: refector parallel/test-http.js
cbff429 
* favor ’===’ over in ’==’
* favor ’assert.strictEqual’ over ’assert.equal’
* favor ’const’ over ’var’

PR-URL: #8471
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Oct 26, 2016
test: refector parallel/test-http.js
8da2dcb 
* favor ’===’ over in ’==’
* favor ’assert.strictEqual’ over ’assert.equal’
* favor ’const’ over ’var’

PR-URL: #8471
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins mentioned this pull request Oct 26, 2016
Closed
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

No reviews

Assignees

No one assigned

Labels

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

Back | FazBrowse Home | New Git URL