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: clean up test-child-process-exec-cwd.js #9231

Closed
wants to merge 1 commit into from
Closed

test: clean up test-child-process-exec-cwd.js #9231

wants to merge 1 commit into from

Conversation

jeenalee
Copy link
Contributor

Checklist
  • make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

test

Description of change

This patch provides minor fixes to test-child-process-exec-cwd.js. Notably:

  • Changed == to ===.
  • Changed var to const where possible.

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Oct 21, 2016
@jeenalee
Copy link
Contributor Author

cc @Trott

@mscdex mscdex added the child_process Issues and PRs related to the child_process subsystem. label Oct 21, 2016
cjihrig
cjihrig reviewed Oct 21, 2016
View reviewed changes
test/parallel/test-child-process-exec-cwd.js
@@ -15,5 +15,5 @@ if (common.isWindows) {

exec(pwdcommand, {cwd: dir}, common.mustCall(function(err, stdout, stderr) {
assert.ifError(err);
assert.ok(stdout.indexOf(dir) == 0);
assert.ok(stdout.indexOf(dir) === 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

This would probably make more sense as assert.strictEqual(stdout.indexOf(dir), 0);

cjihrig
cjihrig approved these changes Oct 21, 2016
View reviewed changes
Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

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

LGTM with a suggestion.

Trott
Trott approved these changes Oct 21, 2016
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. I like @cjihrig's suggestion.

@Trott
Copy link
Member

Trott commented Oct 21, 2016

Nit: Clean-up in commit message to clean up. (Whoever lands the change can fix that up, but if you want to save them some keystrokes, go for it.)

@jeenalee
Copy link
Contributor Author

Review comments, including editing commit message, addressed. Thanks everyone!

@jeenalee
test: clean up test-child-process-exec-cwd.js
381a524 
- Changed `assert.ok()` to `assert.strictEqual()`.
- Changed `var` to `const` where possible.
@jeenalee jeenalee changed the title test: Clean-up test-child-process-exec-cwd.js test: clean up test-child-process-exec-cwd.js Oct 22, 2016
@Trott
Copy link
Member

Trott commented Oct 22, 2016

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

imyller
imyller approved these changes Oct 24, 2016
View reviewed changes
Copy link
Member

@imyller imyller left a comment

Choose a reason for hiding this comment

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

LGTM

@jasnell
Copy link
Member

jasnell commented Oct 25, 2016

too much red in that last CI run.. trying again: https://ci.nodejs.org/job/node-test-pull-request/4667/

Trott pushed a commit to Trott/io.js that referenced this pull request Oct 26, 2016
@jeenalee @Trott
test: clean up test-child-process-exec-cwd.js
2d472a3 
- Changed `assert.ok()` to `assert.strictEqual()`.
- Changed `var` to `const` where possible.

PR-URL: nodejs#9231
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com>
@Trott
Copy link
Member

Trott commented Oct 26, 2016

Landed in 2d472a3

@Trott Trott closed this Oct 26, 2016
evanlucas pushed a commit that referenced this pull request Nov 2, 2016
@jeenalee @evanlucas
test: clean up test-child-process-exec-cwd.js
53520f0 
- Changed `assert.ok()` to `assert.strictEqual()`.
- Changed `var` to `const` where possible.

PR-URL: #9231
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com>
@evanlucas evanlucas mentioned this pull request Nov 3, 2016
Merged
MylesBorins pushed a commit that referenced this pull request Nov 18, 2016
@jeenalee
test: clean up test-child-process-exec-cwd.js
1e0a8d7 
- Changed `assert.ok()` to `assert.strictEqual()`.
- Changed `var` to `const` where possible.

PR-URL: #9231
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com>
MylesBorins pushed a commit that referenced this pull request Nov 19, 2016
@jeenalee @MylesBorins
test: clean up test-child-process-exec-cwd.js
5d4f757 
- Changed `assert.ok()` to `assert.strictEqual()`.
- Changed `var` to `const` where possible.

PR-URL: #9231
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com>
MylesBorins pushed a commit that referenced this pull request Nov 22, 2016
@jeenalee
test: clean up test-child-process-exec-cwd.js
8d985c2 
- Changed `assert.ok()` to `assert.strictEqual()`.
- Changed `var` to `const` where possible.

PR-URL: #9231
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com>
@MylesBorins MylesBorins mentioned this pull request Nov 22, 2016
Merged
@snyk-bot snyk-bot mentioned this pull request Jan 11, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cjihrig cjihrig cjihrig approved these changes

@imyller imyller imyller approved these changes

@lpinca lpinca lpinca approved these changes

@Trott Trott Trott approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
child_process Issues and PRs related to the child_process subsystem. test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@jeenalee @Trott @jasnell @lpinca @imyller @cjihrig @mscdex @MylesBorins @nodejs-github-bot