test: cleanup/update test-os.js #8761
Conversation
| @@ -157,13 +157,13 @@ if (common.isWindows) { | |||
| } else { | |||
| is.number(pwd.uid); | |||
| is.number(pwd.gid); | |||
| assert.notStrictEqual(pwd.shell.indexOf(path.sep), -1); | |||
| assert.notStrictEqual(pwd.shell.includes(path.sep), false); | |||
There was a problem hiding this comment.
This is fine the way it is, but it could probably just be something like assert.ok(!pwdshell.includes(path.sep)); if you want.
There was a problem hiding this comment.
Good call on the use of assert.ok. It's much clearer. I think ! should be omitted since we're testing the condition <cond> !== false
| assert.strictEqual(pwd.uid, pwdBuf.uid); | ||
| assert.strictEqual(pwd.gid, pwdBuf.gid); | ||
| assert.strictEqual(pwd.shell, pwdBuf.shell.toString('utf8')); | ||
| } | ||
|
|
||
| is.string(pwd.username); | ||
| assert.notStrictEqual(pwd.homedir.indexOf(path.sep), -1); | ||
| assert.notStrictEqual(pwd.homedir.includes(path.sep), false); |
There was a problem hiding this comment.
As above: fine the way it is, but could also be that other format.
Replaced `==` with `=== Replaced `indexOf(...) !== -1` with `includes()`
| @@ -101,7 +101,7 @@ console.error(interfaces); | |||
| switch (platform) { | |||
| case 'linux': | |||
| { | |||
| const filter = function(e) { return e.address == '127.0.0.1'; }; | |||
| const filter = function(e) { return e.address === '127.0.0.1'; }; | |||
| const actual = interfaces.lo.filter(filter); | |||
There was a problem hiding this comment.
Nit: Can the filter be replaced with an arrow function, still within 80 chars limits?
There was a problem hiding this comment.
The arrow function will work in the linux part, but I don't believe it can (at least not in any obvious way) in the second instance (in the win32 portion) where the line it needs to be put into is longer. I'm reluctant to make them different from each other. Seems clearer to me to have them both be the same, even if it means an extra line in one case. Since the comment is labeled as a nit and ends with a question mark, I'm going to take that to mean that it's OK for this to land without that suggestion.
Replaced `==` with `=== Replaced `indexOf(...) !== -1` with `includes()` PR-URL: nodejs#8761 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com>
|
Landed in f5d997c |
Replaced `==` with `=== Replaced `indexOf(...) !== -1` with `includes()` PR-URL: #8761 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com>
Replaced `==` with `=== Replaced `indexOf(...) !== -1` with `includes()` PR-URL: #8761 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com>
Checklist
make -j8 test(UNIX), orvcbuild test nosign(Windows) passesAffected core subsystem(s)
test
Description of change
Replaced
==with=== ReplacedindexOf(...) !== -1withincludes()`