Node: Remove `indexOf` usage from tests in favor of `includes`

[refack]

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

testaddons\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)__

[lpinca]

I have no strong opinions, +0.

[vsemozhetbyt]

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

[vsemozhetbyt]

cc @nodejs/lts @nodejs/testing

[refack]

(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]

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

[refack]

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

[gibfahn]

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

[refack]

@gibfahn see new desc

[mscdex]

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]

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

[mscdex]

@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]

+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]

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 https://github.com/nodejs/node/pull/12499#issuecomment-295839361, 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]

@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]

@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/LTS#203

[gibfahn]

@mscdex theoretically test improvements wouldn't be backported to maintenance branches:

Once a release moves into Maintenance mode, only critical bugs, critical security fixes, and documentation updates will be permitted.

Obviously in realistic terms some test changes may be backported, and how much that'll happen is actively being discussed in the LTS WG. However I don't think this change will be particularly disruptive as:

• a backport that includes includes() will fail every time, so won't get into a v4.x release unnoticed
• the change is atomic and only affects a single line, so shouldn't cause too many conflicts
• resolving any conflicts should be very simple (swap .indexOf() for the includes())
• we're unlikely to backport test improvements unless it's to fix a buggy test, so it is unlikely to come up very often

---

Also String .includes() already works in Node 4, it's just Array .includes() that doesn't, so instances of the former can definitely be replaced.

 ❯ node
> process.version
'v4.8.2'
> 'hi'.includes('h')
true
> ['h', 'i'].includes('h')
TypeError: ["h","i"].includes is not a function

[AbrahamFergie]

If there are no objections could i do the swaps for all tests where "indexof" is being applied to strings?

[refack]

@AbrahamFergie there already is #12604, but I'm not sure it's complete, or String exclusive.
If you want to help you could fetch the PR locally and see if it needs more work

[gibfahn]

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.

@mscdex at the last LTS meeting it was agreed that those changes will continue to not be backported, so that shouldn't be an issue. (cc/ @nodejs/lts)

[refack]

IMHO a good replacement for

assert.strictEqual(x.indexof(y), -1)

is

assert.strictEqual(x.includes(y), false)

It's more literal and I found it more readable.

[benjamingr]

Going to use to onboard people today - please update your PRs with the checklists.

[refack]

Going to use to onboard people today - please update your PRs with the checklists.

I think we've converged on assert.strictEqual(foo.includes(bar), true);, Updating OP

[TimothyGu]

Why assert.strictEqual instead of just assert?

[refack]

Why assert.strictEqual instead of just assert?

Mostly for explicitness sake, but also:

1. minimize change, since most assertions now are assert.strictNotEqual(foo.indexOf(bar), -1)
2. assert (and assert.ok) are not strict
3. the excludes case reads better assert.strictEqual(foo.includes(bar), false)

The fail message will be useless (only hint is the stacktrace) anyway 🤷‍♂️ at least until we add assert.includes

[benjamingr]

Sounds good to me

[nataly87s]

I'm working on this

[Bnaya]

Ill take the files under test\pummel

[maasencioh]

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

• [x] test/async-hooks/init-hooks - https://github.com/nodejs/node/pull/14630
• [x] test/parallel/test-repl-tab-complete - https://github.com/nodejs/node/pull/14630
• [ ] test/parallel/test-buffer-indexof

[maasencioh]

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]

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

[BridgeAR]

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.