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

assert: update comments by kaicataldo · Pull Request #10579 · nodejs/node · GitHub

/ node Public

assert: update comments - #10579

Closed
kaicataldo wants to merge 1 commit into
nodejs:masterfrom
kaicataldo:update-assert-comments
ConversationCommits (1)ChecksFiles changed
Closed

assert: update comments#10579
kaicataldo wants to merge 1 commit into
nodejs:masterfrom
kaicataldo:update-assert-comments

Conversation

kaicataldo commented Jan 2, 2017

Copy link
Copy Markdown

Remove the numbers from the comments to make it clear that
assert does not follow the
CJS spec.
Additionally, clean up the existing comments for consistent
formatting/language and ease of reading.

Fixes: #9063

Checklist

Apologies in advance for this, but I've been trying to run the following tests on my machine and keep seeing the same two tests time out. Any guidance on how I might be able to run them locally would be greatly appreciated! This PR only affects JS comments and I was able to run make lint successfully.

  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

assert (though only comments have been changed)

assert: update comments
16e7863 
Remove the numbers from the comments to make it clear that
assert does not follow the
[CJS spec](http://wiki.commonjs.org/wiki/Unit_Testing/1.0).
Additionally, clean up the existing comments for consistent
formatting/language and ease of reading.

Fixes: nodejs#9063
nodejs-github-bot added the assert Issues and PRs related to the assert subsystem. label Jan 2, 2017

Trott commented Jan 3, 2017

Copy link
Copy Markdown
Member

Partial duplicate of #10282 but I'd be OK landing this anyway so that the comment changes stick even if we end up not landing or having to revert that PR.

Trott commented Jan 3, 2017

Copy link
Copy Markdown
Member

I've been trying to run the following tests on my machine and keep seeing the same two tests time out.

Which tests? And what OS are you running?

Trott approved these changes Jan 3, 2017
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 if CI linter is ✅

Trott commented Jan 3, 2017

Copy link
Copy Markdown
Member

CI lint: https://ci.nodejs.org/job/node-test-linter/6148/

Trott commented Jan 3, 2017

Copy link
Copy Markdown
Member

CI lint is green. This seems minor enough that I would be OK landing it without the 48-hour wait period if others felt the same way. (On the other hand, no real reason to rush it either, so
¯\(ツ)/¯.)

kaicataldo commented Jan 3, 2017

Copy link
Copy Markdown
Author

Which tests? And what OS are you running?

I was trying to run make -j4 test on OSX 10.11.6. Is that the right test command?

joyeecheung commented Jan 3, 2017 •
edited
Loading

Copy link
Copy Markdown
Member

@Trott Yeah I think it would be better to let this one go ahead too.
@kaicataldo That's the right command. The failed tests would print out a line saying how you can run this test separately(like out/Release/node test/parallel/test-xxx.js or something). If you bump into the time out again running this command, then the test does have some problems. But usually these tests stop being timed out after a restart for me (on OSX 10.11.6 too).

EDIT: not necessarily -j4 if you have different number of CPU cores. But that shouldn't affect the outcome of the tests, just the time you spend running them.

kaicataldo commented Jan 3, 2017

Copy link
Copy Markdown
Author

@joyeecheung Awesome, thanks for the info! Looking to contribute more, so this is definitely really helpful :)

joyeecheung commented Jan 3, 2017 •
edited
Loading

Copy link
Copy Markdown
Member

@kaicataldo Also you can do tools/test.py test/parallel/test-xxx.js(EDIT: tools/test.py parallel/test-xxx) if you want them to be regulated the way make test do for you(time outs, number of processes.. etc.). You can run tools/test.py -h to see more options.

mhdawson approved these changes Jan 3, 2017
View reviewed changes

mhdawson 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

