Join GitHub today
GitHub is home to over 40 million developers working together to host and review code, manage projects, and build software together.
Sign upAdding where does test go section in writing-test.md #18802
Conversation
nodejs-github-bot
added
the
doc
label
Feb 15, 2018
vsemozhetbyt
added
the
test
label
Feb 15, 2018
|
Thank you, @juggernaut451! |
| @@ -18,6 +18,13 @@ Add tests when: | |||
| - Fixing regressions and bugs. | |||
| - Expanding test coverage. | |||
|
|
|||
| ## Where does the test go | |||
| One can refer [test directory structure](https://github.com/nodejs/node/tree/master/test) | |||
| to decide where to put your test cases. If you want to find any current test that exist then | |||
This comment has been minimized.
This comment has been minimized.
| One can refer [test directory structure](https://github.com/nodejs/node/tree/master/test) | ||
| to decide where to put your test cases. If you want to find any current test that exist then | ||
| go to the defined [test directory structure](https://github.com/nodejs/node/tree/master/test) | ||
| and search for similar APIs and see if there is similar file available |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
vsemozhetbyt
Feb 15, 2018
Contributor
Sorry. I think file available should be file available. (with a dot).
This comment has been minimized.
This comment has been minimized.
|
Thanks for taking this on @juggernaut451 — some small feedback below. |
| @@ -18,6 +18,13 @@ Add tests when: | |||
| - Fixing regressions and bugs. | |||
| - Expanding test coverage. | |||
|
|
|||
| ## Where does the test go | |||
| One can refer [test directory structure](https://github.com/nodejs/node/tree/master/test) | |||
This comment has been minimized.
This comment has been minimized.
apapirovski
Feb 15, 2018
Member
General nit: please keep lines to 80 chars. I think URLs can, maybe, be excepted.
This comment has been minimized.
This comment has been minimized.
| @@ -18,6 +18,13 @@ Add tests when: | |||
| - Fixing regressions and bugs. | |||
| - Expanding test coverage. | |||
|
|
|||
| ## Where does the test go | |||
This comment has been minimized.
This comment has been minimized.
apapirovski
Feb 15, 2018
•
Member
This would sound better as "Where are tests located" (maybe "Where are the..." but that's getting clunky) or even "Test directory structure".
This comment has been minimized.
This comment has been minimized.
| One can refer [test directory structure](https://github.com/nodejs/node/tree/master/test) | ||
| to decide where to put your test cases. If you want to find any current test that exists then | ||
| go to the defined [test directory structure](https://github.com/nodejs/node/tree/master/test) | ||
| and search for similar APIs and see if there is similar file available. |
This comment has been minimized.
This comment has been minimized.
apapirovski
Feb 15, 2018
Member
Overall, this is a bit too verbose and repeats the same information twice. We also do our best to avoid using "you", "your", "one" and similar. We could communicate the same thought in a shorter sentence:
See the test directory structure overview for an outline of existing test types and their locations.
That could probably be improved further. (Note that the link above goes directly to the README rather than the test folder.)
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
apapirovski
Feb 15, 2018
•
Member
It probably doesn't but I don't think the current one does either. I would go with something like:
When deciding on whether to expand an existing test file or create a new one, consider going through the files related to the subsystem (e.g. ones starting with "test-streams" for
lib/streams.js) and assessing whether a related test already exists.
It's possible that should be even more detailed, like for example explaining the difference between parallel and sequential, but that could get quite tricky and complicated.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| See the test [directory structure overview](https://github.com/nodejs/node/blob/master/test/README.md) | ||
| for an outline of existing test types and their locations. | ||
| When deciding on whether to expand an existing test file or create a new one, | ||
| consider going through the files related to the subsystem (e.g. ones starting with "test-streams" for `lib/streams.js`) |
This comment has been minimized.
This comment has been minimized.
joyeecheung
Feb 16, 2018
Member
Nit: can you wrap this to 80-chars? Also maybe ones starting with `test-streams-` for `lib/streams.js`
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
juggernaut451
force-pushed the
juggernaut451:refactoringAddingTest
branch
from
7899496
to
0493c0e
Feb 17, 2018
This comment has been minimized.
This comment has been minimized.
juggernaut451
force-pushed the
juggernaut451:refactoringAddingTest
branch
from
0493c0e
to
ca0448b
Feb 17, 2018
This comment has been minimized.
This comment has been minimized.
|
Just a few nits, LG otherwise. |
| For example, take a look at tests starting with "test-streams" | ||
| when writing tests for `lib/streams.js` | ||
|
|
||
|
|
This comment has been minimized.
This comment has been minimized.
BridgeAR
Feb 18, 2018
Member
Please keep this consistent with the other places in the doc and only use a single new line :-)
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| @@ -18,6 +18,15 @@ Add tests when: | |||
| - Fixing regressions and bugs. | |||
| - Expanding test coverage. | |||
|
|
|||
| ## Test directory structure | |||
| See the test [directory structure overview](https://github.com/nodejs/node/blob/master/test/README.md) | |||
This comment has been minimized.
This comment has been minimized.
BridgeAR
Feb 18, 2018
Member
Since this is longer than 80 characters, please move the reference down to the end of the file and only keep [directory structure overview][].
This comment has been minimized.
This comment has been minimized.
| When deciding on whether to expand an existing test file or create a new one, | ||
| consider going through the files related to the subsystem. | ||
| For example, take a look at tests starting with "test-streams" | ||
| when writing tests for `lib/streams.js` |
This comment has been minimized.
This comment has been minimized.
BridgeAR
Feb 18, 2018
Member
The line breaks in this paragraph are actually not visible when viewing this not in raw form. Therefore it would be good to wrap each line around 80 characters.
This comment has been minimized.
This comment has been minimized.
| @@ -18,6 +18,15 @@ Add tests when: | |||
| - Fixing regressions and bugs. | |||
| - Expanding test coverage. | |||
|
|
|||
| ## Test directory structure | |||
| See the test [directory structure overview](https://github.com/nodejs/node/blob/master/test/README.md) | |||
This comment has been minimized.
This comment has been minimized.
BridgeAR
Feb 18, 2018
Member
Adding a new line after the header would be good for consistency as well.
This comment has been minimized.
This comment has been minimized.
|
Can you please make sure the lines do not exceed 80 characters? :-) |
| ## Test directory structure | ||
|
|
||
| See the test [directory structure overview][] for an outline of existing test types and their locations. | ||
| When deciding on whether to expand an existing test file or create a new one. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| See the test [directory structure overview][] for an outline of existing test types and their locations. | ||
| When deciding on whether to expand an existing test file or create a new one. | ||
| Consider going through the files related to the subsystem. | ||
| For example looking at tests starting with `test-streams` when writing tests for `lib/streams.js` |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| ## Test directory structure | ||
|
|
||
| See [directory structure overview][] for outline of existing test & locations, | ||
| When deciding on whether to expand an existing test file or create a new one. |
This comment has been minimized.
This comment has been minimized.
joyeecheung
Feb 22, 2018
Member
Sorry for not being clear, I meant the punctuation at the end of this line should be ,, so it should be
See [directory structure overview][] for outline of existing test & locations.
When deciding on whether to expand an existing test file or create a new one,
consider going through the files related to the subsystem.
This comment has been minimized.
This comment has been minimized.
juggernaut451
Feb 22, 2018
Author
Contributor
When deciding on whether to expand an existing test file or create a new one, consider going through the files related to the subsystem.
@joyeecheung it will exceed 80 characters.
This comment has been minimized.
This comment has been minimized.
BridgeAR
Feb 22, 2018
Member
I think @joyeecheung meant it to be a text block (without exceeding 80 characters by using new lines in the correct places), not as being in a single line.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
@BridgeAR @joyeecheung can you please review it now |
| See [directory structure overview][] for outline of existing test & locations. | ||
| When deciding on whether to expand an existing test file or create a new one, | ||
| consider going through the files related to the subsystem. | ||
| For example viewing tests for `test-streams` when writing for `lib/streams.js`. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| See [directory structure overview][] for outline of existing test & locations. | ||
| When deciding on whether to expand an existing test file or create a new one, | ||
| consider going through the files related to the subsystem. | ||
| For example viewing tests for `test-streams` when writing a test for `lib/streams.js`. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
@mmarchini I can't find any issue with the above CI. |
This comment has been minimized.
This comment has been minimized.
Yep, CI was green. |
|
LGTM with suggestion |
| @@ -381,3 +388,4 @@ will depend on what is being tested if this is required or not. | |||
| [all maintained branches]: https://github.com/nodejs/lts | |||
| [node.green]: http://node.green/ | |||
| [test fixture]: https://github.com/google/googletest/blob/master/googletest/docs/Primer.md#test-fixtures-using-the-same-data-configuration-for-multiple-tests | |||
| [directory structure overview]: https://github.com/nodejs/node/blob/master/test/README.md | |||
This comment has been minimized.
This comment has been minimized.
gibfahn
Mar 5, 2018
Member
Might be a good idea to change this to:
https://github.com/nodejs/node/blob/master/test/README.md#test-directories
so it links directly to the correct section.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
juggernaut451
force-pushed the
juggernaut451:refactoringAddingTest
branch
from
cfe1481
to
fc0c5b4
Mar 9, 2018
This comment has been minimized.
This comment has been minimized.
|
Resolved :) |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Landed in 59b5d77 |
juggernaut451 commentedFeb 15, 2018
•
edited
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
doc
Fixes: #18774