GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — test: replace indexOf with includes by gwer · Pull Request #12604 · nodejs/node

Conversation

@gwer

@gwer gwer commented Apr 23, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

Partly fixes #12586

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
refack reacted with heart emoji
All reactions
  • ❤️ 1 reaction
@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 Node-API. test Issues and PRs related to Node.js core tests and test infrastructure. labels Apr 23, 2017
@refack

refack commented Apr 23, 2017

Copy link
Copy Markdown
Contributor

@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!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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 Node-API. labels Apr 23, 2017
aqrln
aqrln reviewed Apr 23, 2017
View reviewed changes
Comment thread test/addons-napi/test_constructor/test.js Outdated

@aqrln aqrln Apr 23, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
refack reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
aqrln
aqrln reviewed Apr 23, 2017
View reviewed changes
Comment thread test/addons-napi/test_properties/test.js Outdated

@aqrln aqrln Apr 23, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Ditto.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
refack reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
aqrln
aqrln reviewed Apr 23, 2017
View reviewed changes
Comment thread test/addons/repl-domain-abort/test.js Outdated

@aqrln aqrln Apr 23, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

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 :)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@refack refack Apr 23, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@gwer
gwer force-pushed the indexOf-replace branch from 47c2af9 to 92fe4d9 Compare April 23, 2017 00:25
refack
refack reviewed Apr 23, 2017
View reviewed changes
Comment thread test/addons-napi/test_constructor/test.js Outdated

@refack refack Apr 23, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

👍

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@gwer
test: replace indexOf with includes
d918367 
Partly fixes nodejs#12586
@gwer
gwer force-pushed the indexOf-replace branch from 92fe4d9 to d918367 Compare April 23, 2017 00:27
@aqrln

aqrln commented Apr 23, 2017

Copy link
Copy Markdown
Contributor

CI: https://ci.nodejs.org/job/node-test-pull-request/7605/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
aqrln
aqrln approved these changes Apr 23, 2017
View reviewed changes
@mscdex

mscdex commented Apr 23, 2017

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
jasnell
jasnell approved these changes Apr 24, 2017
View reviewed changes

@jasnell jasnell left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this 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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
gibfahn
gibfahn approved these changes Apr 24, 2017
View reviewed changes
@gwer

gwer commented Apr 24, 2017

Copy link
Copy Markdown
Contributor Author

CI is green now.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jasnell

jasnell commented Apr 24, 2017

Copy link
Copy Markdown
Member

@mscdex ... can you use the "request changes" thing on here to put the big red X so folks don't miss your objection.
refack and gibfahn reacted with thumbs up emoji
All reactions
  • 👍 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
mscdex
mscdex previously requested changes Apr 24, 2017
View reviewed changes

@mscdex mscdex left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this 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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@refack refack mentioned this pull request Apr 27, 2017
Remove indexOf usage from tests in favor of includes #12586
Closed
29 tasks
@refack

refack commented Apr 27, 2017

Copy link
Copy Markdown
Contributor

@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?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@gibfahn

gibfahn commented Apr 27, 2017

Copy link
Copy Markdown
Member

@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?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@mscdex

mscdex commented Apr 27, 2017

Copy link
Copy Markdown
Contributor

@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.
refack reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@mscdex
mscdex dismissed their stale review April 27, 2017 17:37

no longer disapprove

@MylesBorins

MylesBorins commented Apr 27, 2017 via email

Copy link
Copy Markdown
Contributor
Test changes still need to be backport to v6.x
…
On Apr 27, 2017 1:36 PM, "Brian White" ***@***.***> wrote: @gibfahn <https://github.com/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. — You are receiving this because you are on a team that was mentioned. Reply to this email directly, view it on GitHub <#12604 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAecV1aOe51sQVfSSHJ6mImzE3pcvlSTks5r0NIxgaJpZM4NFQYt> .
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@refack

refack commented Apr 27, 2017

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@gibfahn

gibfahn commented Apr 27, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

Seems to work for me:

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@aqrln aqrln added refactor to ES6+ labels Apr 29, 2017
@aqrln

aqrln commented Apr 29, 2017

Copy link
Copy Markdown
Contributor

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/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@aqrln

aqrln commented Apr 30, 2017

Copy link
Copy Markdown
Contributor

Landed in 0142276.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@aqrln aqrln closed this Apr 30, 2017
aqrln pushed a commit that referenced this pull request Apr 30, 2017
@gwer @aqrln
test: replace indexOf with includes
0142276 
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
@jasnell jasnell mentioned this pull request May 11, 2017
8.0.0 Release Proposal #12220
Closed
@gibfahn gibfahn mentioned this pull request Jun 15, 2017
Auditing for 6.11.1 nodejs/Release#230
Closed
2 of 3 tasks
gibfahn pushed a commit that referenced this pull request Jun 18, 2017
@gwer @gibfahn
test: replace indexOf with includes
17cb9ca 
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
@gibfahn gibfahn added land-on-v6.x and removed lts-watch-v6.x labels Jun 18, 2017
gibfahn pushed a commit that referenced this pull request Jun 20, 2017
@gwer @gibfahn
test: replace indexOf with includes
1d8cb44 
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jul 11, 2017
@gwer @MylesBorins
test: replace indexOf with includes
54decfa 
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Jul 18, 2017
v6.11.2 proposal #14356
Merged
gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this pull request Apr 10, 2018
@gwer Gabriel Schulhof
test: replace indexOf with includes
3461bc1 
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
MylesBorins pushed a commit that referenced this pull request Apr 16, 2018
@gwer @MylesBorins
test: replace indexOf with includes
5d2afb2 
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Apr 16, 2018
v6.14.2 proposal #19996
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnell jasnell jasnell approved these changes
+4 more reviewers
@refack refack refack left review comments
@aqrln aqrln aqrln approved these changes
@gibfahn gibfahn gibfahn approved these changes
@mscdex mscdex mscdex left review comments
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

test Issues and PRs related to Node.js core tests and test infrastructure.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Remove indexOf usage from tests in favor of includes

Uh oh!

There was an error while loading. Please reload this page.

8 participants

@gwer @refack @aqrln @mscdex @jasnell @gibfahn @MylesBorins @nodejs-github-bot
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.