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: add comments describing every test #22424

Closed
wants to merge 2 commits into from

Conversation

ratracegrad
Copy link
Contributor

Providing consistency in tests by adding comments to describe the test's functionality.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Aug 21, 2018
@bcoe bcoe self-requested a review August 21, 2018 02:04
@vsemozhetbyt
Copy link
Contributor

Thank you! A tiny note: we usually try to add periods at the end of comments now.

@vsemozhetbyt
Copy link
Contributor

Also, it seems your commits are not attributed to your GitHub account now. Can you config your email setting as described here (see "Please make sure..." paragraph)?

@Trott
Copy link
Member

Trott commented Aug 21, 2018

@Trott
Copy link
Member

Trott commented Aug 21, 2018

Welcome, @ratracegrad, and thanks for the pull request!

Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

Can you config your email setting as described here (see "Please make sure..." paragraph)?

Just to be clear: This is voluntary, and only affects whether Github attributes the commit to you or not. You don't have to do anything. :)

@Trott
Copy link
Member

Trott commented Aug 21, 2018

The change looks good to me, but the commit message could be updated as it suggests (to me, at least) that this is adding comments to all test files rather than a single test file. This is something the person landing the PR can do, but if you want to take care of it for them, it will save someone a few keystrokes.

@Trott Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 22, 2018
Copy link
Contributor

@bcoe bcoe left a comment

Choose a reason for hiding this comment

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

this looks great, once we get the test suite running. If you go ahead and address folks' comments about the commit title and missing period, I'll go ahead and kick off tests again.

@addaleax
Copy link
Member

Landed in d8ef981 (with nits addressed), thanks for the PR! 🎉

@addaleax addaleax closed this Aug 23, 2018
addaleax pushed a commit that referenced this pull request Aug 23, 2018
PR-URL: #22424
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Ben Coe <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
targos pushed a commit that referenced this pull request Aug 24, 2018
PR-URL: #22424
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Ben Coe <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
addaleax pushed a commit that referenced this pull request Sep 3, 2018
PR-URL: #22424
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Ben Coe <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
targos pushed a commit that referenced this pull request Sep 12, 2018
PR-URL: #22424
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Ben Coe <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
targos pushed a commit that referenced this pull request Sep 19, 2018
PR-URL: #22424
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Ben Coe <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
@targos
Copy link
Member

targos commented Sep 20, 2018

Depends on #21875. Marking dont-land-on-v10.x for now.

targos pushed a commit that referenced this pull request Sep 25, 2018
PR-URL: #22424
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Ben Coe <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.