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:parallel: cleanup test-child-process-exec-env #8600

Closed

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

} else {
success_count++;
assert.equal(true, stdout != '');
assert.strictEqual(true, stdout != '');
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 <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com>
@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 <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com>
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