https: made some improvements to test-https-agent.js #8517
Closed
Conversation
This file contains 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
This is my first good contribution, on line 40 replaced '==' with '===' for a strict compare, on line 52 I replaces 'assert.equal' with 'assert.strictEqual' again for a strict compare. Added some comments to make it clear what the test is doing and changed 'var' to 'const' where possible.
nodejs-github-bot
added
the
meta
lpinca
added
test
| @@ -45,6 +45,7 @@ ipch/ | |||
| *.VC.opendb | |||
| .vs/ | |||
| .vscode/ | |||
| .idea/ | |||
There was a problem hiding this comment.
Please don't change .gitignore. There is another PR specifically for this and it is still being discussed.
There was a problem hiding this comment.
If you need .idea/ in your .gitignore, consider adding it to a personal global .gitignore file instead. https://help.github.com/articles/ignoring-files/#create-a-global-gitignore
|
Nit: Keep the first line of the commit message to under 50 chars. I would suggest this:
|
|
Aside from @lpinca's comments and barring anything discovered in CI, the changes look good to me. |
|
LGTM once the comments are addressed. |
Removed redundant comments, and removed .idea from gitignore
|
LGTM |
| @@ -37,7 +38,7 @@ server.listen(0, function() { | |||
| }, function(res) { | |||
| res.resume(); | |||
| console.log(res.statusCode); | |||
There was a problem hiding this comment.
What about replacing this line with assert.strictEqual(res.statusCode, 200);?
Replaced console.log(res.statusCode); with and assertion, Rather than logging the https request status on every loop it will now assert the https status is correct on every loop.
Hey, should we label this as "good first contribution"? |
lpinca
reviewed
Sep 15, 2016
| console.log(res.statusCode); | ||
| if (++responses == N * M) server.close(); | ||
| assert.strictEqual(res.statusCode, 200); | ||
| if (++responses === N * M) server.close(); | ||
| }).on('error', function(e) { |
There was a problem hiding this comment.
I would change this to on('error', common.fail) or on('error', function(e) { throw e; }); not sure what is the preferred way, looks like both are used in other tests.
|
Left one last comment, then I think we are ready to go. |
|
@yorkie is that label used for PRs? I though it was for issues only. |
|
@lpinca I dunno too, never mind :) |
Changed the error listener to throw the error rather than log it.
Trott
pushed a commit
to Trott/io.js
that referenced
this pull request
Sep 16, 2016
On line 40: replace '==' with '===' On line 52: replace 'assert.equal' with 'assert.strictEqual' Added some comments. Changed 'var' to 'const' where possible. Replaced console.log(res.statusCode); with and assertion. Rather than logging the https request status on every loop it will now assert the https status is correct on every loop. Changed the error listener to throw the error rather than log it. PR-URL: nodejs#8517 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
|
Landed in a89c23f |
MylesBorins
pushed a commit
that referenced
this pull request
Oct 6, 2016
On line 40: replace '==' with '===' On line 52: replace 'assert.equal' with 'assert.strictEqual' Added some comments. Changed 'var' to 'const' where possible. Replaced console.log(res.statusCode); with and assertion. Rather than logging the https request status on every loop it will now assert the https status is correct on every loop. Changed the error listener to throw the error rather than log it. PR-URL: #8517 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
MylesBorins
pushed a commit
that referenced
this pull request
Oct 10, 2016
On line 40: replace '==' with '===' On line 52: replace 'assert.equal' with 'assert.strictEqual' Added some comments. Changed 'var' to 'const' where possible. Replaced console.log(res.statusCode); with and assertion. Rather than logging the https request status on every loop it will now assert the https status is correct on every loop. Changed the error listener to throw the error rather than log it. PR-URL: #8517 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Fishrock123
pushed a commit
that referenced
this pull request
Oct 11, 2016
On line 40: replace '==' with '===' On line 52: replace 'assert.equal' with 'assert.strictEqual' Added some comments. Changed 'var' to 'const' where possible. Replaced console.log(res.statusCode); with and assertion. Rather than logging the https request status on every loop it will now assert the https status is correct on every loop. Changed the error listener to throw the error rather than log it. PR-URL: #8517 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
rvagg
pushed a commit
that referenced
this pull request
Oct 18, 2016
On line 40: replace '==' with '===' On line 52: replace 'assert.equal' with 'assert.strictEqual' Added some comments. Changed 'var' to 'const' where possible. Replaced console.log(res.statusCode); with and assertion. Rather than logging the https request status on every loop it will now assert the https status is correct on every loop. Changed the error listener to throw the error rather than log it. PR-URL: #8517 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
MylesBorins
pushed a commit
that referenced
this pull request
Oct 26, 2016
On line 40: replace '==' with '===' On line 52: replace 'assert.equal' with 'assert.strictEqual' Added some comments. Changed 'var' to 'const' where possible. Replaced console.log(res.statusCode); with and assertion. Rather than logging the https request status on every loop it will now assert the https status is correct on every loop. Changed the error listener to throw the error rather than log it. PR-URL: #8517 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Closed
This pull request was closed.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Checklist
make -j4 test(UNIX), orvcbuild test nosign(Windows) passesAffected core subsystem(s)
https test
Description of change
This is my first good contribution, on line 40 replaced '==' with '===' for a strict compare, on line 52 I replaces 'assert.equal' with 'assert.strictEqual' again for a strict compare.
Added some comments to make it clear what the test is doing and changed 'var' to 'const' where possible.
Also added .idea to the gitignore file