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.

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

test: cleanup test-child-process-stdio.js #8584

Closed

Conversation

pmatzavin
Copy link

@pmatzavin pmatzavin commented Sep 17, 2016

Checklist
  • make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)
Description of change
  • refactor vars to const and let

issue: nodejs/code-and-learn#56(comment)

modified file: test/parallel/test-child-process-stdio.js

refactor variable declaration "var common" to "const common"
refactor variable declaration "var assert" to "const assert"
refactor variable declaration "var options" to "let options"
refactor variable declaration "var child" to "let child"

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Sep 17, 2016
Copy link
Contributor

@eljefedelrodeodeljefe eljefedelrodeodeljefe 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 nit. Can you please add a description of change into the commit message and refer to the code and lean issue.

@mscdex mscdex added the child_process Issues and PRs related to the child_process subsystem. label Sep 17, 2016
- refactor vars to const and let

issue: nodejs/code-and-learn#56(comment)

modified file: test/parallel/test-child-process-stdio.js

refactor variable declaration "var common" to "const common"
refactor variable declaration "var assert" to "const assert"
refactor variable declaration "var options" to "let options"
refactor variable declaration "var child" to "let child"
@pmatzavin pmatzavin force-pushed the test-cleanup-test-child-process-stdio branch from 118807c to f8f92e4 Compare September 17, 2016 09:44
@pmatzavin
Copy link
Author

Added a more detailed description of change into the commit message, referring to the source code and the git issue.

var options = {stdio: ['pipe']};
var child = common.spawnPwd(options);
let options = {stdio: ['pipe']};
let child = common.spawnPwd(options);

assert.notEqual(child.stdout, null);
assert.notEqual(child.stderr, null);
Copy link
Member

Choose a reason for hiding this comment

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

You can change assert.notStrictEqual and assert.strictEqual in this test file, too :)

Copy link
Author

@pmatzavin pmatzavin Sep 18, 2016

Choose a reason for hiding this comment

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

@addaleax If i understood that correctly:

Do you mean that i should replace equal with strictEqual and replace notEqual with notStrictEqual ?

In that case will there be any inconsistencies because of the fact that the assertions in this file are against null and:
undefined === null // equals false
undefined !== null // equals true

For example:
assert.notStrictEqual(undefined, null) // this will pass
assert.strictEqual(undefined, null) // this will not pass

Copy link
Member

Choose a reason for hiding this comment

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

@pmatzavin Sorry for the late reply! Yes, that’s what I meant. Just to be clear, notStrictEqual corresponds to !== and strictEqual to ===, and we usually do want the strict tests exactly to catch things like accidentally relying on undefined == null.

issue: nodejs/code-and-learn#56(comment)

modified file: test/parallel/test-child-process-stdio.js

Replace the equal assetion with strictEqual.
Replace the notEqual assertion with strictNotEqual.
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

LGTM

@addaleax
Copy link
Member

Copy link
Member

@jasnell jasnell 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 pushed a commit that referenced this pull request Sep 22, 2016
Refs: nodejs/code-and-learn#56(comment)

Replace the equal assetion with strictEqual.
Replace the notEqual assertion with strictNotEqual.

PR-URL: #8584
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Robert Jefe Lindstaedt <[email protected]>
@jasnell
Copy link
Member

jasnell commented Sep 22, 2016

Landed in 078bf68. Thank you!

@jasnell jasnell closed this Sep 22, 2016
jasnell pushed a commit that referenced this pull request Sep 29, 2016
Refs: nodejs/code-and-learn#56(comment)

Replace the equal assetion with strictEqual.
Replace the notEqual assertion with strictNotEqual.

PR-URL: #8584
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Robert Jefe Lindstaedt <[email protected]>
Fishrock123 pushed a commit that referenced this pull request Oct 11, 2016
Refs: nodejs/code-and-learn#56(comment)

Replace the equal assetion with strictEqual.
Replace the notEqual assertion with strictNotEqual.

PR-URL: #8584
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Robert Jefe Lindstaedt <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
child_process Issues and PRs related to the child_process subsystem. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants