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.

Sign up for GitHub

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

Jump to bottom

assert: update comments #10579

Closed
wants to merge 1 commit into from
Closed

assert: update comments #10579

wants to merge 1 commit into from

Conversation

kaicataldo
Copy link
Contributor

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)

@kaicataldo
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 nodejs-github-bot added the assert Issues and PRs related to the assert subsystem. label Jan 2, 2017
@Trott
Copy link
Member

Trott commented Jan 3, 2017

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
Copy link
Member

Trott commented Jan 3, 2017

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
Trott approved these changes Jan 3, 2017
View reviewed changes
Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

LGTM if CI linter is ✅

@Trott
Copy link
Member

Trott commented Jan 3, 2017

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

@Trott
Copy link
Member

Trott commented Jan 3, 2017

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
Copy link
Contributor 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
Copy link
Member

joyeecheung commented Jan 3, 2017 •
edited
Loading

@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
Copy link
Contributor Author

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

@joyeecheung
Copy link
Member

joyeecheung commented Jan 3, 2017 •
edited
Loading

@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
mhdawson approved these changes Jan 3, 2017
View reviewed changes
Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

jasnell pushed a commit that referenced this pull request Jan 4, 2017
@kaicataldo @jasnell
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@jasnell
Copy link
Member

jasnell commented Jan 4, 2017

Landed in 398229a

@jasnell jasnell closed this Jan 4, 2017
@kaicataldo 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
@kaicataldo @italoacasas
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 19, 2017
@kaicataldo @italoacasas
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@joyeecheung 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
@kaicataldo @italoacasas
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@italoacasas italoacasas mentioned this pull request Jan 25, 2017
Closed
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 27, 2017
@kaicataldo @italoacasas
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@italoacasas italoacasas mentioned this pull request Jan 29, 2017
Merged
@MylesBorins
Copy link
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
@kaicataldo @MylesBorins
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
MylesBorins pushed a commit that referenced this pull request Mar 7, 2017
@kaicataldo @MylesBorins
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
MylesBorins pushed a commit that referenced this pull request Mar 9, 2017
@kaicataldo @MylesBorins
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Mar 9, 2017
Merged
MylesBorins pushed a commit that referenced this pull request Mar 9, 2017
@kaicataldo @MylesBorins
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Mar 9, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mhdawson mhdawson mhdawson approved these changes

@benjamingr benjamingr benjamingr approved these changes

@Trott Trott Trott approved these changes

@jasnell jasnell jasnell approved these changes

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
@kaicataldo @Trott @joyeecheung @jasnell @MylesBorins @benjamingr @mhdawson @nodejs-github-bot