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: remove duplicate required module #9169

Closed
wants to merge 3 commits into from
Closed

Conversation

Trott
Copy link
Member

@Trott Trott commented Oct 18, 2016

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

test

Description of change

common is required twice. Remove second require() statement.

@Trott Trott added the test Issues and PRs related to the tests. label Oct 18, 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

assert.equal(error, null);
assert.equal(stderr, '');
exec(`ps -p ${process.pid} -o args=`, function callback(error, stdout, stderr) {
assert.strictEqual(error, null);
Copy link
Member

Choose a reason for hiding this comment

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

Nit: assert.ifError()?

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

@mscdex mscdex added the process Issues and PRs related to the process subsystem. label Oct 18, 2016
@Trott
Copy link
Member Author

Trott commented Oct 18, 2016

Trott added a commit to Trott/io.js that referenced this pull request Oct 21, 2016
`common` is required twice in test-setproctitle.js. Remove one of the
instances.

Other refactoring:

* var -> const and let
* assert.equal -> assert.strictEqual
* assert.notEqual -> assert.notStrickEqual
* string concatenation -> template string
* use of assert.ifError() instead of asserting error is null

PR-URL: nodejs#9169
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@Trott
Copy link
Member Author

Trott commented Oct 21, 2016

Landed in 3184285

@Trott Trott closed this Oct 21, 2016
jasnell pushed a commit that referenced this pull request Oct 24, 2016
`common` is required twice in test-setproctitle.js. Remove one of the
instances.

Other refactoring:

* var -> const and let
* assert.equal -> assert.strictEqual
* assert.notEqual -> assert.notStrickEqual
* string concatenation -> template string
* use of assert.ifError() instead of asserting error is null

PR-URL: #9169
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Nov 15, 2016
`common` is required twice in test-setproctitle.js. Remove one of the
instances.

Other refactoring:

* var -> const and let
* assert.equal -> assert.strictEqual
* assert.notEqual -> assert.notStrickEqual
* string concatenation -> template string
* use of assert.ifError() instead of asserting error is null

PR-URL: #9169
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Nov 15, 2016
`common` is required twice in test-setproctitle.js. Remove one of the
instances.

Other refactoring:

* var -> const and let
* assert.equal -> assert.strictEqual
* assert.notEqual -> assert.notStrickEqual
* string concatenation -> template string
* use of assert.ifError() instead of asserting error is null

PR-URL: #9169
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
This was referenced Nov 22, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
process Issues and PRs related to the 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