doc: added reference to test coverage info #19825
Closed
Conversation
This file contains 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
Added a reference to test coverage doc since its useful to have it here as well.
mithunsasidharan
changed the title
vsemozhetbyt
reviewed
Apr 5, 2018
doc/guides/writing-tests.md
Outdated
|
|
||
| ### Test Coverage | ||
|
|
||
| Details on generating a test coverage report can be accessed [here].(https://github.com/nodejs/node/blob/master/doc/guides/contributing/pull-requests.md#test-coverage) |
There was a problem hiding this comment.
- It seems all this section should go before the bottom references block.
- The period between
[here]and(https://...)breaks link rendering.
There was a problem hiding this comment.
- And maybe it should be wrapped before
[here]to avoid the long line.
Added a reference to test coverage doc since its useful to have it here as well.
|
@vsemozhetbyt @thefourtheye thanks for the review. I've updated the PR with the review changes suggested. |
thefourtheye
approved these changes
Apr 5, 2018
lpinca
reviewed
Apr 5, 2018
doc/guides/writing-tests.md
Outdated
| @@ -380,11 +380,16 @@ and tearing it down after the tests have finished. | |||
| It also contains a helper to create arguments to be passed into Node.js. It | |||
| will depend on what is being tested if this is required or not. | |||
|
|
|||
| ### Test Coverage | |||
|
|
|||
| Details on generating test coverage report can be accessed in the [test coverage report][]. | |||
There was a problem hiding this comment.
I think the linter will complain that this line is too long.
Added a reference to test coverage doc since its useful to have it here as well.
|
@lpinca : You're right... I've update the PR to fix that. Kindly review now. Thank you. |
lpinca
approved these changes
Apr 5, 2018
vsemozhetbyt
added
fast-track
vsemozhetbyt
approved these changes
Apr 5, 2018
|
Non-blocking nit: if somebody feels that this sounds a bit tautological or recursive, please, suggest rewording: "Details on generating test coverage report can be accessed in the test coverage report." |
jasnell
approved these changes
Apr 5, 2018
|
@vsemozhetbyt Yeah, that wording seems a bit awkward. How about something like this?: To generate a test coverage report, see the [Test Coverage section of the Pull Requests guide][].Additionally, text should probably be added pointing out that people can nightly test coverage reports (for Linux) are available at coverage.nodejs.org. |
|
+1 on adding reference to the nightly coverage reports as well, jut not 100% necessary in this PR. |
|
@Trott : Do you want me to change it to #19825 (comment) ? |
Added a reference to test coverage doc since its useful to have it here as well.
|
@Trott : Thanks. I've updated the PR as per your suggestions. Kindly review. |
trivikr
approved these changes
Apr 6, 2018
BridgeAR
approved these changes
Apr 6, 2018
Trott
approved these changes
Apr 6, 2018
|
Landed in 0c55abf |
trivikr
pushed a commit
that referenced
this pull request
Apr 6, 2018
Added a reference to test coverage doc since its useful to have it here as well PR-URL: #19825 Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Rich Trott <[email protected]>
targos
pushed a commit
that referenced
this pull request
Apr 12, 2018
Added a reference to test coverage doc since its useful to have it here as well PR-URL: #19825 Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Rich Trott <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Added a reference to test coverage doc since its useful to have it here as well.
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes