Skip to content

test-tls-check-server-identity var->const, assert, string template #9986

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.

Sign up for GitHub

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

Closed
koxauvin wants to merge 3 commits into from
Closed

test-tls-check-server-identity var->const, assert, string template #9986

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9b0c4cd
test: update test-tls-check-server-identity.js
koxauvin Dec 1, 2016
a55a9ac
test: update test-tls-check-server-identity.js
koxauvin Dec 5, 2016
7b341c3
test: update test-tls-check-server-identity.js
koxauvin Dec 5, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
test: update test-tls-check-server-identity.js 
Changed var to const, assert.equal to assert.strictEqual, and
used a template string for error output.
  • Loading branch information
@koxauvin @Trott
koxauvin authored and Trott committed Dec 24, 2016
commit 9b0c4cd8078ea4023f2020beb4a0c2a4aebeb86f
20 changes: 10 additions & 10 deletions test/parallel/test-tls-check-server-identity.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,16 +1,16 @@
'use strict';
var common = require('../common');
var assert = require('assert');
var util = require('util');
const common = require('../common');
const assert = require('assert');
Copy link
Copy Markdown
Member

@jasnell jasnell Dec 5, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please move the assert and util requires after the hasCrypto check

Copy link
Copy Markdown
Contributor Author

@koxauvin koxauvin Dec 5, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried removing the block
if (!common.hasCrypto) {
common.skip('missing crypto');
return;
}
but module common is not used "error 'common' is assigned a value but never used no-unused-vars".
Then when I remove the require for common I get "error Mandatory module "common" must be loaded required-modules"
The util requires in used in the string template, can't remove that.

Copy link
Copy Markdown
Member

@Trott Trott Dec 22, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@koxauvin Please don't remove the hasCrypto check. It is needed for this test. But please move the assignments to assert and util to be after that block.

const util = require('util');

if (!common.hasCrypto) {
common.skip('missing crypto');
return;
}
var tls = require('tls');
const tls = require('tls');


var tests = [
const tests = [
// False-y values.
{
host: false,
Expand Down Expand Up @@ -253,9 +253,9 @@ var tests = [
];

tests.forEach(function(test, i) {
var err = tls.checkServerIdentity(test.host, test.cert);
assert.equal(err && err.reason,
test.error,
'Test#' + i + ' failed: ' + util.inspect(test) + '\n' +
test.error + ' != ' + (err && err.reason));
const err = tls.checkServerIdentity(test.host, test.cert);
assert.strictEqual(err && err.reason,
test.error,
`Test# ${i} failed: ${util.inspect(test)} \n
${test.error} != ${(err && err.reason)}`);
Copy link
Copy Markdown
Member

@jasnell jasnell Dec 5, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please avoid multi-line template literals, especially given that the additional whitespace will be included in the string.

                     `Test# ${i} failed: ${util.inspect(test)} \n` +
                     `${test.error} != ${(err && err.reason)}`);

});