doc: document directories in test directory by mike182uk · Pull Request #5557 · nodejs/node

mike182uk · 2016-03-03T23:20:54Z

Pull Request check-list

Please make sure to review and check all of these items:

Does make -j8 test (UNIX) or vcbuild test nosign (Windows) pass with
this change (including linting)?
Is the commit message formatted according to CONTRIBUTING.md?
If this change fixes a bug (or a performance problem), is a regression
test (or a benchmark) included?
Is a documentation update included (if this change modifies
existing APIs, or introduces new ones)?

NOTE: these things are not required to open a PR and can be done
afterwards / while the PR is open.

Affected core subsystem(s)

tests

Description of change

Add documentation for test directory as per #5538

mike182uk · 2016-03-03T23:25:36Z

There are 2 directories that i am struggling to document: cctest and testpy. If some one could drop a sentence or two on this thread for them i'll update the PR.

Trott · 2016-03-03T23:51:02Z

Nit: are run in parallel -> may be run in parallel.

able to be run in parallel ;)

👍 Updated

Fishrock123 · 2016-03-04T00:14:34Z

This should be true I think.

Yeah, it's run as part of make. If git blame isn't misleading me, @bnoordhuis wrote the one file in that directory and may be able to supply a half-sentence summary of what it does if no one else steps up. If all else fails, you can just say something hand-waving like "C++ test that is run as part of the build process".

Trott · 2016-03-04T00:18:01Z

testpy is not a directory of tests like the others. It is used by the tools/test.py test wrapper (which is used, for example, if you run make test).

Trott · 2016-03-04T00:40:27Z

Overall, looks like a fine start to me. Thanks for doing this. Hopefully we can fill in the one or two gaps and land this.

mike182uk · 2016-03-04T09:44:44Z

@Trott Thanks for the feedback, i've updated the PR to include descriptions for cctest and testpy. I've used your description above for cctest for now, but if someone comments with a more descriptive description i'll update the PR.

evanlucas · 2016-03-04T11:54:54Z

Maybe give more details about how these actually make outbound connections. There are tests for all of those modules in both /parallel and /sequential. None of those actually go outside the local network though.

nit: long line. Please keep them under 80 columns

evanlucas · 2016-03-04T11:59:00Z

LGTM with the few nits fixed. Thanks!

Florian-R · 2016-03-04T12:01:30Z

Maybe an explanation of why those tests are not run in CI could be useful?

I agree that would be useful.

Trott · 2016-03-04T17:42:32Z

#5528 added a known_issues directory so maybe add that if you're already tidying up a few other things?

mike182uk · 2016-03-04T21:47:21Z

Fixed the long line issues, updated the description for the internet directory, and added the known_issues directory.

Trott · 2016-03-05T05:57:10Z

LGTM. (There are gaps that experienced people might be able to fill in. But as this is, it is much better than what we have now, which is nothing.)

silverwind · 2016-03-05T06:09:33Z

Please add a space after the hashes. Various markdown parsers fail to render such headings otherwise.

👍 fixed.

evanlucas · 2016-03-05T13:34:01Z

LGTM

Trott · 2016-03-05T20:37:51Z

I know we usually don't run CI on doc-only changes, but I have this nagging sense that there's a non-zero possibility that an extra file in test can have side effects. I'm almost certainly wrong, but just in case:

https://ci.nodejs.org/job/node-test-pull-request/1869/

jasnell · 2016-03-07T18:29:50Z

LGTM

PR-URL: nodejs#5557 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

Trott · 2016-03-07T21:11:48Z

Landed in 96af474

PR-URL: #5557 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

mike182uk mentioned this pull request Mar 3, 2016
Test Directory Documentation #5538
Closed

Trott reviewed Mar 3, 2016
View reviewed changes

mscdex added doc Issues and PRs related to the documentations. test Issues and PRs related to the tests. labels Mar 4, 2016

Fishrock123 reviewed Mar 4, 2016
View reviewed changes

evanlucas reviewed Mar 4, 2016
View reviewed changes

Florian-R reviewed Mar 4, 2016
View reviewed changes

silverwind reviewed Mar 5, 2016
View reviewed changes

doc: document directories in test directory
d1a244e

jasnell added the lts-watch-v4.x label Mar 7, 2016

Trott pushed a commit to Trott/io.js that referenced this pull request Mar 7, 2016
doc: document directories in test directory
96af474
PR-URL: nodejs#5557 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

Trott closed this Mar 7, 2016

Fishrock123 mentioned this pull request Mar 7, 2016
v5.8.0 proposal #5559
Merged

Fishrock123 pushed a commit that referenced this pull request Mar 8, 2016
doc: document directories in test directory
c17a71c
PR-URL: #5557 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

Fishrock123 pushed a commit that referenced this pull request Mar 8, 2016
doc: document directories in test directory
3c92352
PR-URL: #5557 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

MylesBorins pushed a commit that referenced this pull request Mar 17, 2016
doc: document directories in test directory
09c7e36
PR-URL: #5557 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

MylesBorins added land-on-v4.x and removed lts-watch-v4.x labels Mar 17, 2016

MylesBorins pushed a commit that referenced this pull request Mar 21, 2016
doc: document directories in test directory
81d00f3
PR-URL: #5557 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

MylesBorins pushed a commit that referenced this pull request Mar 21, 2016
doc: document directories in test directory
98a1bb6
PR-URL: #5557 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

Uh oh!

Uh oh!

Conversation

mike182uk commented Mar 3, 2016

Pull Request check-list

Affected core subsystem(s)

Description of change

Uh oh!

mike182uk commented Mar 3, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Trott commented Mar 4, 2016

Uh oh!

Trott commented Mar 4, 2016

Uh oh!

mike182uk commented Mar 4, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

evanlucas commented Mar 4, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Trott commented Mar 4, 2016

Uh oh!

mike182uk commented Mar 4, 2016

Uh oh!

Trott commented Mar 5, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

evanlucas commented Mar 5, 2016

Uh oh!

Trott commented Mar 5, 2016

Uh oh!

jasnell commented Mar 7, 2016

Uh oh!

Trott commented Mar 7, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants