Repository navigation
Remove indexOf usage from tests in favor of includes #12586
Description
- 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)
Activity
lpinca commentedon Apr 22, 2017
I have no strong opinions, +0.
vsemozhetbyt commentedon Apr 22, 2017
(BTW, how about new
ES6+label for such issues and PRs?)vsemozhetbyt commentedon Apr 22, 2017
cc @nodejs/lts @nodejs/testing
refack commentedon Apr 22, 2017
Since it's not a problem with ES6, I think more like
move-to-ES6+?jseijas commentedon Apr 22, 2017
IMHO that should be done. Reasons: readability and [NaN].
refack commentedon Apr 22, 2017
Maybe a "good first contribution" or "code-and-lean" like #12376
gibfahn commentedon Apr 22, 2017
Obviously doesn't make sense for the cases where you actually use the index, but otherwise SGTM.
refack commentedon Apr 22, 2017
@gibfahn see new desc
mscdex commentedon 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.
52 remaining items
test: replace indexOf with includes
test: replace `indexOf` with `includes`
test: replace indexOf with includes and startsWith
test: replace indexOf with includes and startsWith
test: replace `indexOf` with `includes`
test: replace indexOf with includes and startsWith
maasencioh commentedon Aug 24, 2017
actually for the case in
test/parallel/test-buffer-indexofI don't find anything to be replaced, should this issue be closed then?cc @refack
gibfahn commentedon Sep 16, 2017
This has already been backported to LTS, removing the label.
BridgeAR commentedon Dec 16, 2017
This is pretty outdated. We had all errors migrated and now there are a couple new files that contain
indexOfthat 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.
test: replace indexOf with includes
test: replace indexOf with includes