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

doc: Add example to test doc for clarity #27561

Closed

Conversation

addityasingh
Copy link
Contributor

Add example to the test doc for clarity

Checklist

@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. doc Issues and PRs related to the documentations. labels May 4, 2019
BUILDING.md Outdated Show resolved Hide resolved
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.

Hi, @addityasingh! Welcome and thanks for the pull request! I feel like the way this changes the sentence to be a run-on sentence with confusing capitalization and seemingly out-of-place abbreviation usage does more harm to the text than benefit. It makes it harder to scan/read/understand. I think a better approach might be to update the example to specify the full path to the filename of the test file. That will make it more obvious what the sample code is doing without having to explain it in the text like this.

@Trott
Copy link
Member

Trott commented May 5, 2019

Hi, @addityasingh! Welcome and thanks for the pull request! I feel like the way this changes the sentence to be a run-on sentence with confusing capitalization and seemingly out-of-place abbreviation usage does more harm to the text than benefit. It makes it harder to scan/read/understand. I think a better approach might be to update the example to specify the full path to the filename of the test file. That will make it more obvious what the sample code is doing without having to explain it in the text like this.

So, for example, maybe update the sample code to be this?

$ python tools/test.py -J --mode=release test/parallel/test-stream2-transform.js

Furthermore (and optionally--these things below are not required in this PR, but I'd be happy to see them myself), this can be simplified. Since this is an example of just running a single test, the -J option seems unnecessary:

$ python tools/test.py --mode=release test/parallel/test-stream2-transform.js

And honestly, I'm not sure --mode=release is useful here. I never use it myself, and the point of this is to show how to simply run a single test, so in the interest of keeping it simple:

$ python tools/test.py test/parallel/test-stream2-transform.js

Lastly, tools/test.py does not need to be called explicitly with python assuming the execution bit is set (which it is in the repo, at least on UNIX-like operating systems), so:

$ tools/test.py test/parallel/test-stream2-transform.js

@addityasingh
Copy link
Contributor Author

addityasingh commented May 5, 2019

@Trott Thanks for the suggestions.

Since this is an example of just running a single test, the -J option seems unnecessary:

Agreed. I will remove the -J. I didn't update since it already existed.

And honestly, I'm not sure --mode=release is useful here

I will remove it from this specific command

Lastly, tools/test.py does not need to be called explicitly with python assuming the execution bit is set

I disagree. For the sake of inter-operability and explicitness, we should still keep the python

On top of those, I would update the sentence to specify that the command is to execute all tests in a file and not just one test. Currently, its unclear if the command is intended to execute all the tests in same file or just a single test.

@addityasingh addityasingh force-pushed the as-update-test-example branch from de17ba0 to c54b637 Compare May 5, 2019 17:46
addityasingh and others added 4 commits May 5, 2019 20:13
Update the documentation for test execution for tests in a single file
Co-Authored-By: addityasingh <[email protected]>
@addityasingh addityasingh force-pushed the as-update-test-example branch from c54b637 to 0b105ac Compare May 5, 2019 18:14
BUILDING.md Outdated Show resolved Hide resolved
BUILDING.md Outdated Show resolved Hide resolved
@Trott
Copy link
Member

Trott commented May 5, 2019

Left two optional suggestions, but this looks good to me with or without those suggestions.

@addityasingh
Copy link
Contributor Author

@Trott Can you check now? I applied your suggestions

@Trott

This comment has been minimized.

@Trott Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 6, 2019
@addityasingh

This comment has been minimized.

@Trott
Copy link
Member

Trott commented May 6, 2019

@danbev
Copy link
Contributor

danbev commented May 7, 2019

Landed in fa3eefc 🎉

@danbev danbev closed this May 7, 2019
danbev pushed a commit that referenced this pull request May 7, 2019
Update the documentation for test execution for tests in a single file

PR-URL: #27561
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
@addityasingh addityasingh deleted the as-update-test-example branch May 7, 2019 07:58
targos pushed a commit that referenced this pull request May 9, 2019
Update the documentation for test execution for tests in a single file

PR-URL: #27561
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
@BridgeAR BridgeAR mentioned this pull request May 21, 2019
4 tasks
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. build Issues and PRs related to build files or the CI. doc Issues and PRs related to the documentations.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants