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 How to write a Node.js test guide #6984

Merged
merged 1 commit into from
May 30, 2016

Conversation

santigimeno
Copy link
Member

Checklist
  • documentation is changed or added
  • the commit message follows commit guidelines
Affected core subsystem(s)

doc, test

Description of change

Refs: nodejs/testing#30

/cc @nodejs/testing @nodejs/documentation

@nodejs-github-bot nodejs-github-bot added the doc Issues and PRs related to the documentations. label May 26, 2016
@santigimeno santigimeno added the test Issues and PRs related to the tests. label May 26, 2016
- It exits due to an uncaught exception.
- It never exits. In this case, the test runner will terminate the test because it sets a maximum time limit.

They can be added for multiple reasons:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I’d prefer s/They/Tests/ because there’s no plural tests before this in the text.

@addaleax
Copy link
Member

There’s a “test” missing in the commit message/PR title.

Generally looking good, nice work!

@addaleax
Copy link
Member

Oh, and I think linking this from the corresponding CONTRIBUTING.md section would be really helpful.

@Fishrock123 Fishrock123 changed the title doc: add How to write a Node.js guide doc: add How to write a Node.js test guide May 26, 2016
@@ -0,0 +1,166 @@
# How to write a Node.js test
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would "How to write a test for the Node.js project" be better because I thought this is for how to write a test case in Node.js community, not the Node.js project in my first sight on this title. :-)

@santigimeno santigimeno force-pushed the test_guid branch 3 times, most recently from f5bd5fe to 8259de3 Compare May 27, 2016 16:29
@santigimeno
Copy link
Member Author

santigimeno commented May 27, 2016

PR updated with @addaleax and @yorkie suggestions. I'll rebase the PR before merging. Thanks!

@addaleax
Copy link
Member

LGTM and I still think linking this from the CONTRIBUTING.md is a good idea :)

@santigimeno
Copy link
Member Author

Oh! I forgot. Adding it...

@santigimeno santigimeno force-pushed the test_guid branch 6 times, most recently from 6b5c7a0 to 9fec0a5 Compare May 27, 2016 17:29
@santigimeno
Copy link
Member Author

Link added to CONTRIBUTING.md. @addaleax PTAL.

structured (license boilerplate, common includes, etc.).
`test/parallel/` directory. For guidance on how to write a test for the Node.js
project, see this [guide](./doc/guides/writing_tests.md). Looking at other tests
to see how they should be structured (license boilerplate, common includes,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are only very few cases of explicit license notifications in the test code base (and in general) left, so I’d drop the mention here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe dropping the whole parenthesis? The common includes info is already in the guide.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@santigimeno Oh, sorry, didn’t see this was taken directly from the existing text. But yeah, +1 to dropping the whole parenthesis here.

@addaleax
Copy link
Member

LGTM with a comment

@santigimeno
Copy link
Member Author

Updated. Thanks!


**Lines 3-4**

```
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing javascript?

@Trott
Copy link
Member

Trott commented May 27, 2016

LGTM.

There are definitely some more improvements that can happen in the CONTRIBUTING doc but it's probably best for that to happen in another PR.

@santigimeno
Copy link
Member Author

Added the missing javascript. Thanks

@santigimeno
Copy link
Member Author

There are definitely some more improvements that can happen in the CONTRIBUTING doc but it's probably best for that to happen in another PR.

The sentence about the location of the tests could be changed and then explained in some depth in the guide.

but it's probably best for that to happen in another PR.

Agreed

PR-URL: nodejs#6984
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Yorkie Liu <[email protected]>
@santigimeno
Copy link
Member Author

CI: https://ci.nodejs.org/job/node-test-pull-request/2862/. All green except unrelated failures in some ARM bots. Landing

@santigimeno santigimeno merged commit b83b363 into nodejs:master May 30, 2016
@santigimeno santigimeno deleted the test_guid branch May 30, 2016 08:27
@santigimeno
Copy link
Member Author

Landed in b83b363. Thanks!

Fishrock123 pushed a commit to Fishrock123/node that referenced this pull request May 30, 2016
PR-URL: nodejs#6984
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Yorkie Liu <[email protected]>
rvagg pushed a commit that referenced this pull request Jun 2, 2016
PR-URL: #6984
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Yorkie Liu <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jul 11, 2016
PR-URL: #6984
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Yorkie Liu <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jul 12, 2016
PR-URL: #6984
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Yorkie Liu <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jul 12, 2016
PR-URL: #6984
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Yorkie Liu <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Jul 12, 2016
MylesBorins pushed a commit that referenced this pull request Jul 14, 2016
PR-URL: #6984
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Yorkie Liu <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jul 14, 2016
PR-URL: #6984
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Yorkie Liu <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants