Recently-introduced breaking change (unintentional?) in process.uptime()

[Trott]

https://ci.nodejs.org/job/node-test-commit-custom-suites/879/default/console

test-rackspace-ubuntu1604-x64-1

00:09:47 not ok 95 pummel/test-process-uptime
00:09:47   ---
00:09:47   duration_ms: 0.214
00:09:47   severity: fail
00:09:47   exitcode: 1
00:09:47   stack: |-
00:09:47     107252.491
00:09:47     assert.js:351
00:09:47         throw err;
00:09:47         ^
00:09:47     
00:09:48     AssertionError [ERR_ASSERTION]: The expression evaluated to a falsy value:
00:09:48     
00:09:48       assert.ok(process.uptime() <= 2)
00:09:48     
00:09:48         at Object.<anonymous> (/home/iojs/build/workspace/node-test-commit-custom-suites/default/test/pummel/test-process-uptime.js:27:8)
00:09:48         at Module._compile (internal/modules/cjs/loader.js:746:30)
00:09:48         at Object.Module._extensions..js (internal/modules/cjs/loader.js:757:10)
00:09:48         at Module.load (internal/modules/cjs/loader.js:638:32)
00:09:48         at tryModuleLoad (internal/modules/cjs/loader.js:570:12)
00:09:48         at Function.Module._load (internal/modules/cjs/loader.js:562:3)
00:09:48         at Function.Module.runMain (internal/modules/cjs/loader.js:809:12)
00:09:48         at internal/main/run_main_module.js:21:11
00:09:48   ...

[Trott]

Actually, this looks like a genuine bug and not something flaky.

[Trott]

fd0a861 seems like a likely culprit. /cc @joyeecheung

[addaleax]

Btw, the test doesn’t look like something that should go into pummel/?

[Trott]

Btw, the test doesn’t look like something that should go into pummel/?

Agreed. It can at least go in sequential and might be perfectly fine in parallel, although the check that the initial call is <= 2 might need to be <= common.platformTimeout(2). (Would need empirical testing to find out.) The long 2000ms timer can also likely be shortened. In fact, we can probably get rid of it entirely and just check that a subsequent call to uptime() returns a number greater than the previous call (and not too much greater than the previous call perhaps).

[joyeecheung]

If we are only testing that the uptime is in the right unit, testing the initial value < 1000 should be fine? Or something like common.platformTimeout(10)?

[addaleax]

If we are only testing that the uptime is in the right unit, testing the initial value < 1000 should be fine?

The 2 seconds that are currently in the test doesn’t seem like a bad value to me, but I think we had problems in the recent past with a test that relied on a fixed startup time limit. (I unfortunately don’t remember which test that was, so I don’t know what the problematic value was).

[joyeecheung]

@addaleax The current value in test-performance.js is 15 seconds, that seems to work fine?

[joyeecheung]

Fix in #26206