jasnell pushed a commit that referenced this pull request Jan 4, 2017
assert: update comments
398229a 
Remove the numbers from the comments to make it clear that
assert does not follow the
[CJS spec](http://wiki.commonjs.org/wiki/Unit_Testing/1.0).
Additionally, clean up the existing comments for consistent
formatting/language and ease of reading.

PR-URL: #10579
Fixes: #9063
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

jasnell commented Jan 4, 2017

Copy link
Copy Markdown
Member

Landed in 398229a

jasnell closed this Jan 4, 2017
kaicataldo deleted the update-assert-comments branch January 4, 2017 19:27
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 18, 2017
assert: update comments
6030faf 
Remove the numbers from the comments to make it clear that
assert does not follow the
[CJS spec](http://wiki.commonjs.org/wiki/Unit_Testing/1.0).
Additionally, clean up the existing comments for consistent
formatting/language and ease of reading.

PR-URL: nodejs#10579
Fixes: nodejs#9063
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 19, 2017
assert: update comments
4edd3c2 
Remove the numbers from the comments to make it clear that
assert does not follow the
[CJS spec](http://wiki.commonjs.org/wiki/Unit_Testing/1.0).
Additionally, clean up the existing comments for consistent
formatting/language and ease of reading.

PR-URL: nodejs#10579
Fixes: nodejs#9063
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
joyeecheung mentioned this pull request Jan 21, 2017
Closed
3 tasks
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 24, 2017
assert: update comments
480aad8 
Remove the numbers from the comments to make it clear that
assert does not follow the
[CJS spec](http://wiki.commonjs.org/wiki/Unit_Testing/1.0).
Additionally, clean up the existing comments for consistent
formatting/language and ease of reading.

PR-URL: nodejs#10579
Fixes: nodejs#9063
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
italoacasas mentioned this pull request Jan 25, 2017
Closed
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 27, 2017
assert: update comments
b2d0c44 
Remove the numbers from the comments to make it clear that
assert does not follow the
[CJS spec](http://wiki.commonjs.org/wiki/Unit_Testing/1.0).
Additionally, clean up the existing comments for consistent
formatting/language and ease of reading.

PR-URL: nodejs#10579
Fixes: nodejs#9063
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
italoacasas mentioned this pull request Jan 29, 2017
Merged

MylesBorins commented Mar 7, 2017

Copy link
Copy Markdown
Contributor

I've landed this both in v4 and v6. Please let me know if that was a mistake

MylesBorins pushed a commit that referenced this pull request Mar 7, 2017
assert: update comments
33a3861 
Remove the numbers from the comments to make it clear that
assert does not follow the
[CJS spec](http://wiki.commonjs.org/wiki/Unit_Testing/1.0).
Additionally, clean up the existing comments for consistent
formatting/language and ease of reading.

PR-URL: #10579
Fixes: #9063
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
MylesBorins pushed a commit that referenced this pull request Mar 7, 2017
assert: update comments
075de21 
Remove the numbers from the comments to make it clear that
assert does not follow the
[CJS spec](http://wiki.commonjs.org/wiki/Unit_Testing/1.0).
Additionally, clean up the existing comments for consistent
formatting/language and ease of reading.

PR-URL: #10579
Fixes: #9063
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
MylesBorins pushed a commit that referenced this pull request Mar 9, 2017
assert: update comments
7ecfe49 
Remove the numbers from the comments to make it clear that
assert does not follow the
[CJS spec](http://wiki.commonjs.org/wiki/Unit_Testing/1.0).
Additionally, clean up the existing comments for consistent
formatting/language and ease of reading.

PR-URL: #10579
Fixes: #9063
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
MylesBorins mentioned this pull request Mar 9, 2017
Merged
MylesBorins pushed a commit that referenced this pull request Mar 9, 2017
assert: update comments
994f562 
Remove the numbers from the comments to make it clear that
assert does not follow the
[CJS spec](http://wiki.commonjs.org/wiki/Unit_Testing/1.0).
Additionally, clean up the existing comments for consistent
formatting/language and ease of reading.

PR-URL: #10579
Fixes: #9063
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
MylesBorins mentioned this pull request Mar 9, 2017
Merged
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

jasnell jasnell approved these changes
Trott Trott approved these changes
benjamingr benjamingr approved these changes
+1 more reviewer
mhdawson mhdawson approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

assert Issues and PRs related to the assert subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

assert.js rule 7.4 (similar to CJS rule 7.3)

8 participants

Back | FazBrowse Home | New Git URL