Skip to content
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

Jump to bottom

test: replace string concatenation in async-hooks/test-tlswrap.js with template literals #14319

Closed
wants to merge 3 commits into from
Closed

test: replace string concatenation in async-hooks/test-tlswrap.js with template literals #14319

wants to merge 3 commits into from

Conversation

vincentcn
Copy link
Contributor

@vincentcn vincentcn commented Jul 17, 2017 •
edited by tniessen
Loading

[JSConf CN Code&Learn] Replace string concatenation in async-hooks/test-tlswrap.js with template literals

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

no.
Just update tests.

@nodejs-github-bot nodejs-github-bot added async_hooks Issues and PRs related to the async hooks subsystem. test Issues and PRs related to the tests. labels Jul 17, 2017
@vincentcn vincentcn changed the title Replace string concatenation in async-hooks/test-tlswrap.js with template literals test: replace string concatenation in async-hooks/test-tlswrap.js with template literals Jul 17, 2017
Trott
Trott approved these changes Jul 17, 2017
View reviewed changes
Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

LGTM if CI is green. If you want to take it a step further, you can replace the template literals with path.join() calls. (For example: path.join(common.fixturesDir, 'test_cert.pem'))

@vincentcn
Copy link
Contributor Author

Thanks @Trott .
Yes, path.join is more suitable in this case.
Updated it to use path.join instead of template literals just now.

vsemozhetbyt
vsemozhetbyt reviewed Jul 17, 2017
View reviewed changes
test/async-hooks/test-tlswrap.js Outdated
@@ -19,8 +20,8 @@ hooks.enable();
//
const server = tls
.createServer({
cert: fs.readFileSync(common.fixturesDir + '/test_cert.pem'),
key: fs.readFileSync(common.fixturesDir + '/test_key.pem')
cert: fs.readFileSync(path.join(common.fixturesDir, '/test_cert.pem')),
Copy link
Contributor

@vsemozhetbyt vsemozhetbyt Jul 17, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Should not the '/test_cert.pem' and '/test_key.pem' be the 'test_cert.pem' and 'test_key.pem'? Path delimiters shoud be inserted by path.join().

Copy link
Member

Choose a reason for hiding this comment

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

I agree (even though this will work as far as I know).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it is better remove the delimiters. Will update it.
Thanks.

@vincentcn
Copy link
Contributor Author

Updated accordingly.
Thanks.

@vsemozhetbyt
Copy link
Contributor

CI: https://ci.nodejs.org/job/node-test-pull-request/9190/

Trott
Trott reviewed Jul 17, 2017
View reviewed changes
test/async-hooks/test-tlswrap.js Outdated
@@ -4,6 +4,7 @@ const common = require('../common');
if (!common.hasCrypto)
common.skip('missing crypto');

const path = require('path');
Copy link
Member

@Trott Trott Jul 17, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

This is a nit pick, but can you move path below assert? In tests, we try to (but admittedly often don't) keep the built-in modules in alphabetical order. (Well, ASCII order actually. https://github.com/nodejs/node/blob/master/doc/guides/writing-tests.md#lines-7-8)

Trott
Trott approved these changes Jul 17, 2017
View reviewed changes
Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

LGTM with or without my additional nit-pick addressed. (Nit-pick can be addressed while landing if it doesn't get addressed in this PR.)

@vincentcn
Copy link
Contributor Author

@Trott
Updated it to make the imports of build modules in alphabetical order.

Thanks.

@AndreasMadsen AndreasMadsen added the code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. label Jul 17, 2017
@vsemozhetbyt
Copy link
Contributor

CI: https://ci.nodejs.org/job/node-test-pull-request/9196/

vsemozhetbyt
vsemozhetbyt reviewed Jul 17, 2017
View reviewed changes
test/async-hooks/test-tlswrap.js Outdated
@@ -4,12 +4,15 @@ const common = require('../common');
if (!common.hasCrypto)
common.skip('missing crypto');


Copy link
Contributor

@vsemozhetbyt vsemozhetbyt Jul 17, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

If this extra empty line is unwanted, anybody who will land the PR can delete it.

@vincentcn
Copy link
Contributor Author

vincentcn commented Jul 18, 2017 •
edited by gibfahn
Loading

If this extra empty line is unwanted, anybody who will land the PR can delete it.

Deleted.

@Trott
Copy link
Member

Trott commented Jul 21, 2017

CI: https://ci.nodejs.org/job/node-test-pull-request/9280/

@tniessen
Copy link
Member

Landed in d8eb30a, thank you for your first contribution! 🎉

@tniessen tniessen closed this Jul 21, 2017
tniessen pushed a commit that referenced this pull request Jul 21, 2017
@vincentcn @tniessen
test: use path.join in async-hooks/test-tlswrap.js
d8eb30a 
PR-URL: #14319
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
addaleax pushed a commit that referenced this pull request Jul 22, 2017
@vincentcn @addaleax
test: use path.join in async-hooks/test-tlswrap.js
98bba54 
PR-URL: #14319
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
@vincentcn
Copy link
Contributor Author

Thanks.

Fishrock123 pushed a commit that referenced this pull request Jul 24, 2017
@vincentcn @Fishrock123
test: use path.join in async-hooks/test-tlswrap.js
3c92b78 
PR-URL: #14319
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
@addaleax addaleax mentioned this pull request Jul 24, 2017
Closed
@addaleax addaleax mentioned this pull request Aug 2, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vsemozhetbyt vsemozhetbyt vsemozhetbyt approved these changes

@tniessen tniessen tniessen approved these changes

@gireeshpunathil gireeshpunathil gireeshpunathil approved these changes

@cjihrig cjihrig cjihrig approved these changes

@lpinca lpinca lpinca approved these changes

@benjamingr benjamingr benjamingr approved these changes

@Trott Trott Trott approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
async_hooks Issues and PRs related to the async hooks subsystem. code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.