test: replace string concatenation in async-hooks/test-tlswrap.js with template literals#14319
Uh oh!
There was an error while loading. Please reload this page.
Conversation
nodejs-github-bot added async_hooks 
vincentcn changed the title 
Trott left a comment
Trott left a comment There was a problem hiding this comment.
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 commented Jul 17, 2017
Thanks @Trott . |
test/async-hooks/test-tlswrap.js Outdated
There was a problem hiding this comment.
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().
There was a problem hiding this comment.
I agree (even though this will work as far as I know).
There was a problem hiding this comment.
Yes, it is better remove the delimiters. Will update it.
Thanks.
…of template literals
vincentcn commented Jul 17, 2017
Updated accordingly. |
vsemozhetbyt commented Jul 17, 2017
test/async-hooks/test-tlswrap.js Outdated
| if(!common.hasCrypto) | ||
| common.skip('missing crypto'); | ||
| constpath=require('path'); |
There was a problem hiding this comment.
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 left a comment
Trott left a comment There was a problem hiding this comment.
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 commented Jul 17, 2017
@Trott Thanks. |
AndreasMadsen added the code-and-learn 
vsemozhetbyt commented Jul 17, 2017
test/async-hooks/test-tlswrap.js Outdated
There was a problem hiding this comment.
If this extra empty line is unwanted, anybody who will land the PR can delete it.
Deleted. |
Trott commented Jul 21, 2017
tniessen commented Jul 21, 2017
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]>
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 commented Jul 24, 2017
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]>
11 participants
[JSConf CN Code&Learn] Replace string concatenation in
async-hooks/test-tlswrap.jswith template literalsChecklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
no.
Just update tests.