Skip to content

Commit

Permalink
test: check memoryUsage properties
Browse files Browse the repository at this point in the history 
The properties on memoryUsage were not checked before,
this commit checks them.

PR-URL: #5546
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
  • Loading branch information
@geek
geek committed Mar 3, 2016
1 parent 7ae3dfb commit 7d3a7ea
Showing 1 changed file with 3 additions and 1 deletion.
4 changes: 3 additions & 1 deletion test/parallel/test-memory-usage.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,4 +3,6 @@ require('../common');
var assert = require('assert');

var r = process.memoryUsage();
assert.equal(true, r['rss'] > 0);
assert.ok(r.rss > 0);
assert.ok(r.heapTotal > 0);
assert.ok(r.heapUsed > 0);

8 comments on commit 7d3a7ea

@ChALkeR
Copy link
Member

@ChALkeR ChALkeR commented on 7d3a7ea Mar 8, 2016

Choose a reason for hiding this comment

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

Note that this commit does not include full PR-URL, it has:

PR-URL: #5546

while it's supposed to be:

PR-URL: https://github.com/nodejs/node/pull/5546

@trevnorris
Copy link
Contributor

Choose a reason for hiding this comment

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

Also the commit message is too long. Max of 50 chars. Rest should be placed in the commit message body.

@geek
Copy link
Member Author

@geek geek commented on 7d3a7ea Mar 11, 2016

Choose a reason for hiding this comment

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

I appreciate the feedback.

If the full URL is so important, shouldn't we be using it for release commits as well: 7b0a83d ?

Is it Ref or Refs, I see it both ways in our commits?

@jasnell
Copy link
Member

Choose a reason for hiding this comment

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

Yep, sometimes commits land without the appropriate PR metadata. It happens. When it does (and someone catches it) the usual approach is to add a comment in the commit itself that points out the issue.

The reason for the full PR-URL is that our release tools use the full URL to build out a proper changelog. Without the appropriate metadata the releaser has to go chase things down which is annoying.

FWIW, The style guide for commits is already documented in the CONTRIBUTING.md doc: https://github.com/nodejs/node/blob/master/CONTRIBUTING.md#step-3-commit

@geek
Copy link
Member Author

@geek geek commented on 7d3a7ea Mar 11, 2016

Choose a reason for hiding this comment

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

@jasnell thanks, I will update the collaborator guide to refer to it.

@geek
Copy link
Member Author

@geek geek commented on 7d3a7ea Mar 11, 2016

Choose a reason for hiding this comment

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

@jasnell I added a PR to help with the docs around this: #5661

Do you know if there is a way to lint commit messages, this would be a better solution going forward. Especially with my mistake around not adding an extra newline on the title -> body :/

@MylesBorins
Copy link
Contributor

Choose a reason for hiding this comment

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

@geek I'm researching into how we could do that with a git hook

--> https://github.com/mozilla/fjord/blob/master/bin/hooks/lint.commit-msg
--> https://git-scm.com/book/en/v2/Customizing-Git-An-Example-Git-Enforced-Policy

@jasnell
Copy link
Member

Choose a reason for hiding this comment

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

I started on a commit linter tool but haven't had any time to finish it up. I'll be trying to get back to it here in the next week or two.

Please sign in to comment.