test: Updated tls-getcipher test#9923
test: Updated tls-getcipher test#9923Ethan-Arrowood wants to merge 2 commits intonodejs:masterfrom Ethan-Arrowood:my-branch
Conversation
imyller
added
the
code-and-learn
Changed var to ES6 syntax const/let. Added common.mustCall function wrapper to all functions called exactly once. Changed assert.equal to assert.strictEqual.
|
What went wrong here? Why did it fail on the arm-fanned and windows-fanned tests? How I can I fix my code so it works on those? |
|
@Ethan-Arrowood Someone must have gotten alarmed at the number of CI jobs I started yesterday (due to Code & Learn) and thought they were all stalled jobs. So yours (and many others) were terminated. No problem, though! I'll just run yours again. CI: https://ci.nodejs.org/job/node-test-pull-request/5106/ |
|
Okay great thank you!
…On Fri, Dec 2, 2016 at 11:19 AM Rich Trott ***@***.***> wrote:
@Ethan-Arrowood <https://github.com/Ethan-Arrowood> Someone must have
gotten alarmed at the number of CI jobs I started yesterday (due to Code &
Learn) and thought they were all stalled jobs. So yours (and many others)
were terminated.
No problem, though! I'll just run yours again. CI:
https://ci.nodejs.org/job/node-test-pull-request/5106/
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#9923 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/APZXHleXp9xGlZ07LM2nXsz9QrJXow91ks5rEEUegaJpZM4LBmyy>
.
|
test/parallel/test-tls-getcipher.js
Outdated
|
|
||
| var server = tls.createServer(options, | ||
| const server = tls.createServer(options, | ||
| common.mustCall(function(cleartextStream) {})); |
There was a problem hiding this comment.
This needs to be lined up now.
There was a problem hiding this comment.
What do you mean by lined up? Sorry, still new to this!
There was a problem hiding this comment.
@Ethan-Arrowood The argument (common.mustCall...)on line 22 should line up with the argument on line 21 (options). So line 22 needs to be indented two more spaces. I'm surprised this isn't caught by the linter.
There was a problem hiding this comment.
Okay I went ahead and fixed that issue and made another commit
Very simple fix to indentation issue on line 22.
|
Silly question, but when does my approved PR's get closed? Will it be done automatically or is there something I need to do? |
|
It should only need a new CI run, and then someone with commit access will land it. New CI: https://ci.nodejs.org/job/node-test-pull-request/5184/ |
|
CI was fine except that Windows stalled (on the CI side, nothing to do with this change). Re-running just Windows: https://ci.nodejs.org/job/node-test-commit-windows-fanned/5657/ |
| var common = require('../common'); | ||
| var assert = require('assert'); | ||
| const common = require('../common'); | ||
| const assert = require('assert'); |
There was a problem hiding this comment.
Nit: can you please move this require after the hasCrypto check?
Changed var to ES6 syntax const/let. Added common.mustCall function wrapper to all functions called exactly once. Changed assert.equal to assert.strictEqual. PR-URL: #9923 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
|
Landed in be1f52b. Thank you for the PR and for participating in the code-and-learn! |
Changed var to ES6 syntax const/let. Added common.mustCall function wrapper to all functions called exactly once. Changed assert.equal to assert.strictEqual. PR-URL: #9923 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
Changed var to ES6 syntax const/let. Added common.mustCall function wrapper to all functions called exactly once. Changed assert.equal to assert.strictEqual. PR-URL: nodejs#9923 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
Changed var to ES6 syntax const/let. Added common.mustCall function wrapper to all functions called exactly once. Changed assert.equal to assert.strictEqual. PR-URL: nodejs#9923 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
Changed var to ES6 syntax const/let. Added common.mustCall function wrapper to all functions called exactly once. Changed assert.equal to assert.strictEqual. PR-URL: #9923 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
10 participants
Checklist
make -j8 test(UNIX), orvcbuild test nosign(Windows) passesAffected core subsystem(s)
tests
Description of change
Changed var to ES6 syntax const/let.
Added common.mustCall function wrapper to all functions called exactly once.
Changed assert.equal to assert.strictEqual.