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: improve check in test-os #14655

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
8 participants
@Trott
Member

Trott commented Aug 7, 2017

The check for os.networkInterfaces() in test-os.js may be too
strict. It's apparently possible for a machine to be configured with
multiple IPv4 loopback interfaces. Increase specificity of filter to
check on only the object we expect.

Fixes: #14654

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

test os

@Trott Trott added os test labels Aug 7, 2017

@Trott Trott changed the title from test: relax check in test-os to test: improve check in test-os Aug 7, 2017

@lpinca

lpinca approved these changes Aug 7, 2017

@Trott

This comment has been minimized.

Show comment
Hide comment
@cjihrig

cjihrig approved these changes Aug 7, 2017

Show outdated Hide outdated test/parallel/test-os.js
test: improve check in test-os
The check for `os.networkInterfaces()` in `test-os.js` may be too
strict. It's apparently possible for a machine to be configured with
multiple IPv4 loopback interfaces. Increase specificity of filter to
check on only the object we expect.

Fixes: #14654
@refack

refack approved these changes Aug 7, 2017

@@ -116,7 +116,8 @@ const interfaces = os.networkInterfaces();
switch (platform) {
case 'linux':
{
const filter = (e) => e.address === '127.0.0.1';
const filter =
(e) => e.address === '127.0.0.1' && e.netmask === '255.0.0.0';
const actual = interfaces.lo.filter(filter);
const expected = [{ address: '127.0.0.1', netmask: '255.0.0.0',

This comment has been minimized.

@refack

refack Aug 7, 2017

Member

[warning 🚨 scope-creep]
AFAICT expected and the asserting could be hoisted, and switch replaced with if or trinary for selecting interfaces.lo / interfaces['Loopback Pseudo-Interface 1']

@refack

refack Aug 7, 2017

Member

[warning 🚨 scope-creep]
AFAICT expected and the asserting could be hoisted, and switch replaced with if or trinary for selecting interfaces.lo / interfaces['Loopback Pseudo-Interface 1']

This comment has been minimized.

@Trott

Trott Aug 7, 2017

Member

Some reasons to maybe not do that:

  • If we want to add macOS to the switch statement (in another PR), it will have a different expected value. (The MAC address is different. Yes, the MAC address is different on Mac. Ha ha, ha ha, gosh, I'm funny.)

  • Probably don't want to replace the switch because we need a do-nothing default for macos, smartos... A switch makes sense here, I think. I mean, you can make it work with an if, of course. But I think this more verbose code is actually more readable even though it does violate DRY. (I often don't value DRY much in tests, TBH.)

Even if I'm wrong about that evaluation... must...resist....scope creep.

@Trott

Trott Aug 7, 2017

Member

Some reasons to maybe not do that:

  • If we want to add macOS to the switch statement (in another PR), it will have a different expected value. (The MAC address is different. Yes, the MAC address is different on Mac. Ha ha, ha ha, gosh, I'm funny.)

  • Probably don't want to replace the switch because we need a do-nothing default for macos, smartos... A switch makes sense here, I think. I mean, you can make it work with an if, of course. But I think this more verbose code is actually more readable even though it does violate DRY. (I often don't value DRY much in tests, TBH.)

Even if I'm wrong about that evaluation... must...resist....scope creep.

@jasnell

jasnell approved these changes Aug 8, 2017

@refack

This comment has been minimized.

Show comment
Hide comment
@refack

refack Aug 8, 2017

Member

👍

(The MAC address is different. Yes, the MAC address is different on Mac. Ha ha, ha ha, gosh, I'm funny.)

Apple SMH...

Member

refack commented Aug 8, 2017

👍

(The MAC address is different. Yes, the MAC address is different on Mac. Ha ha, ha ha, gosh, I'm funny.)

Apple SMH...

@Trott

This comment has been minimized.

Show comment
Hide comment
@Trott

Trott Aug 10, 2017

Member

Landed in 2923ed1

Member

Trott commented Aug 10, 2017

Landed in 2923ed1

@Trott Trott closed this Aug 10, 2017

Trott added a commit to Trott/io.js that referenced this pull request Aug 10, 2017

test: improve check in test-os
The check for `os.networkInterfaces()` in `test-os.js` may be too
strict. It's apparently possible for a machine to be configured with
multiple IPv4 loopback interfaces. Increase specificity of filter to
check on only the object we expect.

PR-URL: nodejs#14655
Fixes: nodejs#14654
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

addaleax added a commit that referenced this pull request Aug 10, 2017

test: improve check in test-os
The check for `os.networkInterfaces()` in `test-os.js` may be too
strict. It's apparently possible for a machine to be configured with
multiple IPv4 loopback interfaces. Increase specificity of filter to
check on only the object we expect.

PR-URL: #14655
Fixes: #14654
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

icarter09 added a commit to icarter09/node that referenced this pull request Aug 12, 2017

test: improve check in test-os
The check for `os.networkInterfaces()` in `test-os.js` may be too
strict. It's apparently possible for a machine to be configured with
multiple IPv4 loopback interfaces. Increase specificity of filter to
check on only the object we expect.

PR-URL: nodejs#14655
Fixes: nodejs#14654
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

@addaleax addaleax referenced this pull request Aug 13, 2017

Merged

v8.4.0 proposal #14811

MylesBorins added a commit that referenced this pull request Sep 19, 2017

test: improve check in test-os
The check for `os.networkInterfaces()` in `test-os.js` may be too
strict. It's apparently possible for a machine to be configured with
multiple IPv4 loopback interfaces. Increase specificity of filter to
check on only the object we expect.

PR-URL: #14655
Fixes: #14654
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins added a commit that referenced this pull request Sep 19, 2017

test: improve check in test-os
The check for `os.networkInterfaces()` in `test-os.js` may be too
strict. It's apparently possible for a machine to be configured with
multiple IPv4 loopback interfaces. Increase specificity of filter to
check on only the object we expect.

PR-URL: #14655
Fixes: #14654
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

@MylesBorins MylesBorins referenced this pull request Sep 20, 2017

Merged

v6.11.4 proposal #15506

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment