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.

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

test: improve the code in test-process-cpuUsage #10714

Closed
wants to merge 1 commit into from

Conversation

edsadr
Copy link
Member

@edsadr edsadr commented Jan 9, 2017

  • validate the errors for assert.throws
  • use arrow functions
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

test

@nodejs-github-bot nodejs-github-bot added test Issues and PRs related to the tests. lts-watch-v6.x labels Jan 9, 2017
@@ -33,27 +33,57 @@ for (let i = 0; i < 10; i++) {
assert(diffUsage.system >= 0);
}

const invalidUserArgument = new RegExp('TypeError: value of user ' +
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add ^ and $ to these regular expressions.

@mscdex mscdex added the process Issues and PRs related to the process subsystem. label Jan 9, 2017
@edsadr
Copy link
Member Author

edsadr commented Jan 9, 2017

@cjihrig sorry, forgot that... just fixed it

@@ -33,27 +33,57 @@ for (let i = 0; i < 10; i++) {
assert(diffUsage.system >= 0);
}

const invalidUserArgument = new RegExp('^TypeError: value of user ' +
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason to not write these as:

const invalidUserArgument =
  /^TypeError: value of user property of argument is invalid$/

Copy link
Member Author

Choose a reason for hiding this comment

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

@cjihrig, honestIy would like to do it that way ... but can not find consensus about identation, got requests to change things when doing something like that in previous commits ... is there any document detailing the official code style? the linter seems to be fine with the previous commits but reviewers don't agree... so... would really love to understand when is fine to use an identation like the one you are suggesting and when I should keep it in line after the =

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe the official style is what I suggested (although I can't remember if the indentation on the next line is supposed to be 2 or 4 spaces).

Copy link
Member

Choose a reason for hiding this comment

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

I think 4 (i.e. an extra indent to separate it from any indented code on the next line)

Copy link
Member Author

Choose a reason for hiding this comment

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

would be really nice to have a guide for this...

Copy link
Member

Choose a reason for hiding this comment

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

@edsadr it would be great to have it documented, I'm not sure where, maybe in the test-writing guide for now? I think the easiest way to get started would be to raise a PR with whatever you think it should be, @mention nodejs/collaborators, and see what the consensus is.

Copy link
Member Author

Choose a reason for hiding this comment

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

@gibfahn thanks, I think is worth it to have a basic one and will work in something like that for a PR

* validate the errors for assert.throws
* use arrow functions
@edsadr
Copy link
Member Author

edsadr commented Jan 12, 2017

@cjihrig changed the RegExp now 🙂

@cjihrig
Copy link
Contributor

cjihrig commented Jan 12, 2017

@italoacasas
Copy link
Contributor

Landed c963094

italoacasas pushed a commit that referenced this pull request Jan 16, 2017
* validate the errors for assert.throws
* use arrow functions

PR-URL: #10714
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 18, 2017
* validate the errors for assert.throws
* use arrow functions

PR-URL: nodejs#10714
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 23, 2017
* validate the errors for assert.throws
* use arrow functions

PR-URL: nodejs#10714
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 25, 2017
* validate the errors for assert.throws
* use arrow functions

PR-URL: nodejs#10714
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 27, 2017
* validate the errors for assert.throws
* use arrow functions

PR-URL: nodejs#10714
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@italoacasas italoacasas mentioned this pull request Jan 29, 2017
MylesBorins pushed a commit that referenced this pull request Mar 8, 2017
* validate the errors for assert.throws
* use arrow functions

PR-URL: #10714
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Mar 8, 2017
* validate the errors for assert.throws
* use arrow functions

PR-URL: #10714
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Mar 9, 2017
* validate the errors for assert.throws
* use arrow functions

PR-URL: #10714
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Mar 9, 2017
MylesBorins pushed a commit that referenced this pull request Mar 9, 2017
* validate the errors for assert.throws
* use arrow functions

PR-URL: #10714
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Mar 9, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
process Issues and PRs related to the process subsystem. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants