Skip to content

Conversation

@sejoker
Copy link
Contributor

@sejoker sejoker commented Sep 17, 2016

Cleanup according to nodejs/code-and-learn#56

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

test

Description of change

replace equal with strictEqual, use const instead of var

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

@mscdex mscdex added child_process Issues and PRs related to the child_process subsystem. test Issues and PRs related to the tests. labels Sep 17, 2016
@cjihrig
Copy link
Contributor

cjihrig commented Sep 17, 2016

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 with a suggestion

Copy link
Member

Choose a reason for hiding this comment

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

This might also be just assert.notStrictEqual(stdout, '');?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@addaleax updated PR with assert.notStrictEqual

replace equal with strictEqual, use const instead of var,
improve test with use of assert.notStrictEqual
@sejoker sejoker force-pushed the test-child-process-exec-env-cleanup branch from 9aa994d to b69f522 Compare September 18, 2016 08:43
Copy link
Member

@lpinca lpinca left a comment

Choose a reason for hiding this comment

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

LGTM

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

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

@imyller imyller self-assigned this Sep 20, 2016
@imyller
Copy link
Member

imyller commented Sep 20, 2016

@imyller
Copy link
Member

imyller commented Sep 20, 2016

I'll start landing this:

  • Five LGTMs
  • No objections
  • CI tests passed (only the usual CI failures)

imyller pushed a commit to imyller/node that referenced this pull request Sep 20, 2016
Replace equal with strictEqual, use const instead of var and
improve test with use of assert.notStrictEqual

PR-URL: nodejs#8600
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ilkka Myller <[email protected]>
@imyller
Copy link
Member

imyller commented Sep 20, 2016

landed in 4019c32

Thank you for your contribution, @sejoker

@imyller imyller closed this Sep 20, 2016
@imyller imyller removed their assignment Sep 20, 2016
Fishrock123 pushed a commit that referenced this pull request Oct 11, 2016
Replace equal with strictEqual, use const instead of var and
improve test with use of assert.notStrictEqual

PR-URL: #8600
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ilkka Myller <[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.

8 participants