-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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.
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
test: Updated tls-getcipher test #9923
Conversation
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>
.
|
key: fs.readFileSync(common.fixturesDir + '/keys/agent2-key.pem'), | ||
cert: fs.readFileSync(common.fixturesDir + '/keys/agent2-cert.pem'), | ||
ciphers: cipher_list.join(':'), | ||
honorCipherOrder: true | ||
}; | ||
|
||
var server = tls.createServer(options, | ||
const server = tls.createServer(options, | ||
common.mustCall(function(cleartextStream) {})); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to be lined up now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you mean by lined up? Sorry, still new to this!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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>
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.