test: Updated tls-getcipher test#9923
Uh oh!
There was an error while loading. Please reload this page.
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.
Trott commented Dec 1, 2016
Ethan-Arrowood commented Dec 2, 2016
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? |
Trott commented Dec 2, 2016
@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/ |
Ethan-Arrowood commented Dec 2, 2016 via email
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
| varserver=tls.createServer(options, | ||
| constserver=tls.createServer(options, | ||
| common.mustCall(function(cleartextStream){})); |
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.
Ethan-Arrowood commented Dec 4, 2016
Silly question, but when does my approved PR's get closed? Will it be done automatically or is there something I need to do? |
cjihrig commented Dec 5, 2016
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/ |
Trott commented Dec 5, 2016
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/ |
| varcommon=require('../common'); | ||
| varassert=require('assert'); | ||
| constcommon=require('../common'); | ||
| constassert=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 <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]>
jasnell commented Dec 5, 2016
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 <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]>
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 <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]>
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 <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]>
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 <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]>
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.