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

Adding where does test go section in writing-test.md #18802

Closed

Conversation

@juggernaut451
Copy link
Contributor

commented Feb 15, 2018

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

doc

Fixes: #18774

@vsemozhetbyt
Copy link
Contributor

left a comment

Thank you, @juggernaut451!
I've made some tiny notes, but they can be addressed by a comment lander if it is more easy for you.

@@ -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.

Copy link
@vsemozhetbyt

vsemozhetbyt Feb 15, 2018

Contributor

exist -> exists

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.

Copy link
@vsemozhetbyt

vsemozhetbyt Feb 15, 2018

Contributor

A nit: missing period.

This comment has been minimized.

Copy link
@juggernaut451

juggernaut451 Feb 15, 2018

Author Contributor

did not get this one. Can you please elaborate

This comment has been minimized.

Copy link
@vsemozhetbyt

vsemozhetbyt Feb 15, 2018

Contributor

Sorry. I think file available should be file available. (with a dot).

This comment has been minimized.

Copy link
@juggernaut451

juggernaut451 Feb 15, 2018

Author Contributor

resolved :)

@apapirovski
Copy link
Member

left a comment

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.

Copy link
@apapirovski

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.

Copy link
@juggernaut451

juggernaut451 Feb 15, 2018

Author Contributor

thanks 👍

@@ -18,6 +18,13 @@ Add tests when:
- Fixing regressions and bugs.
- Expanding test coverage.

## Where does the test go

This comment has been minimized.

Copy link
@apapirovski

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.

Copy link
@juggernaut451

juggernaut451 Feb 15, 2018

Author Contributor

changing it to "Test directory structure"

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.

Copy link
@apapirovski

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.

Copy link
@juggernaut451

juggernaut451 Feb 15, 2018

Author Contributor

Does above description answers #18774 second issue?

This comment has been minimized.

Copy link
@apapirovski

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.

Copy link
@juggernaut451

juggernaut451 Feb 15, 2018

Author Contributor

Thanks making changes with the above one. :)

This comment has been minimized.

Copy link
@juggernaut451

juggernaut451 Feb 15, 2018

Author Contributor

changes made.

This comment has been minimized.

Copy link
@juggernaut451

juggernaut451 Feb 15, 2018

Author Contributor

Thank you for feedback.

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.

Copy link
@joyeecheung

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.

Copy link
@juggernaut451

juggernaut451 Feb 16, 2018

Author Contributor

fixing it

This comment has been minimized.

Copy link
@juggernaut451

juggernaut451 Feb 17, 2018

Author Contributor

@joyeecheung fixed. Kindly have a look

@juggernaut451 juggernaut451 force-pushed the juggernaut451:refactoringAddingTest branch from 7899496 to 0493c0e Feb 17, 2018

@mmarchini

This comment has been minimized.

@juggernaut451 juggernaut451 force-pushed the juggernaut451:refactoringAddingTest branch from 0493c0e to ca0448b Feb 17, 2018

@BridgeAR

This comment has been minimized.

Copy link
Member

commented Feb 18, 2018

@BridgeAR
Copy link
Member

left a comment

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.

Copy link
@BridgeAR

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.

Copy link
@juggernaut451

juggernaut451 Feb 19, 2018

Author Contributor

changes made.

This comment has been minimized.

Copy link
@BridgeAR

BridgeAR Feb 21, 2018

Member

Hm, this does not seem to be addressed?

@@ -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.

Copy link
@BridgeAR

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.

Copy link
@juggernaut451

juggernaut451 Feb 19, 2018

Author Contributor

changes made.

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.

Copy link
@BridgeAR

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.

Copy link
@juggernaut451

juggernaut451 Feb 19, 2018

Author Contributor

changes made.

@@ -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.

Copy link
@BridgeAR

BridgeAR Feb 18, 2018

Member

Adding a new line after the header would be good for consistency as well.

This comment has been minimized.

Copy link
@juggernaut451

juggernaut451 Feb 19, 2018

Author Contributor

changes made.

@BridgeAR
Copy link
Member

left a comment

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.

Copy link
@joyeecheung

joyeecheung Feb 21, 2018

Member

The punctuation at the end should be ,

This comment has been minimized.

Copy link
@juggernaut451

juggernaut451 Feb 21, 2018

Author Contributor

changes made :)

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.

Copy link
@joyeecheung

joyeecheung Feb 21, 2018

Member

Missing . at the end.

This comment has been minimized.

Copy link
@juggernaut451

juggernaut451 Feb 21, 2018

Author Contributor

changes made :)

## 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.

Copy link
@joyeecheung

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.

Copy link
@juggernaut451

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.

Copy link
@BridgeAR

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.

Copy link
@juggernaut451

juggernaut451 Feb 22, 2018

Author Contributor

ok got it.

@juggernaut451

This comment has been minimized.

Copy link
Contributor Author

commented Feb 26, 2018

@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.

Copy link
@joyeecheung

joyeecheung Feb 26, 2018

Member

...when writing a test for..?

This comment has been minimized.

Copy link
@juggernaut451

juggernaut451 Mar 1, 2018

Author Contributor

changes made.

@joyeecheung

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.

Copy link
@BridgeAR

BridgeAR Mar 1, 2018

Member

This should fail in the linter as it is above 80 characters.

@mmarchini

This comment has been minimized.

Copy link
Member

commented Mar 5, 2018

@juggernaut451

This comment has been minimized.

Copy link
Contributor Author

commented Mar 5, 2018

@mmarchini I can't find any issue with the above CI.

@gibfahn

This comment has been minimized.

Copy link
Member

commented Mar 5, 2018

@mmarchini I can't find any issue with the above CI.

Yep, CI was green.

@gibfahn
gibfahn approved these changes Mar 5, 2018
Copy link
Member

left a comment

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.

Copy link
@gibfahn

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.

Copy link
@juggernaut451

juggernaut451 Mar 7, 2018

Author Contributor

changes made :)

@gibfahn

This comment has been minimized.

Copy link
Member

commented Mar 9, 2018

@juggernaut451 juggernaut451 force-pushed the juggernaut451:refactoringAddingTest branch from cfe1481 to fc0c5b4 Mar 9, 2018

@juggernaut451

This comment has been minimized.

Copy link
Contributor Author

commented Mar 14, 2018

Resolved :)

@Leko
Leko approved these changes Mar 17, 2018
@trivikr
Copy link
Member

left a comment

LGTM

@trivikr

This comment has been minimized.

Copy link
Member

commented Mar 25, 2018

@trivikr

This comment has been minimized.

Copy link
Member

commented Mar 25, 2018

Landed in 59b5d77

@trivikr trivikr closed this Mar 25, 2018

trivikr added a commit that referenced this pull request Mar 25, 2018
doc: add directory structure in writing-tests.md
PR-URL: #18802
Fixes: #18774
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matheus Marchini <matheus@sthima.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Shingo Inoue <leko.noor@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
targos added a commit that referenced this pull request Mar 27, 2018
doc: add directory structure in writing-tests.md
PR-URL: #18802
Fixes: #18774
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matheus Marchini <matheus@sthima.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Shingo Inoue <leko.noor@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
BethGriggs added a commit that referenced this pull request Dec 3, 2018
doc: add directory structure in writing-tests.md
PR-URL: #18802
Fixes: #18774
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matheus Marchini <matheus@sthima.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Shingo Inoue <leko.noor@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@BethGriggs BethGriggs referenced this pull request Dec 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.