https: made some improvements to test-https-agent.js#8517
Conversation
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.
| *.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"? |
| 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.
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 |
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>
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