test: replace string concatenation in async-hooks/test-tlswrap.js with template literals by vincentcn · Pull Request #14319 · nodejs/node

vincentcn · 2017-07-17T05:09:58Z

[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.

Trott

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 · 2017-07-17T06:17:09Z

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

vsemozhetbyt · 2017-07-17T08:55:04Z

test/async-hooks/test-tlswrap.js

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().

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

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

…of template literals

vincentcn · 2017-07-17T09:24:41Z

Updated accordingly.
Thanks.

vsemozhetbyt · 2017-07-17T09:26:50Z

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

Trott · 2017-07-17T10:14:47Z

test/async-hooks/test-tlswrap.js

 if (!common.hasCrypto)
  common.skip('missing crypto');

+const path = require('path');


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

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 · 2017-07-17T15:05:10Z

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

Thanks.

vsemozhetbyt · 2017-07-17T17:29:27Z

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

vsemozhetbyt · 2017-07-17T17:37:50Z

test/async-hooks/test-tlswrap.js

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

vincentcn · 2017-07-18T01:41:04Z

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

Deleted.

Trott · 2017-07-21T06:06:23Z

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

tniessen · 2017-07-21T08:12:23Z

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

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 · 2017-07-24T01:52:03Z

Thanks.

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]>

Replace string concatenation in with template literals

4d8489d

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 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 approved these changes Jul 17, 2017

View reviewed changes

gireeshpunathil approved these changes Jul 17, 2017

View reviewed changes

benjamingr approved these changes Jul 17, 2017

View reviewed changes

tniessen approved these changes Jul 17, 2017

View reviewed changes

vsemozhetbyt reviewed Jul 17, 2017

View reviewed changes

update tests in async-hooks/test-tlswrap.js to just path.join istead …

707e50a

…of template literals

vincentcn force-pushed the testfix branch from c06c3b0 to 707e50a Compare July 17, 2017 09:21

vsemozhetbyt approved these changes Jul 17, 2017

View reviewed changes

Trott reviewed Jul 17, 2017

View reviewed changes

Trott approved these changes Jul 17, 2017

View reviewed changes

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

lpinca approved these changes Jul 17, 2017

View reviewed changes

jasnell approved these changes Jul 17, 2017

View reviewed changes

vsemozhetbyt reviewed Jul 17, 2017

View reviewed changes

cjihrig approved these changes Jul 17, 2017

View reviewed changes

re-order the imports of buildin modules in alphabetical order

8a08a43

vincentcn force-pushed the testfix branch from 5f729eb to 8a08a43 Compare July 18, 2017 01:39

AndreasMadsen added the dont-land-on-v6.x label Jul 18, 2017

tniessen closed this Jul 21, 2017

addaleax mentioned this pull request Jul 24, 2017

v8.3.0 proposal #14452

Closed

addaleax mentioned this pull request Aug 2, 2017

v8.3.0 proposal #14594

Merged

Uh oh!

Conversation

vincentcn commented Jul 17, 2017 • edited by tniessen Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Uh oh!

Trott left a comment

Choose a reason for hiding this comment

Uh oh!

vincentcn commented Jul 17, 2017

Uh oh!

vsemozhetbyt Jul 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tniessen Jul 17, 2017

Choose a reason for hiding this comment

Uh oh!

vincentcn Jul 17, 2017

Choose a reason for hiding this comment

Uh oh!

vincentcn commented Jul 17, 2017

Uh oh!

vsemozhetbyt commented Jul 17, 2017

Uh oh!

Trott Jul 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Trott left a comment

Choose a reason for hiding this comment

Uh oh!

vincentcn commented Jul 17, 2017

Uh oh!

vsemozhetbyt commented Jul 17, 2017

Uh oh!

vsemozhetbyt Jul 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vincentcn commented Jul 18, 2017 • edited by gibfahn Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Trott commented Jul 21, 2017

Uh oh!

tniessen commented Jul 21, 2017

Uh oh!

vincentcn commented Jul 24, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

vincentcn commented Jul 17, 2017 •

edited by tniessen

Loading

vsemozhetbyt Jul 17, 2017 •

edited

Loading

Trott Jul 17, 2017 •

edited

Loading

vsemozhetbyt Jul 17, 2017 •

edited

Loading

vincentcn commented Jul 18, 2017 •

edited by gibfahn

Loading