Skip to content

test: fix conversion of microseconds in test - #14706

Closed
nickstanish wants to merge 1 commit into
nodejs:masterfrom
nickstanish:fix-process-cpuusage-test
Closed

test: fix conversion of microseconds in test#14706
nickstanish wants to merge 1 commit into
nodejs:masterfrom
nickstanish:fix-process-cpuusage-test

Conversation

@nickstanish

@nickstanishnickstanish commented Aug 9, 2017

Copy link
Copy Markdown
Contributor

Fixes: #8728

Changes the conversion for process cpuUsage test

Checklist
Affected core subsystem(s)

process

@nodejs-github-botnodejs-github-bot added the test Issues and PRs related to the tests. label Aug 9, 2017
Trott
Trott approved these changes Aug 9, 2017
View reviewed changes

@TrottTrott left a comment •
edited
Loading

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.

LGTM.

@Trott

Trott commented Aug 9, 2017

Copy link
Copy Markdown
Member

Hi, @nickstanish! Welcome, and thanks for the contribution!

Small change to the commit message (and I guess this PR title too): The subsystem here is test rather than process. So that should be the first word (before the colon) of the commit message and PR title. No big deal if it doesn't happen--whoever lands the PR can do it, but if you want to save them a few keystrokes, that would be great.

@Trott

Trott commented Aug 9, 2017

Copy link
Copy Markdown
Member

Pummel tests don't get run in Ci so only the linter results are relevant, but here's a CI run anyway...

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

CI has not been having a good week, but we should be able to get some linter results at least. :-D

@nickstanishnickstanish changed the title process: fix conversion of microseconds in testtest: fix conversion of microseconds in testAug 9, 2017
@nickstanish
nickstanishforce-pushed the fix-process-cpuusage-test branch from 7989ff7 to d0621a6CompareAugust 9, 2017 05:03
@mscdexmscdex added the process Issues and PRs related to the process subsystem. label Aug 9, 2017
@nickstanishnickstanish mentioned this pull request Aug 9, 2017
Closed
@pmuellr

pmuellr commented Aug 9, 2017

Copy link
Copy Markdown

thanks @nickstanish !!! LGTM

@Trott

Trott commented Aug 10, 2017

Copy link
Copy Markdown
Member

CI is good (only failure is a known issue unrelated to this).

Given the small nature of the change, the low risk (it's a test modification, and easy to revert if it turns out there is an unforeseen problem), and the copious approvals, I'm going to land this now rather than waiting the full 48 hours.

Trott pushed a commit to Trott/io.js that referenced this pull request Aug 10, 2017
@nickstanish@Trott
test: fix conversion of microseconds in test
a439cf4
PR-URL: nodejs#14706Fixes: nodejs#8728
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
@Trott

Trott commented Aug 10, 2017

Copy link
Copy Markdown
Member

Landed in a439cf4.

Thanks for the contribution! 🎉

@TrottTrott closed this Aug 10, 2017
addaleax pushed a commit that referenced this pull request Aug 10, 2017
@nickstanish@addaleax
test: fix conversion of microseconds in test
06ba2da
PR-URL: #14706Fixes: #8728
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
@addaleaxaddaleax mentioned this pull request Aug 13, 2017
Merged
MylesBorins pushed a commit that referenced this pull request Sep 19, 2017
@nickstanish@MylesBorins
test: fix conversion of microseconds in test
9d84641
PR-URL: #14706Fixes: #8728
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
@MylesBorinsMylesBorins mentioned this pull request Sep 20, 2017
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@TrottTrottTrott approved these changes
@lpincalpincalpinca approved these changes
@cjihrigcjihrigcjihrig approved these changes
@tniessentniessentniessen approved these changes
+1 more reviewer
@TimothyGuTimothyGuTimothyGu approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

processIssues and PRs related to the process subsystem.testIssues and PRs related to the tests.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

process.cpuUsage accuracy

10 participants

@nickstanish@Trott@pmuellr@lpinca@TimothyGu@cjihrig@tniessen@mscdex@MylesBorins@nodejs-github-bot