Skip to content

[jsconfcn] test: replace string concatenation #14295

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
Closed
wants to merge 1 commit into from
Closed

[jsconfcn] test: replace string concatenation #14295

wants to merge 1 commit into from

Conversation

4garfield
Copy link
Contributor

replace string-concation in test/async-hooks/test-signalwrap.js with template literals

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

@4garfield
test: replace string concatenation
b45f9a2 
replace string-concation in test/async-hooks/test-signalwrap.js with template literals
@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 16, 2017
@4garfield 4garfield changed the title test: replace string concatenation [jsconfcn] test: replace string concatenation 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 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

@targos targos mentioned this pull request Jul 16, 2017
Closed
4 tasks
@vsemozhetbyt
Copy link
Contributor

vsemozhetbyt commented Jul 16, 2017 •
edited
Loading

(It seems to be duplicate of #14269)
CI: https://ci.nodejs.org/job/node-test-pull-request/9157/

@vsemozhetbyt vsemozhetbyt marked this as a duplicate of #14269 Jul 16, 2017
@tniessen tniessen self-assigned this Jul 16, 2017
refack
refack previously requested changes Jul 16, 2017
View reviewed changes
test/async-hooks/test-signalwrap.js
@@ -22,7 +22,7 @@ assert.strictEqual(typeof signal1.triggerAsyncId, 'number');
checkInvocations(signal1, { init: 1 }, 'when SIGUSR2 handler is set up');

let count = 0;
exec('kill -USR2 ' + process.pid);
exec(`kill -USR2 ${process.pid}`);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should use process.kill(pid, signal) - https://nodejs.org/api/process.html#process_process_kill_pid_signal

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you @refack.
It's better to use the process.kill(pid, signal), will update once I'm avaliable.

@refack refack dismissed their stale review July 16, 2017 16:44

exec vs kill is not worth blocking for

@refack
Copy link
Contributor

refack commented Jul 16, 2017

@4garfield thank you very much for you contribution. "Change requests" are a normal part of the process. Personally I'd be very happy if you did follow up and made the code even better 👍

@Trott
Copy link
Member

Trott commented Jul 16, 2017

CI was green but I canceled it before the Raspberry Pi 1 devices finished in order to help with our current CI backlog problem.

Not going to land right now to give @4garfield a chance to implement @refack's suggestion.

tniessen pushed a commit that referenced this pull request Jul 23, 2017
@4garfield @tniessen
test: replace concatenation with template literals
651fc55 
Replace string concatenation in test/async-hooks/test-signalwrap.js
with template literals.

PR-URL: #14295
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
@tniessen
Copy link
Member

Landed in 651fc55, thank you for your contribution! 🎉

CI on master: https://ci.nodejs.org/job/node-test-commit-linuxone/7502/

If you want to follow @refack's suggestion, feel free to open a separate PR.

@tniessen tniessen closed this Jul 23, 2017
addaleax pushed a commit that referenced this pull request Jul 24, 2017
@4garfield @addaleax
test: replace concatenation with template literals
265f159 
Replace string concatenation in test/async-hooks/test-signalwrap.js
with template literals.

PR-URL: #14295
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
@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

@jasnell jasnell jasnell approved these changes

@Trott Trott Trott approved these changes

@benjamingr benjamingr benjamingr approved these changes

@targos targos targos approved these changes

@cjihrig cjihrig cjihrig approved these changes

@tniessen tniessen tniessen approved these changes

@BridgeAR BridgeAR BridgeAR approved these changes

+2 more reviewers

@TimothyGu TimothyGu TimothyGu approved these changes

@refack refack refack left review comments

Reviewers whose approvals may not affect merge requirements
Assignees

@tniessen tniessen

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.