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` #13215

Closed
wants to merge 1 commit into from

Conversation

@thelostone-mc
Copy link
Contributor

commented May 25, 2017

Refs: #12586

Tests Updated:

  • test/parallel/test-http-client-agent.js
  • test/parallel/test-http-client-default-headers-exist.js
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

test

@thelostone-mc thelostone-mc force-pushed the thelostone-mc:master branch May 25, 2017

@lpinca
lpinca approved these changes May 25, 2017

@mscdex mscdex added the http label May 25, 2017

@refack
refack approved these changes May 25, 2017
@@ -55,7 +55,8 @@ function request(i) {
socket.on('close', function() {
++count;
if (count < max) {
assert.strictEqual(http.globalAgent.sockets[name].indexOf(socket), -1);
assert.strictEqual(http.globalAgent.sockets[name].includes(socket),

This comment has been minimized.

Copy link
@refack

refack May 25, 2017

Member

I like this approach 👍
More literal then assert.ok(!...

@refack refack self-assigned this May 25, 2017

@refack refack referenced this pull request May 25, 2017
0 of 29 tasks complete
test/parallel/test-http-client-default-headers-exist.js Outdated
expectedHeaders[req.method].indexOf(header.toLowerCase()),
-1,
expectedHeaders[req.method].includes(header.toLowerCase()),
false,

This comment has been minimized.

Copy link
@gibfahn

gibfahn May 25, 2017

Member

Could we change assert.notStrictEqual(..., false, ...) -> assert.strictEqual(..., true, ...)? The double negative seems unnecessary now.

This comment has been minimized.

Copy link
@thelostone-mc

thelostone-mc May 26, 2017

Author Contributor

Ah missed that! Will fix it!
Also @gibfahn , why doesn't assert provide a functions like assertFalse ?
(Similar to assertok )

This comment has been minimized.

Copy link
@gibfahn

gibfahn May 26, 2017

Member

We have assert.fail() to test if something is falsy, but it's not a strict comparison (it's == not ===), and I find the assertStrictEqual(..., false) more clear (not sure why we have assert.fail not assert.false).

test: replace `indexOf` with `includes`
Replaced `indexOf` with `includes` in test-http-client-agent,
test-http-client-default-headers-exist

Refs: #12586

@thelostone-mc thelostone-mc force-pushed the thelostone-mc:master branch to 2ce4ac1 May 26, 2017

@gibfahn
Copy link
Member

left a comment

LGTM, thanks!

refack added a commit to refack/node that referenced this pull request May 27, 2017
test: replace `indexOf` with `includes`
PR-URL: nodejs#13215
Refs: nodejs#12586
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
@refack

This comment has been minimized.

Copy link
Member

commented May 27, 2017

Landed in 9c64278

@refack refack closed this May 27, 2017

@refack

This comment has been minimized.

Copy link
Member

commented May 27, 2017

jasnell added a commit that referenced this pull request May 28, 2017
test: replace `indexOf` with `includes`
PR-URL: #13215
Refs: #12586
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
@jasnell jasnell referenced this pull request May 28, 2017
@gibfahn gibfahn referenced this pull request Jun 15, 2017
2 of 3 tasks complete
@MylesBorins

This comment has been minimized.

Copy link
Member

commented Jul 17, 2017

I have mixed feelings in backporting this to v6.x. @nodejs/v8 is this something we need to be concerned about from a performance standpoint?

If the change is fairly moot in 8.x, but causes problems on 6.x, I'm not convinced the churn is worth it (as it will make backporting harder). I would appreciate if this type of research could be done before landing these changes wholesale.

If this has already been researched and it is a non issue please accept my humble apologies for crankiness in the above paragraph 😄

@refack

This comment has been minimized.

Copy link
Member

commented Jul 17, 2017

I have mixed feelings in backporting this to v6.x. @nodejs/v8 is this something we need to be concerned about from a performance standpoint?

If the change is fairly moot in 8.x, but causes problems on 6.x, I'm not convinced the churn is worth it (as it will make backporting harder). I would appreciate if this type of research could be done before landing these changes wholesale.

If this has already been researched and it is a non issue please accept my humble apologies for crankiness in the above paragraph 😄

This was discussed re backporting in #12586 (comment) and was "empirically" tested for compatibility.
Re performance, such changes were limited to /test/ and /tools/. /lib/ was excluded to preclude actual performance degradation, and /benchmark/ was excluded to preclude any perceived perturbations.

Hope this info helps.

@gibfahn

This comment has been minimized.

Copy link
Member

commented Jul 18, 2017

is this something we need to be concerned about from a performance standpoint?

As @refack says, only test/ has been changed for exactly that reason.

I assume we'd plan to wait till v6.x goes into maintenance before considering changes to lib/, and they would require proper performance testing in any case.

I would appreciate if this type of research could be done before landing these changes wholesale.

We did discuss this in an LTS meeting: #12586 (comment) and https://github.com/nodejs/LTS/blob/master/doc/meetings/2017-04-24.md#backporting-test-fixes-to-maintenance-branches-205

If this has already been researched and it is a non issue please accept my humble apologies for crankiness in the above paragraph

Sounds like backporting induced PTSD to me 😁

@MylesBorins

This comment has been minimized.

Copy link
Member

commented Jul 18, 2017

my humble apologies for crankiness in the above paragraph 😄

MylesBorins added a commit that referenced this pull request Aug 14, 2017
test: replace `indexOf` with `includes`
PR-URL: #13215
Refs: #12586
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
MylesBorins added a commit that referenced this pull request Aug 16, 2017
test: replace `indexOf` with `includes`
PR-URL: #13215
Refs: #12586
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
9 participants
You can’t perform that action at this time.