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: s/assert.notEqual()/assert.notStrictEqual()/ #10541

Merged
merged 1 commit into from
Jan 6, 2017
Merged

test: s/assert.notEqual()/assert.notStrictEqual()/ #10541

merged 1 commit into from
Jan 6, 2017

Conversation

cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Dec 30, 2016

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

test

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Dec 30, 2016
@cjihrig
Copy link
Contributor Author

cjihrig commented Dec 30, 2016

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

Trott
Trott approved these changes Dec 31, 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 if CI is ✅ . Left some nits, but they can be ignored and someone can do them later if they feel strongly about it.

test/parallel/test-console.js
@@ -115,7 +115,7 @@ assert.strictEqual("{ foo: 'bar', inspect: [Function: inspect] }\n",
strings.shift());
assert.strictEqual("{ foo: 'bar', inspect: [Function: inspect] }\n",
strings.shift());
assert.notEqual(-1, strings.shift().indexOf('foo: [Object]'));
assert.notStrictEqual(-1, strings.shift().indexOf('foo: [Object]'));
Copy link
Member

Choose a reason for hiding this comment

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

For readability, might be good to change this to assert(strings.shift().includes('foo: [Object]'));

test/parallel/test-crypto-fips.js
if (FIPS_ENABLED !== expectedOutput && FIPS_DISABLED !== expectedOutput) {
// In the case of expected errors just look for a substring.
assert.notEqual(-1, response.indexOf(expectedOutput));
assert.notStrictEqual(-1, response.indexOf(expectedOutput));
Copy link
Member

Choose a reason for hiding this comment

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

Might be good to use .includes() here and change assert.notStrictEqual() to assert().

test/parallel/test-module-loading-error.js
@@ -28,11 +28,11 @@ try {
try {
require();
} catch (e) {
assert.notEqual(e.toString().indexOf('missing path'), -1);
assert.notStrictEqual(e.toString().indexOf('missing path'), -1);
Copy link
Member

Choose a reason for hiding this comment

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

Same.

test/parallel/test-module-loading-error.js
}

try {
require({});
} catch (e) {
assert.notEqual(e.toString().indexOf('path must be a string'), -1);
assert.notStrictEqual(e.toString().indexOf('path must be a string'), -1);
Copy link
Member

Choose a reason for hiding this comment

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

Ditto.

test/parallel/test-net-remote-address-port.js
@@ -13,16 +13,19 @@ var remoteFamilyCandidates = ['IPv4'];
if (common.hasIPv6) remoteFamilyCandidates.push('IPv6');

var server = net.createServer(common.mustCall(function(socket) {
assert.notEqual(-1, remoteAddrCandidates.indexOf(socket.remoteAddress));
assert.notEqual(-1, remoteFamilyCandidates.indexOf(socket.remoteFamily));
assert.notStrictEqual(-1, remoteAddrCandidates.indexOf(socket.remoteAddress));
Copy link
Member

Choose a reason for hiding this comment

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

Here and the next line too. I'll stop. You get the idea. :-D

@Trott
Copy link
Member

Trott commented Dec 31, 2016

Nit: commit message should start with an imperative verb. Although I guess arguably the s/.../ is substitute and that's an imperative verb, so...
¯\(ツ)/¯

@cjihrig
Copy link
Contributor Author

cjihrig commented Jan 2, 2017

FWIW, I went with s/.../ to work within the 50 character limit.

@cjihrig
Copy link
Contributor Author

cjihrig commented Jan 6, 2017

New CI with @Trott's suggestions: https://ci.nodejs.org/job/node-test-pull-request/5729/

@cjihrig
test: s/assert.notEqual()/assert.notStrictEqual()/
e5499b3 
PR-URL: nodejs#10541
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Italo A. Casas <me@italoacasas.com>
@cjihrig cjihrig merged commit e5499b3 into nodejs:master Jan 6, 2017
@cjihrig cjihrig deleted the not-equal branch January 6, 2017 19:11
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 18, 2017
@cjihrig @italoacasas
test: s/assert.notEqual()/assert.notStrictEqual()/
5d0aa40 
PR-URL: nodejs#10541
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Italo A. Casas <me@italoacasas.com>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 19, 2017
@cjihrig @italoacasas
test: s/assert.notEqual()/assert.notStrictEqual()/
9e4d439 
PR-URL: nodejs#10541
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Italo A. Casas <me@italoacasas.com>
@italoacasas italoacasas mentioned this pull request Jan 25, 2017
Closed
targos pushed a commit that referenced this pull request Jan 28, 2017
@cjihrig @targos
test: s/assert.notEqual()/assert.notStrictEqual()/
bb71ef7 
PR-URL: #10541
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Italo A. Casas <me@italoacasas.com>
@italoacasas italoacasas mentioned this pull request Jan 29, 2017
Merged
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 30, 2017
@cjihrig @italoacasas
test: s/assert.notEqual()/assert.notStrictEqual()/
5013022 
PR-URL: nodejs#10541
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Italo A. Casas <me@italoacasas.com>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 30, 2017
@cjihrig @italoacasas
test: s/assert.notEqual()/assert.notStrictEqual()/
37d1f56 
PR-URL: nodejs#10541
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Italo A. Casas <me@italoacasas.com>
@jasnell
Copy link
Member

jasnell commented Mar 8, 2017

This will need backport PRs to land on v4 and v6

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gibfahn gibfahn gibfahn approved these changes

@italoacasas italoacasas italoacasas approved these changes

@Trott Trott Trott approved these changes

@jasnell jasnell jasnell approved these changes

@sam-github sam-github sam-github approved these changes

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

Successfully merging this pull request may close these issues.
8 participants
@cjihrig @Trott @jasnell @sam-github @italoacasas @gibfahn @mscdex @nodejs-github-bot