Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Closed
wants to merge 4 commits into from
Closed

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

wants to merge 4 commits into from

Conversation

jun-oka
Copy link
Contributor

@jun-oka jun-oka commented Sep 9, 2016

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 nodejs-github-bot added the test Issues and PRs related to the tests. label Sep 9, 2016
@addaleax
Copy link
Member

addaleax commented Sep 9, 2016

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
Copy link
Contributor

cjihrig commented Sep 9, 2016

LGTM

@jun-oka
Copy link
Contributor Author

jun-oka commented Sep 9, 2016 •
edited
Loading

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

@addaleax
Copy link
Member

addaleax commented Sep 9, 2016

@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
Trott reviewed Sep 9, 2016
View reviewed changes
test/parallel/test-http.js
@@ -10,7 +10,7 @@ var body0 = '';
var body1 = '';

var server = http.Server(function(req, res) {
if (responses_sent == 0) {
if (responses_sent === 0) {
assert.equal('GET', req.method);
Copy link
Member

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

jun-oka commented Sep 9, 2016

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

@Trott
Copy link
Member

Trott commented Sep 9, 2016

LGTM if CI doesn't reveal any surprises.

@Trott
Copy link
Member

Trott commented Sep 9, 2016

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

@jun-oka
Copy link
Contributor Author

jun-oka commented Sep 9, 2016

It seems unrelated error, I hope.

@lpinca
Copy link
Member

lpinca commented Sep 10, 2016

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

thefourtheye
thefourtheye reviewed Sep 10, 2016
View reviewed changes
test/parallel/test-http.js
if (responses_sent == 0) {
assert.equal('GET', req.method);
assert.equal('/hello', url.parse(req.url).pathname);
const server = http.Server(function(req, res) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@thefourtheye I will finish it today.

@jasnell
Copy link
Member

jasnell commented Sep 12, 2016

LGTM

@jun-oka
Copy link
Contributor Author

jun-oka commented Sep 13, 2016

I updated :)

@Trott
Copy link
Member

Trott commented Sep 13, 2016

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

@jun-oka
Copy link
Contributor Author

jun-oka commented Sep 13, 2016

Hmm error again.

@Trott
Copy link
Member

Trott commented Sep 13, 2016

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
@jun-oka @Trott
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
Copy link
Member

Trott commented Sep 13, 2016

Landed in 7f2c9ba

@Trott Trott closed this Sep 13, 2016
@jun-oka
Copy link
Contributor Author

jun-oka commented Sep 13, 2016

Thank you Trott

Fishrock123 pushed a commit that referenced this pull request Sep 14, 2016
@jun-oka @Fishrock123
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
@jun-oka
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
@jun-oka
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
@jun-oka @rvagg
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
@jun-oka @MylesBorins
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 MylesBorins mentioned this pull request Oct 26, 2016
Closed
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
@jun-oka @addaleax @cjihrig @Trott @lpinca @jasnell @thefourtheye @MylesBorins @nodejs-github-bot