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 with template literals #14280

Closed
wants to merge 1 commit into from
Closed

test: replace string concatenation with template literals #14280

wants to merge 1 commit into from

Conversation

jankjn
Copy link
Contributor

@jankjn jankjn commented Jul 16, 2017

from Workshop Code + Learn
replace string concatenation in test/parallel/test-require-extensions-same-filename-as-dir.js with template literals.

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Jul 16, 2017
@Trott Trott added the code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. label Jul 16, 2017
Trott
Trott requested changes Jul 16, 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.

Maybe this instead?:

const fixturesDir = common.fixturesDir;
const content = require(
  `${fixturesDir}/json-with-directory-name-module/module-stub/one/two/three.js`
);

@TimothyGu TimothyGu added code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. and removed code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. labels Jul 16, 2017
@jankjn
Copy link
Contributor Author

jankjn commented Jul 16, 2017 •
edited
Loading

Yeah, this looks much better, I've changed it @Trott

Trott
Trott approved these changes Jul 16, 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

@tniessen tniessen self-assigned this Jul 16, 2017
@vsemozhetbyt
Copy link
Contributor

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

refack
refack reviewed Jul 16, 2017
View reviewed changes
test/parallel/test-require-extensions-same-filename-as-dir.js Outdated
@@ -23,8 +23,10 @@
const common = require('../common');
const assert = require('assert');

const content = require(common.fixturesDir +
'/json-with-directory-name-module/module-stub/one/two/three.js');
const fixturesDir = common.fixturesDir;
Copy link
Contributor

Choose a reason for hiding this comment

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

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

const filePath = path.join(
  common.fixturesDir,
  'json-with-directory-name-module',
  'module-stub',
  'one-trailing-slash',
  'two',
  'three.js'
);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for suggestion, I like the path.join way

@Trott
Copy link
Member

Trott commented Jul 16, 2017

CI was green but I canceled some of the slower hosts to help with our current CI backlog problem.

@jankjn Are you interested in implementing the path.join() suggestion from @refack?

@jankjn
Copy link
Contributor Author

jankjn commented Jul 17, 2017

Thanks for @refack 's suggestion, I've implemented it @Trott.

@gireeshpunathil
Copy link
Member

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 restart aborted CI runs?

@Trott
Copy link
Member

Trott commented Jul 17, 2017

Assuming that CI is healed from back pressure (how to verify that? I see only 5 jobs in the pipeline)

@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 code-and-learn PRs), I'd try to keep it to about five jobs in the pipeline.

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
Copy link
Member

Trott commented Jul 17, 2017

Also, is it possible to restart aborted CI runs?

@gireeshpunathil I don't think so, but maybe someone in @nodejs/build knows otherwise...

@gibfahn
Copy link
Member

gibfahn commented Jul 17, 2017

Also, is it possible to restart aborted CI runs?

No. The closest you can get is the Rebuild plugin (nodejs/build#672).

refack
refack approved these changes Jul 17, 2017
View reviewed changes
Copy link
Contributor

@refack refack left a comment

Choose a reason for hiding this comment

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

💯
Thank you!

@Trott Trott changed the title test: replace string concatenation with template liberals test: replace string concatenation with template literals Jul 18, 2017
refack pushed a commit to refack/node that referenced this pull request Jul 19, 2017
@jankjn @refack
test: use path.join for long path concatenation
8ddb725 
PR-URL: nodejs#14280
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
@refack
Copy link
Contributor

refack commented Jul 19, 2017

Landed in 8ddb725

@refack refack closed this Jul 19, 2017
addaleax pushed a commit that referenced this pull request Jul 22, 2017
@jankjn @addaleax
test: use path.join for long path concatenation
66406ce 
PR-URL: #14280
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Fishrock123 pushed a commit that referenced this pull request Jul 24, 2017
@jankjn @Fishrock123
test: use path.join for long path concatenation
c866c90 
PR-URL: #14280
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
@addaleax addaleax mentioned this pull request Jul 24, 2017
Closed
@addaleax addaleax mentioned this pull request Aug 2, 2017
Merged
MylesBorins pushed a commit that referenced this pull request Aug 16, 2017
@jankjn @MylesBorins
test: use path.join for long path concatenation
c1dee46 
PR-URL: #14280
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Aug 16, 2017
Merged
MylesBorins pushed a commit that referenced this pull request Aug 16, 2017
@jankjn @MylesBorins
test: use path.join for long path concatenation
bc1d4c5 
PR-URL: #14280
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
MylesBorins pushed a commit that referenced this pull request Sep 3, 2017
@jankjn @MylesBorins
test: use path.join for long path concatenation
4d5a21c 
PR-URL: #14280
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
MylesBorins pushed a commit that referenced this pull request Sep 5, 2017
@jankjn @MylesBorins
test: use path.join for long path concatenation
bbd1c79 
PR-URL: #14280
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
This was referenced Sep 5, 2017
Closed
Closed
Closed
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gibfahn gibfahn gibfahn approved these changes

@gireeshpunathil gireeshpunathil gireeshpunathil approved these changes

@tniessen tniessen tniessen approved these changes

@cjihrig cjihrig cjihrig approved these changes

@targos targos targos approved these changes

@Trott Trott Trott approved these changes

@jasnell jasnell jasnell approved these changes

@refack refack refack approved these changes

Assignees

@tniessen tniessen

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