test: improve check in test-os - #14655
Conversation
Sorry, something went wrong.
Sorry, something went wrong.
There was a problem hiding this comment.
Not a big deal but FWIW, the fltr const name is now inconsistent with the filter used in the win32 case
Sorry, something went wrong.
There was a problem hiding this comment.
Can you leave the name as filter and just break the line on the && operator.
Sorry, something went wrong.
There was a problem hiding this comment.
@cjihrig Sure, done.
Sorry, something went wrong.
There was a problem hiding this comment.
Can you leave the name as filter and just break the line on the && operator.
Sorry, something went wrong.
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: nodejs#14654
| 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', |
There was a problem hiding this comment.
[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']
Sorry, something went wrong.
There was a problem hiding this comment.
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.
Sorry, something went wrong.
|
👍
Apple SMH... |
Sorry, something went wrong.
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>
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>
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>
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
Affected core subsystem(s)
test os