test: replace string concatenation with template literals#14280
Uh oh!
There was an error while loading. Please reload this page.
Conversation
Trott added the code-and-learn 
Trott left a comment
Trott left a comment There was a problem hiding this comment.
Maybe this instead?:
constfixturesDir=common.fixturesDir;constcontent=require(`${fixturesDir}/json-with-directory-name-module/module-stub/one/two/three.js`);
TimothyGu added code-and-learn 
Yeah, this looks much better, I've changed it @Trott |
vsemozhetbyt commented Jul 16, 2017
There was a problem hiding this comment.
This is Ok.
But it would be much better to use path.join instead, see #14272 (comment)
You could even do it with multiple parts
constfilePath=path.join(common.fixturesDir,'json-with-directory-name-module','module-stub','one-trailing-slash','two','three.js');There was a problem hiding this comment.
Thanks for suggestion, I like the path.join way
Trott commented Jul 16, 2017
jankjn commented Jul 17, 2017
gireeshpunathil commented Jul 17, 2017
Assuming that CI is healed from back pressure (how to verify that? I see only 5 jobs in the pipeline) issued a fresh one: https://ci.nodejs.org/job/node-test-pull-request/9183/ Also, is it possible to |
Trott commented Jul 17, 2017
@gireeshpunathil It's subjective. Things get ugly in the CI interface after 15 jobs are in the pipeline, so I would take that as an absolute hard limit. But even at 10 jobs, it means delaying other people who are trying to use CI by potentially hours. So for small changes (like most I'd also note that the 5-job recommendation above is taking into account that our CI is much slower right now than is usually is (because Node.js startup is much slower now than it usually is due to the temporary disabling of V8 snapshots in Node.js). In more typical times, I'd probably put that suggested limit to more like 8 or 10 jobs. |
Trott commented Jul 17, 2017
@gireeshpunathil I don't think so, but maybe someone in @nodejs/build knows otherwise... |
gibfahn commented Jul 17, 2017
No. The closest you can get is the Rebuild plugin (nodejs/build#672). |
Trott changed the title 
PR-URL: nodejs#14280 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>
refack commented Jul 19, 2017
Landed in 8ddb725 |
PR-URL: #14280 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>
PR-URL: #14280 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>
PR-URL: #14280 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>
PR-URL: #14280 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>
PR-URL: #14280 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>
PR-URL: #14280 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>
13 participants
from Workshop Code + Learn
replace string concatenation in test/parallel/test-require-extensions-same-filename-as-dir.js with template literals.