test: increase test coverage for os.js #14098

viktor-ku · 2017-07-06T05:39:35Z

Basically added tests for Symbol.toPrimitive cases

Statements from 82% to 94.12%
I think it is actually a 100% because some statements/branches are platform dependent
Functions from 66.67% to 100%

NB Obviously I ran tests, but seems like master branch has some tests failing in other modules. os module not failing though

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

test

gireeshpunathil · 2017-07-06T06:04:29Z

thanks, now you are at it, will you please accommodate AIX as well in the os.networkInterfaces(); section (Currently covering win and linux only)? If you don't have access to an AIX box, please let me know, I shall provide the format in which AIX output appears for this call.

Trott

The substantive additions at the bottom of the test file look good to me. Would be good to remove the unrelated style/formatting changes.

Trott · 2017-07-06T05:46:37Z

test/parallel/test-os.js

@@ -20,15 +20,16 @@
 // USE OR OTHER DEALINGS IN THE SOFTWARE.

 'use strict';
+


See https://github.com/nodejs/node/blob/master/doc/guides/writing-tests.md for our general guidelines on test style. The import of the common module should be on the line immediately after use strict. Inserting a blank line between common and the next module would be good, though.

Alright, I will change it back. I am still learning nodejs guides though

Trott · 2017-07-06T05:47:17Z

test/parallel/test-os.js

-  array: (value) => { assert.ok(Array.isArray(value)); },
+  string: (value) => assert.strictEqual(typeof value, 'string'),
+  number: (value) => assert.strictEqual(typeof value, 'number'),
+  array: (value) => assert.ok(Array.isArray(value)),


Removing the braces here changes the function to return a value. It doesn't matter in this case, but by returning a value, the code implies the return value is significant. Moreover, this is an unrelated change from increasing code coverage.

Removing the braces here changes the function to return a value. It doesn't matter in this case, but by returning a value, the code implies the return value is significant

Right, I have never thought that way

Trott · 2017-07-06T05:47:54Z

test/parallel/test-os.js

@@ -93,8 +94,9 @@ const release = os.release();
 is.string(release);
 assert.ok(release.length > 0);
 //TODO: Check format on more than just AIX
