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: replace indexOf with includes #12604

Closed
wants to merge 1 commit into from
Closed

Conversation

gwer
Copy link
Contributor

@gwer gwer commented Apr 23, 2017

Partly fixes #12586

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

@nodejs-github-bot nodejs-github-bot added addons Issues and PRs related to native addons. dont-land-on-v4.x inspector Issues and PRs related to the V8 inspector protocol node-api Issues and PRs related to the Node-API. test Issues and PRs related to the tests. labels Apr 23, 2017
@refack
Copy link
Contributor

refack commented Apr 23, 2017

@gwer Looks good. But we're still discussing the implication of #12586, so it might take some time to land (or might even be rejected). But thank you very much for the effort!

@refack refack removed addons Issues and PRs related to native addons. inspector Issues and PRs related to the V8 inspector protocol node-api Issues and PRs related to the Node-API. labels Apr 23, 2017
assert.ok(propertyNames.indexOf('readonlyValue') >= 0);
assert.ok(propertyNames.includes('echo'));
assert.ok(propertyNames.includes('readwriteValue'));
assert.ok(propertyNames.includes('readonlyValue'));
assert.ok(propertyNames.indexOf('hiddenValue') < 0);
assert.ok(propertyNames.indexOf('readwriteAccessor1') < 0);
assert.ok(propertyNames.indexOf('readwriteAccessor2') < 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

These should be updated to !propertyNames.includes() too so that the code is consistent.

assert.ok(propertyNames.indexOf('readonlyValue') >= 0);
assert.ok(propertyNames.includes('echo'));
assert.ok(propertyNames.includes('readwriteValue'));
assert.ok(propertyNames.includes('readonlyValue'));
assert.ok(propertyNames.indexOf('hiddenValue') < 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto.

@@ -54,7 +54,7 @@ dInput._read = function _read(size) {
};

dOutput._write = function _write(chunk, encoding, cb) {
if (chunk.toString().indexOf('cb_ran') === 0)
if (chunk.toString().startsWith('cb_ran'))
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it makes sense to decouple changes to startsWith to a different PR. As far as I can see from the discussion in #12586, that PR won't be controversial like this one may potentially be :)

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree, all === 0 could be in a separate PR, and land easily.

assert.ok(propertyNames.includes('readwriteValue'));
assert.ok(propertyNames.includes('readonlyValue'));
assert.ok(!propertyNames.includes('hiddenValue'));
assert.ok(!propertyNames.includes('readwriteAccessor1'));
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@aqrln
Copy link
Contributor

aqrln commented Apr 23, 2017

@mscdex
Copy link
Contributor

mscdex commented Apr 23, 2017

I'm -1 on this for the reasons I described in #12586.

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 if CI is green

@gwer
Copy link
Contributor Author

gwer commented Apr 24, 2017

CI is green now.

@jasnell
Copy link
Member

jasnell commented Apr 24, 2017

@mscdex ... can you use the "request changes" thing on here to put the big red X so folks don't miss your objection.

mscdex
mscdex previously requested changes Apr 24, 2017
Copy link
Contributor

@mscdex mscdex left a comment

Choose a reason for hiding this comment

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

I'm -1 on this for the reasons I described in #12586.

@refack
Copy link
Contributor

refack commented Apr 27, 2017

@mscdex the @nodejs/lts have deceived that "theoretically test improvements wouldn't be backported to maintenance branches" will you be willing to lift your objection?

@gibfahn
Copy link
Member

gibfahn commented Apr 27, 2017

@mscdex at the last LTS meeting we agreed that test changes will not be backported (except in some once-in-a-blue-moon occasion where it's really crucial). Given that, do you still object?

cc/ @nodejs/lts , does anyone have an issue with these changes landing now?

@mscdex
Copy link
Contributor

mscdex commented Apr 27, 2017

@gibfahn It's not just general test changes (e.g. adding common.mustCall() in places) but even things like non-semver-major changes that get backported that include tests (that may use .includes()). I guess as long as whoever is backporting doesn't mind having to change commit(s) for v4.x, then it's fine. Anyway, it seems I'm in the minority here, so I say go ahead.

@mscdex mscdex dismissed their stale review April 27, 2017 17:37

no longer disapprove

@MylesBorins
Copy link
Contributor

MylesBorins commented Apr 27, 2017 via email

@refack
Copy link
Contributor

refack commented Apr 27, 2017

@MylesBorins are String.prototype.includes and Array.prototype.includes compatible with v6.x?

@gibfahn
Copy link
Member

gibfahn commented Apr 27, 2017

Seems to work for me:

> process.version
'v6.10.1'
> [1,2,3].includes(2)
true
> '123'.includes(2)
true

@aqrln
Copy link
Contributor

aqrln commented Apr 29, 2017

Given that there are no more objections, I'm going to go ahead and land this.
Fresh CI: https://ci.nodejs.org/job/node-test-pull-request/7749/

@aqrln
Copy link
Contributor

aqrln commented Apr 30, 2017

Landed in 0142276.

@aqrln aqrln closed this Apr 30, 2017
aqrln pushed a commit that referenced this pull request Apr 30, 2017
Start the transition to Array.prototype.includes() and
String.prototype.includes().  This commit refactors most of the
comparisons of Array.prototype.indexOf() and String.prototype.indexOf()
return values with -1 to the former methods in tests.

PR-URL: #12604
Refs: #12586
Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
@jasnell jasnell mentioned this pull request May 11, 2017
@gibfahn gibfahn mentioned this pull request Jun 15, 2017
3 tasks
gibfahn pushed a commit that referenced this pull request Jun 18, 2017
Start the transition to Array.prototype.includes() and
String.prototype.includes().  This commit refactors most of the
comparisons of Array.prototype.indexOf() and String.prototype.indexOf()
return values with -1 to the former methods in tests.

PR-URL: #12604
Refs: #12586
Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
gibfahn pushed a commit that referenced this pull request Jun 20, 2017
Start the transition to Array.prototype.includes() and
String.prototype.includes().  This commit refactors most of the
comparisons of Array.prototype.indexOf() and String.prototype.indexOf()
return values with -1 to the former methods in tests.

PR-URL: #12604
Refs: #12586
Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jul 11, 2017
Start the transition to Array.prototype.includes() and
String.prototype.includes().  This commit refactors most of the
comparisons of Array.prototype.indexOf() and String.prototype.indexOf()
return values with -1 to the former methods in tests.

PR-URL: #12604
Refs: #12586
Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Jul 18, 2017
gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this pull request Apr 10, 2018
Start the transition to Array.prototype.includes() and
String.prototype.includes().  This commit refactors most of the
comparisons of Array.prototype.indexOf() and String.prototype.indexOf()
return values with -1 to the former methods in tests.

PR-URL: nodejs#12604
Refs: nodejs#12586
Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
MylesBorins pushed a commit that referenced this pull request Apr 16, 2018
Start the transition to Array.prototype.includes() and
String.prototype.includes().  This commit refactors most of the
comparisons of Array.prototype.indexOf() and String.prototype.indexOf()
return values with -1 to the former methods in tests.

Backport-PR-URL: #19447
PR-URL: #12604
Refs: #12586
Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Apr 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove indexOf usage from tests in favor of includes
8 participants