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

Remove indexOf usage from tests in favor of includes #12586

Closed
29 tasks
refack opened this issue Apr 22, 2017 · 31 comments
Closed
29 tasks

Remove indexOf usage from tests in favor of includes #12586

refack opened this issue Apr 22, 2017 · 31 comments
Labels
discuss Issues opened for discussions and feedbacks. lts Issues and PRs related to Long Term Support releases. test Issues and PRs related to the tests.

Comments

@refack
Copy link
Contributor

refack commented Apr 22, 2017

  • Version: > v4
  • Platform: all
  • Subsystem: test

Since v4 entered maintenance, do you think we can eliminate most of the 433 instances of indexOf from /test/*.js?
Note: The prefered alternative is assert.strictEqual(foo.includes(bar), true)

Each hit should be evaluated whether it's an includes surrogate, or a real indexOf use:

test\addons\repl-domain-abort

  • test.js (1 usage found)

test\async-hooks

  • init-hooks.js (2 usages found)

test\common

  • index.js (1 usage found)

test\disabled

  • test-sendfd.js (1 usage found)

test\doctool

  • test-doctool-html.js (3 usages found)

test\internet

  • test-dns-any.js (1 usage found)
  • test-dns.js (1 usage found)

test\parallel

  • test-buffer-fakes.js (1 usage found)
  • test-buffer-write.js (1 usage found)
  • test-child-process-exec-cwd.js (1 usage found)
  • test-console.js (1 usage found)
  • test-dgram-error-message-address.js (1 usage found)
  • test-domain-top-level-error-handler-throw.js (2 usages found)
  • test-domain-uncaught-exception.js (2 usages found)
  • test-domain.js (1 usage found)
  • test-http-keepalive-maxsockets.js (1 usage found)
  • test-http-methods.js (3 usages found)
  • test-http-outgoing-first-chunk-singlebyte-encoding.js (1 usage found)
  • test-http-parser.js (13 usages found)
  • test-http-write-head.js (1 usage found)
  • test-net-server-connections.js (1 usage found)
  • test-path-parse-format.js (1 usage found)
  • test-process-getgroups.js (1 usage found)
  • test-repl-tab-complete.js (11 usages found)
  • test-tls-interleave.js (1 usage found)

test\pummel

  • test-dtrace-jsstack.js (3 usages found)
  • test-regress-G H-814.js (1 usage found)
  • test-regress-GH-814_2.js (1 usage found)

and a special treat

  • test\parallel\test-buffer-indexof.js (292 usages found)
@refack refack added discuss Issues opened for discussions and feedbacks. test Issues and PRs related to the tests. labels Apr 22, 2017
@lpinca
Copy link
Member

lpinca commented Apr 22, 2017

I have no strong opinions, +0.

@vsemozhetbyt
Copy link
Contributor

vsemozhetbyt commented Apr 22, 2017

(BTW, how about new ES6+ label for such issues and PRs?)

@vsemozhetbyt
Copy link
Contributor

cc @nodejs/lts @nodejs/testing

@refack
Copy link
Contributor Author

refack commented Apr 22, 2017

(BTW, how about new ES6+ label for such issues and PRs?)

Since it's not a problem with ES6, I think more like move-to-ES6+?

@jseijas
Copy link

jseijas commented Apr 22, 2017

IMHO that should be done. Reasons: readability and [NaN].

@refack
Copy link
Contributor Author

refack commented Apr 22, 2017

Maybe a "good first contribution" or "code-and-lean" like #12376

@refack refack added the good first issue Issues that are suitable for first-time contributors. label Apr 22, 2017
@gibfahn
Copy link
Member

gibfahn commented Apr 22, 2017

Obviously doesn't make sense for the cases where you actually use the index, but otherwise SGTM.

@refack
Copy link
Contributor Author

refack commented Apr 22, 2017

@gibfahn see new desc

@refack refack added discuss Issues opened for discussions and feedbacks. and removed discuss Issues opened for discussions and feedbacks. labels Apr 22, 2017
@mscdex
Copy link
Contributor

mscdex commented Apr 22, 2017

I'm not sure we should make these changes until v4 is officially no longer supported at all, just in case we make some improvements to tests and want to backport them.

@vsemozhetbyt
Copy link
Contributor

vsemozhetbyt commented Apr 22, 2017

BTW, v4 supports ''.includes() and ''.startsWith(), so maybe string cases can be addressed, if any.

@mscdex
Copy link
Contributor

mscdex commented Apr 22, 2017

@vsemozhetbyt I dunno, personally I'd rather just avoid it altogether (string or array) to avoid any confusion. startsWith() is fine though, since that's only available for strings and exists in v4.x.

@Fishrock123
Copy link
Contributor

+1 to what @mscdex said. Also I think .includes() is pretty fast but if we wait a bit longer it'l probably be on bar with .indexOf() with Ignition and TurboFan.

@refack
Copy link
Contributor Author

refack commented Apr 22, 2017

I'm not sure we should make these changes until v4 is officially no longer supported at all, just in case we make some improvements to tests and want to backport them.

From what I understand from what @MylesBorins wrote in #12499 (comment), there will be no backporting of any improvements, just bug fixes. Those will probably come with new test-cases, that will need to be re-written to v4 anyway.

@mscdex
Copy link
Contributor

mscdex commented Apr 22, 2017

@refack I said improvements to tests, not feature additions to core that are backported. Sometimes tests themselves are refactored (e.g. what code and learn is doing now with adding common.mustCall() usage, replacing generic error checking, etc.) and those changes are backported.

@refack
Copy link
Contributor Author

refack commented Apr 22, 2017

@refack I said improvements to tests, not feature additions to core that are backported. Sometimes tests are refactored (e.g. what code and learn is doing now with adding common.mustCall() usage, replacing generic error checking, etc.) and those changes are backported.

Since this is the first time a big version enters maintenance, I think you are raising an interesting issue that should be discussed in a wider context then this thread. spinning-off nodejs/Release#203

@refack refack added lts Issues and PRs related to Long Term Support releases. lts-agenda labels Apr 22, 2017
gwer added a commit to gwer/node that referenced this issue Apr 23, 2017
nataly87s pushed a commit to nataly87s/node that referenced this issue Jul 2, 2017
tniessen pushed a commit that referenced this issue Jul 2, 2017
PR-URL: #13852
Refs: #12586
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
addaleax pushed a commit to addaleax/node that referenced this issue Jul 3, 2017
PR-URL: nodejs#13852
Refs: nodejs#12586
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
MylesBorins pushed a commit that referenced this issue 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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
addaleax pushed a commit that referenced this issue Jul 11, 2017
PR-URL: #13852
Refs: #12586
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
addaleax pushed a commit that referenced this issue Jul 18, 2017
PR-URL: #13852
Refs: #12586
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
Fishrock123 pushed a commit that referenced this issue Jul 19, 2017
PR-URL: #13852
Refs: #12586
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
maasencioh added a commit to maasencioh/node that referenced this issue Aug 4, 2017
@maasencioh
Copy link
Contributor

maasencioh commented Aug 4, 2017

Hello @refack , could you please update a little bit the list? because I think that the current state is the following, but I would like your oppinion:

indexOf that shouldn't be changed

  • test/common/index
  • test/pummel/test-dtrace-jsstack

pending

jasnell pushed a commit that referenced this issue Aug 8, 2017
Refs: #12586
PR-URL: #14630
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Alexey Orlenko <[email protected]>
addaleax pushed a commit that referenced this issue Aug 10, 2017
Refs: #12586
PR-URL: #14630
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Alexey Orlenko <[email protected]>
MylesBorins pushed a commit that referenced this issue Aug 14, 2017
PR-URL: #13215
Refs: #12586
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
MylesBorins pushed a commit that referenced this issue Aug 15, 2017
PR-URL: #13852
Refs: #12586
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
MylesBorins pushed a commit that referenced this issue Aug 15, 2017
PR-URL: #13852
Refs: #12586
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
MylesBorins pushed a commit that referenced this issue Aug 16, 2017
PR-URL: #13215
Refs: #12586
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
MylesBorins pushed a commit that referenced this issue Aug 16, 2017
PR-URL: #13852
Refs: #12586
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
@maasencioh
Copy link
Contributor

actually for the case in test/parallel/test-buffer-indexof I don't find anything to be replaced, should this issue be closed then?

cc @refack

@gibfahn
Copy link
Member

gibfahn commented Sep 16, 2017

This has already been backported to LTS, removing the label.

@BridgeAR
Copy link
Member

This is pretty outdated. We had all errors migrated and now there are a couple new files that contain indexOf that might be changed but it is likely that new ones will pop in if we do not have a eslint rule to prevent that.

I am closing this as resolved for now. If we get lots of those in, we can reopen the issue / create a new one.

gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this issue 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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
MylesBorins pushed a commit that referenced this issue 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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discuss Issues opened for discussions and feedbacks. lts Issues and PRs related to Long Term Support releases. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging a pull request may close this issue.