Skip to content
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.

Sign up for GitHub

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

Jump to bottom

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

Closed
Trott opened this issue Feb 19, 2019 · 8 comments
Closed

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

Trott opened this issue Feb 19, 2019 · 8 comments
Labels
flaky-test Issues and PRs related to the tests with unstable failures on the CI.

Comments

@Trott
Copy link
Member

Trott commented Feb 19, 2019

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 Trott added the flaky-test Issues and PRs related to the tests with unstable failures on the CI. label Feb 19, 2019
@Trott
Copy link
Member Author

Trott commented Feb 19, 2019

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

@Trott Trott changed the title Investigate flaky pummel/test-process-uptime Recently-introduced bug in process.uptime() Feb 19, 2019
@Trott Trott changed the title Recently-introduced bug in process.uptime() Recently-introduced breaking change (unintentional?) in process.uptime() Feb 19, 2019
@Trott
Copy link
Member Author

Trott commented Feb 19, 2019

fd0a861 seems like a likely culprit. /cc @joyeecheung

@Trott Trott mentioned this issue Feb 19, 2019
Closed
4 tasks
@addaleax
Copy link
Member

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

@Trott
Copy link
Member Author

Trott commented Feb 19, 2019

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
Copy link
Member

joyeecheung commented Feb 19, 2019 •
edited
Loading

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
Copy link
Member

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
Copy link
Member

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

@joyeecheung joyeecheung mentioned this issue Feb 19, 2019
Closed
3 tasks
@joyeecheung
Copy link
Member

Fix in #26206

Trott pushed a commit to Trott/io.js that referenced this issue Feb 19, 2019
@joyeecheung @Trott
process: move test-process-uptime to parallel
5c9b37b 
In addition, do not make too many assumptions about the startup
time and timer latency in test-process-uptime. Instead only test
that the value is likely in the correct unit (seconds) and it should
be increasing in subsequent calls.

PR-URL: nodejs#26206
Fixes: nodejs#26205
Refs: nodejs#26016
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
@Trott Trott closed this as completed in a32c574 Feb 19, 2019
addaleax pushed a commit that referenced this issue Feb 21, 2019
@joyeecheung @addaleax
process: fix calculation in process.uptime()
14b8eb5 
In #26016 the result returned
by process.uptime() was mistakenly set to be based in the wrong
unit. This patch fixes the calculation and makes sure the returned
value is in seconds.

Refs: #26016

PR-URL: #26206
Fixes: #26205
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
addaleax pushed a commit that referenced this issue Feb 21, 2019
@joyeecheung @addaleax
process: move test-process-uptime to parallel
356923f 
In addition, do not make too many assumptions about the startup
time and timer latency in test-process-uptime. Instead only test
that the value is likely in the correct unit (seconds) and it should
be increasing in subsequent calls.

PR-URL: #26206
Fixes: #26205
Refs: #26016
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
rvagg pushed a commit that referenced this issue Feb 28, 2019
@joyeecheung @rvagg
process: fix calculation in process.uptime()
fde4011 
In #26016 the result returned
by process.uptime() was mistakenly set to be based in the wrong
unit. This patch fixes the calculation and makes sure the returned
value is in seconds.

Refs: #26016

PR-URL: #26206
Fixes: #26205
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
rvagg pushed a commit that referenced this issue Feb 28, 2019
@joyeecheung @rvagg
process: move test-process-uptime to parallel
cd10e25 
In addition, do not make too many assumptions about the startup
time and timer latency in test-process-uptime. Instead only test
that the value is likely in the correct unit (seconds) and it should
be increasing in subsequent calls.

PR-URL: #26206
Fixes: #26205
Refs: #26016
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
flaky-test Issues and PRs related to the tests with unstable failures on the CI.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

process: fix calculation in process.uptime() joyeecheung/node
3 participants
@Trott @addaleax @joyeecheung