-if (common.isAix)
+if (common.isAix) {


Unrelated change. Our code base has many examples of both including and omitting braces in this type of situation. Probably best not to add this just so the next person can remove it etc.

It is still a little confusing to me because it is not obvious when to add braces and when not to. I have seen conditions with one line of code after it being with and without braces. Maybe we need to use some eslint rule so it will yell at us on some cases?

The only rule we currently have is to use braces when the contained statement is more than one line. I don't think it is necessary to enforce either behavior when braces are optional, but if it does not churn too much, we might as well lint it, even though I don't think we will get consensus on that.

I don't think we will get consensus on that

@tniessen you are probably right.

Sure I will remove them

viktor-ku · 2017-07-06T07:15:19Z

If you don't have access to an AIX box, please let me know, I shall provide the format in which AIX output appears for this call

@gireeshpunathil I will definitely need it

gireeshpunathil · 2017-07-06T08:02:23Z

I see 2 patterns in AIX:

Loopback interface configured, in which case it goes under lo0[0]
Loopback interface not configured. In which case only Ethernet entries are retrieved.

Pattern 1:

{ en0: 
   [ { address: 'X.X.X.X',
       netmask: 'Y.Y.Y.Y',
       family: 'IPv4',
       mac: '00:00:00:00:00:00',
       internal: false } ],
  lo0: 
   [ { address: '127.0.0.1',
       netmask: '0.0.0.1',
       family: 'IPv4',
       mac: 'xx:xx:xx:xx:xx:xx',
       internal: true },
     { address: '::1',
       netmask: '::700:0:205b:ea9',
       family: 'IPv6',
       mac: '00:00:00:01:10:bc',
       scopeid: 1,
       internal: true } ] }

Pattern 2:

{ en0: 
   [ { address: 'X.X.X.X',
       netmask: '0.0.0.0',
       family: 'IPv4',
       mac: '00:00:00:00:00:00',
       internal: false } ] }

Also I see one deviation from Linux: MAC address is non-zero on the loopback interface.

So the test logic may be on the lines of:

Retrieve the object
Check whether interfaces["lo0"] exists or not
If yes:
a. extract interfaces["lo0"][0] as the loopback entry.
b. nullify the mac field in the actual as well as the expected (as we cannot make a meaningful comparison here)
c. make comparison
If no, skip.

viktor-ku · 2017-07-06T13:02:24Z

@gireeshpunathil got it

viktor-ku · 2017-07-06T14:43:32Z

@gireeshpunathil are you sure about lo0 IPv4 netmask being 0.0.0.1 and not 255.0.0.0?

gireeshpunathil · 2017-07-06T14:53:10Z

yes, here is a portion of os.networkInterfaces() output:

  lo0: 
   [ { address: '127.0.0.1',
       netmask: '0.0.0.1',
       family: 'IPv4',

viktor-ku · 2017-07-06T15:44:22Z

@gireeshpunathil check out

comments have been addressed, dismissing my review, thanks

gireeshpunathil · 2017-07-07T03:43:58Z

@kuroljov - thanks. While I was validating your code in AIX, it revealed a new issue - the os.networkInterfaces() seem to be producing inconsistent results. I have captured this in #14119 .

This would mean you may either (a) hold this PR until #14119 is addressed, (b) go ahead with the PR without the AIX changes. The current changes in AIX will fail in CI. I recommend (a).

Thanks once again, catching the bug early being the essence of test cases, and this PR helped in that.

viktor-ku · 2017-07-07T15:16:49Z

@gireeshpunathil I will wait until someone answers to that issue there. However these AIX changes has nothing with increasing test coverage so I would prefer go without AIX changes but we will see.

Trott · 2017-07-07T18:13:27Z

@nodejs/testing

refack

2 tiny nits

refack · 2017-07-07T21:21:23Z

test/parallel/test-os.js

@@ -139,9 +162,13 @@ switch (platform) {
 const EOL = os.EOL;
 assert.ok(EOL.length > 0);


Now this could be removed

refack · 2017-07-07T21:23:21Z

test/parallel/test-os.js

+  [`${os.endianness}`, os.endianness()],
+  [`${os.tmpdir}`, os.tmpdir()],
+  [`${os.arch}`, os.arch()],
+  [`${os.platform}`, os.platform()]
 ].forEach((set) => assert.strictEqual(set[0], set[1]));


tiny nit: .forEach(([expected, actual]) => assert.strictEqual(expected, actual));

refack · 2017-07-07T21:27:47Z

Regarding the AIX fail, you can edit test/parallel/parallel.status and add:

[$system==aix]
test-os.js:                         : PASS,FLAKY

And we can land this now

refack · 2017-07-07T21:32:29Z

CI FWIW: https://ci.nodejs.org/job/node-test-pull-request/9033/

refack

LGTM

refack · 2017-07-08T18:27:16Z

New CI: https://ci.nodejs.org/job/node-test-pull-request/9041/
Last one had this error, let's see if it reproduces

not ok 225 parallel/test-os
  ---
  duration_ms: 0.139
  severity: fail
  stack: |-
    assert.js:55
      throw new errors.AssertionError({
      ^
    
    AssertionError [ERR_ASSERTION]: 2157244416 === 2157236224
        at __dirname.forEach (c:\workspace\node-test-binary-windows\RUN_SUBSET\0\VS_VERSION\vs2015-x86\label\win2008r2\test\parallel\test-os.js:225:27)
        at Array.forEach (<anonymous>)
        at Object.<anonymous> (c:\workspace\node-test-binary-windows\RUN_SUBSET\0\VS_VERSION\vs2015-x86\label\win2008r2\test\parallel\test-os.js:225:3)
        at Module._compile (module.js:569:30)
        at Object.Module._extensions..js (module.js:580:10)
        at Module.load (module.js:503:32)
        at tryModuleLoad (module.js:466:12)
        at Function.Module._load (module.js:458:3)
        at Function.Module.runMain (module.js:605:10)
        at startup (bootstrap_node.js:158:16)
  ...

https://ci.nodejs.org/job/node-test-binary-windows/9697/

refack · 2017-07-08T18:30:51Z

From last run same error on ubuntu1604-arm64,
Probably from one of:

[os.freemem, os.freemem()],
[os.totalmem, os.totalmem()],
[os.uptime, os.uptime()],

Probably need to add tolerance to the values, and change the error message.

not ok 893 parallel/test-os
  ---
  duration_ms: 0.628
  severity: fail
  stack: |-
    assert.js:55
      throw new errors.AssertionError({
      ^
    
    AssertionError [ERR_ASSERTION]: 129072861184 === 129070764032
        at __dirname.forEach (/home/iojs/build/workspace/node-test-commit-arm/nodes/ubuntu1604-arm64/test/parallel/test-os.js:225:27)
        at Array.forEach (<anonymous>)
        at Object.<anonymous> (/home/iojs/build/workspace/node-test-commit-arm/nodes/ubuntu1604-arm64/test/parallel/test-os.js:225:3)
        at Module._compile (module.js:569:30)
        at Object.Module._extensions..js (module.js:580:10)
        at Module.load (module.js:503:32)
        at tryModuleLoad (module.js:466:12)
        at Function.Module._load (module.js:458:3)
        at Function.Module.runMain (module.js:605:10)
        at startup (bootstrap_node.js:158:16)
  ...

https://ci.nodejs.org/job/node-test-commit-arm/10771/nodes=ubuntu1604-arm64/

refack · 2017-07-08T18:35:06Z

test/parallel/test-os.js

 [
+  [+os.freemem, os.freemem()],


Idea:
replace all the pairs with just the field name and last line to

].forEach((field) => assert.strictEqual(os[field], os[field](), `os.${field} failed Symbol.toPrimitive test`));

@refack there are sometimes string sometimes number so the check should be more complicated than just os[field]. So I guess it's not worth it and the current assertion is not that bad

Since the *mem & uptime can't be compared with strictEqual (the values can change between the two calls), they should be moved to their own loop anyway.

I agree with @refack that it would be very good to add the field name as it is difficult to know otherwise from what entry a failure is actually coming from.
This could also be done by either adding a string to each entry you are iterating over as a third entry or by just using the index of the forEach loop as in Entry ${i} failed but I prefer what @refack suggests.

I personally would prefer a third alternative - to just remove the loop altogether as it is unnecessary.
We would even have less lines if we use e.g. assert.strictEqual(${os.endianness}, os.endianness()) directly instead of values from the loop and it would be clear where the error is coming from.

I also do not see any way to compare freemem or uptime without those being flaky. So please remove those two entries again.

viktor-ku · 2017-07-21T15:25:15Z

I've lost an access to my working pc and got back it just now. Sorry for that. I will push changes soon

BridgeAR · 2017-08-30T21:28:04Z

Ping @kuroljov

viktor-ku · 2017-08-31T19:45:36Z

Yeah I am sorry. I remember everything and I will finish this PR until 16.09

viktor-ku · 2017-09-14T17:32:47Z

It has been 583 commits since my last push here. Fascinating 👍

cjihrig · 2017-09-14T18:13:34Z

It has been 583 commits since my last push here.

We call that a slow Tuesday in this repository.

Also, this needs a rebase.

refack · 2017-09-15T14:53:55Z

CI: https://ci.nodejs.org/job/node-test-commit/12384/

Statements from 82% to 94.12% Functions from 66.67% to 100%

Remove it because after 2 months it was implemented actually. Started with this PR though

viktor-ku · 2017-09-25T17:32:09Z

Replaced to + and string templates. Completely forgot about them last time

refack

LGTM
Left a few comments

refack · 2017-09-25T18:04:56Z

test/parallel/parallel.status

@@ -18,4 +18,3 @@ test-npm-install:      PASS,FLAKY
 [$system==solaris] # Also applies to SmartOS

 [$system==freebsd]
-


This shows two lines at the end of the file (i.e. the file ends in \n\n - one shows up as a new line, and the other as an EOF \n which is required...)

refack · 2017-09-25T18:09:41Z

test/parallel/test-os.js

+
+assert.strictEqual(+os.totalmem, os.totalmem());
+
+// At least we can check that these values are numbers


Suggestion: change comment to:

// Assert that the following values are coercible to numbers.

refack · 2017-09-25T18:12:00Z

test/parallel/test-os.js

+      family: 'IPv4',
+      internal: true,
+      cidr: '127.0.0.1/8'
+    }];
    assert.deepStrictEqual(actual, expected);
    break;
  }


Should add a default:

default: common.printSkipMessage(`Skipping assertion of 'os.networkInterfaces()' on ${platform}`); break;

refack · 2017-09-25T21:49:43Z

CI: https://ci.nodejs.org/job/node-test-commit/12599/

BridgeAR · 2017-10-02T03:18:31Z

test/parallel/test-os.js

  assert.ok(/^\d+\.\d+$/.test(release));
+}


This is a unrelated change and should be reverted.

BridgeAR · 2017-10-02T03:57:22Z

Landed in ea9322168cb7042ffb83669ab2934b006af10221

I addressed all the comments while landing.

BridgeAR · 2017-10-02T04:08:44Z

Arg, I made a mistake here and removed the landed commit again. Reopening.

PR-URL: #14098 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: James M Snell <[email protected]>

refack · 2017-10-02T17:49:06Z

Landed in 22f95e7

refack · 2017-10-02T17:49:38Z

@kuroljov Thanks!

PR-URL: #14098 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: James M Snell <[email protected]>

PR-URL: nodejs/node#14098 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: James M Snell <[email protected]>

PR-URL: #14098 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: James M Snell <[email protected]>

MylesBorins · 2017-10-17T19:59:16Z

This does not land cleanly in LTS. Please feel free to manually backport by following the guide. Please also feel free to replace do-not-land if it is being backported

nodejs-github-bot added the test Issues and PRs related to the tests. label Jul 6, 2017

mscdex added the os Issues and PRs related to the os subsystem. label Jul 6, 2017

Trott previously requested changes Jul 6, 2017

View reviewed changes

refack approved these changes Jul 7, 2017

View reviewed changes

refack approved these changes Jul 8, 2017

View reviewed changes

refack suggested changes Jul 8, 2017

View reviewed changes

viktor-ku mentioned this pull request Sep 14, 2017

aix: add netmask, mac address into net interfaces libuv/libuv#1410

Closed

refack approved these changes Sep 15, 2017

View reviewed changes

refack self-assigned this Sep 15, 2017

viktor-ku added 6 commits September 25, 2017 20:24

test: increase test coverage for os.js

bf4b193

Statements from 82% to 94.12% Functions from 66.67% to 100%

test: add os.networkInterfaces test case for aix

3233b4d

test: add PASS,FLAKY in parallel.status

4c6aaab

test: Resolve conflicts, remove aix test case

fdd2780

Remove it because after 2 months it was implemented actually. Started with this PR though

Unset aix flaky, remove loop for toPrimitive tests

c923d4e

Change to + and string templates

eced626

refack approved these changes Sep 25, 2017

View reviewed changes

jasnell approved these changes Sep 25, 2017

View reviewed changes

BridgeAR reviewed Oct 2, 2017

View reviewed changes

test/parallel/test-os.js

assert.ok(/^\d+\.\d+$/.test(release));

}

Copy link

Member

BridgeAR Oct 2, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a unrelated change and should be reverted.

BridgeAR closed this Oct 2, 2017

BridgeAR reopened this Oct 2, 2017

refack pushed a commit that referenced this pull request Oct 2, 2017

test: increase test coverage for os.js

7069e63

PR-URL: #14098 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: James M Snell <[email protected]>

refack closed this Oct 2, 2017

refack removed their assignment Oct 2, 2017

MylesBorins pushed a commit that referenced this pull request Oct 3, 2017

test: increase test coverage for os.js

3608983

PR-URL: #14098 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: James M Snell <[email protected]>

MylesBorins mentioned this pull request Oct 3, 2017

v8.7.0 proposal #15762

Merged

MylesBorins pushed a commit that referenced this pull request Oct 3, 2017

test: increase test coverage for os.js

efa83f7

PR-URL: #14098 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: James M Snell <[email protected]>

MylesBorins pushed a commit that referenced this pull request Oct 11, 2017

test: increase test coverage for os.js

1582260

PR-URL: #14098 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: James M Snell <[email protected]>

MylesBorins added the dont-land-on-v6.x label Oct 17, 2017

		@@ -20,15 +20,16 @@
		// USE OR OTHER DEALINGS IN THE SOFTWARE.

		'use strict';

		@@ -139,9 +162,13 @@ switch (platform) {
		const EOL = os.EOL;
		assert.ok(EOL.length > 0);

		@@ -18,4 +18,3 @@ test-npm-install: PASS,FLAKY
		[$system==solaris] # Also applies to SmartOS

		[$system==freebsd]


		assert.strictEqual(+os.totalmem, os.totalmem());

		// At least we can check that these values are numbers

test: increase test coverage for os.js #14098

test: increase test coverage for os.js #14098

Conversation

viktor-ku commented Jul 6, 2017 • edited Loading

Checklist

Affected core subsystem(s)

gireeshpunathil commented Jul 6, 2017

Trott left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

viktor-ku commented Jul 6, 2017

gireeshpunathil commented Jul 6, 2017

viktor-ku commented Jul 6, 2017

viktor-ku commented Jul 6, 2017

gireeshpunathil commented Jul 6, 2017

viktor-ku commented Jul 6, 2017

gireeshpunathil commented Jul 7, 2017

viktor-ku commented Jul 7, 2017

Trott commented Jul 7, 2017

refack left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

refack commented Jul 7, 2017

refack commented Jul 7, 2017

refack left a comment

Choose a reason for hiding this comment

refack commented Jul 8, 2017

refack commented Jul 8, 2017

Choose a reason for hiding this comment

viktor-ku Sep 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

viktor-ku commented Jul 21, 2017

BridgeAR commented Aug 30, 2017

viktor-ku commented Aug 31, 2017

viktor-ku commented Sep 14, 2017

cjihrig commented Sep 14, 2017

refack commented Sep 15, 2017

viktor-ku commented Sep 25, 2017

refack left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

refack commented Sep 25, 2017

Choose a reason for hiding this comment

BridgeAR commented Oct 2, 2017

BridgeAR commented Oct 2, 2017

refack commented Oct 2, 2017

refack commented Oct 2, 2017

MylesBorins commented Oct 17, 2017

viktor-ku commented Jul 6, 2017 •

edited

Loading

viktor-ku Sep 14, 2017 •

edited

Loading