FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

process: fix calculation in process.uptime() by joyeecheung · Pull Request #26206 · nodejs/node · GitHub

/ node Public

process: fix calculation in process.uptime() - #26206

Closed
joyeecheung wants to merge 3 commits into
nodejs:masterfrom
joyeecheung:fix-uptime
ConversationCommits (3)ChecksFiles changed
Closed

process: fix calculation in process.uptime()#26206
joyeecheung wants to merge 3 commits into
nodejs:masterfrom
joyeecheung:fix-uptime

Conversation

joyeecheung commented Feb 19, 2019 •
edited
Loading

Copy link
Copy Markdown
Member

process: fix calculation in process.uptime()

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
Fixes: #26205

process: move test-process-uptime to parallel

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.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

nodejs-github-bot commented Feb 19, 2019

Copy link
Copy Markdown
Collaborator

@joyeecheung build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/2666/pipeline

nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Feb 19, 2019
joyeecheung changed the title Fix uptime process: fix calculation in process.uptime() Feb 19, 2019
joyeecheung mentioned this pull request Feb 19, 2019
Closed
addaleax approved these changes Feb 19, 2019
View reviewed changes

addaleax left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

process: move test-process-uptime to pummel

^ should this read to parallel?

Trott reviewed Feb 19, 2019
View reviewed changes
Comment thread test/parallel/test-process-uptime.js Outdated

Trott Feb 19, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

We don't need to use common.platformTimeout() at all if all we're checking is original < uptime. For that matter, this can probably use a value more like 5 than 100?

Trott approved these changes Feb 19, 2019
View reviewed changes

Trott left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM. (OK by me if my one comment/nit is ignored.)

joyeecheung commented Feb 19, 2019

Copy link
Copy Markdown
Member Author

@addaleax

^ should this read to parallel?

Yes. (This is what happens when I stay up at 2am 🤦‍♀️ )

Updated the test description and the timeout.
CI: https://ci.nodejs.org/job/node-test-pull-request/20886/

Trott added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Feb 19, 2019
process: fix calculation in process.uptime()
bd09f7f 
In nodejs#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: nodejs#26016
process: move test-process-uptime to parallel
a6ea194 
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.

Trott commented Feb 19, 2019

Copy link
Copy Markdown
Member

Tiny lint error: common assigned but never used in the test.

Trott reviewed Feb 19, 2019
View reviewed changes
Comment thread test/parallel/test-process-uptime.js Outdated

Trott Feb 19, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality
Suggested change
const common = require('../common');
require('../common');

Trott commented Feb 19, 2019

Copy link
Copy Markdown
Member

I'd like to fast-track this (to fix the failing test), but it's 3AM in China, so I'm going to push the tiny lint fix and re-run CI.

Trott commented Feb 19, 2019

Copy link
Copy Markdown
Member

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

Trott commented Feb 19, 2019

Copy link
Copy Markdown
Member

Please 👍 here if you support fast-tracking. @addaleax @richardlau

Trott added the fast-track PRs that do not need to wait for 48 hours to land. label Feb 19, 2019

Trott commented Feb 19, 2019

Copy link
Copy Markdown
Member

Landed in 129516d...5c9b37b. Thanks, @joyeecheung! 🎉

Trott closed this Feb 19, 2019
Trott pushed a commit to Trott/io.js that referenced this pull request Feb 19, 2019
process: fix calculation in process.uptime()
a32c574 
In nodejs#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: nodejs#26016

PR-URL: nodejs#26206
Fixes: nodejs#26205
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Trott pushed a commit to Trott/io.js that referenced this pull request Feb 19, 2019
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 <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
addaleax pushed a commit that referenced this pull request Feb 21, 2019
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 <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
addaleax pushed a commit that referenced this pull request Feb 21, 2019
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 <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
BridgeAR mentioned this pull request Feb 26, 2019
Merged
rvagg pushed a commit that referenced this pull request Feb 28, 2019
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 <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
rvagg pushed a commit that referenced this pull request Feb 28, 2019
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 <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Trott Trott approved these changes
addaleax addaleax approved these changes
richardlau richardlau approved these changes

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no outstanding review comments, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. fast-track PRs that do not need to wait for 48 hours to land.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

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

5 participants

Back | FazBrowse Home | New Git URL