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: improve testing guide #11150

Closed
wants to merge 6 commits into
from

Conversation

Projects
None yet
9 participants
@joyeecheung
Member

joyeecheung commented Feb 3, 2017

Add guide on choice of assertions, use of ES.Next features, and the WPT upstream.

Refs: #11142

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

joyeecheung added some commits Feb 3, 2017

doc: update test writing guide
Add guide about choice of assertions and encourage using
ES.Next features.

@joyeecheung joyeecheung requested a review from Trott Feb 3, 2017

@mscdex mscdex added the test label Feb 3, 2017

@Trott

Trott approved these changes Feb 3, 2017

LGTM

I left a bunch of style nits, but they are all optional. I'm fine with this as is, and I'm fine with it with any or all of the changes I suggested.

Show outdated Hide outdated doc/guides/writing-tests.md
### Assertions
When writing assertions for object comparison, prefer the strict versions,
namely:

This comment has been minimized.

@Trott

Trott Feb 3, 2017

Member

Nit: Get rid of namely.

...prefer the strict versions:

  • assert.strictEqual()...
@Trott

Trott Feb 3, 2017

Member

Nit: Get rid of namely.

...prefer the strict versions:

  • assert.strictEqual()...
Show outdated Hide outdated doc/guides/writing-tests.md
@@ -209,6 +209,36 @@ const assert = require('assert');
const freelist = require('internal/freelist');
```
### Assertions
When writing assertions for object comparison, prefer the strict versions,

This comment has been minimized.

@Trott

Trott Feb 3, 2017

Member

Nit: Remove object.

When writing assertions for comparisons, prefer the strict versions:

@Trott

Trott Feb 3, 2017

Member

Nit: Remove object.

When writing assertions for comparisons, prefer the strict versions:

Show outdated Hide outdated doc/guides/writing-tests.md
### ES.Next features
At the moment we only use a selected subset of ES.Next features in
JavaScript code under the `lib` directory for performance considerations,

This comment has been minimized.

@Trott

Trott Feb 3, 2017

Member

Nit: under -> in

@Trott

Trott Feb 3, 2017

Member

Nit: under -> in

Show outdated Hide outdated doc/guides/writing-tests.md
### ES.Next features
At the moment we only use a selected subset of ES.Next features in
JavaScript code under the `lib` directory for performance considerations,

This comment has been minimized.

@Trott

Trott Feb 3, 2017

Member

Nit: Remove At the moment and move for performance considerations to the beginning:

For performance considerations, we only use a selected subset...

@Trott

Trott Feb 3, 2017

Member

Nit: Remove At the moment and move for performance considerations to the beginning:

For performance considerations, we only use a selected subset...

Show outdated Hide outdated doc/guides/writing-tests.md
* `let` and `const` over `var`
* Template literals over string concatenation
* Arrow functions over normal functions, if appropriate

This comment has been minimized.

@Trott

Trott Feb 3, 2017

Member

Nit:

  • Arrow functions when appropriate
@Trott

Trott Feb 3, 2017

Member

Nit:

  • Arrow functions when appropriate
Show outdated Hide outdated doc/guides/writing-tests.md
At the moment we only use a selected subset of ES.Next features in
JavaScript code under the `lib` directory for performance considerations,
but when writing tests, it is encouraged to use ES.Next features that have

This comment has been minimized.

@Trott

Trott Feb 3, 2017

Member

Nit: Make this the start of its own sentence and change but to However.

However, when writing tests, it is...

@Trott

Trott Feb 3, 2017

Member

Nit: Make this the start of its own sentence and change but to However.

However, when writing tests, it is...

Show outdated Hide outdated doc/guides/writing-tests.md
Some of the tests for the WHATWG URL implementation (named
`test-whatwg-url-*.js`) are imported from the
[Web Platform Tests Project](https://github.com/w3c/web-platform-tests/tree/master/url).
If you see a test wrapped in code like this

This comment has been minimized.

@Trott

Trott Feb 3, 2017

Member

Nit: Replace this line with:

These imported tests will be wrapped like this:

@Trott

Trott Feb 3, 2017

Member

Nit: Replace this line with:

These imported tests will be wrapped like this:

Show outdated Hide outdated doc/guides/writing-tests.md
/* eslint-enable */
```
and want to improve it, please send a PR to the upstream project

This comment has been minimized.

@Trott

Trott Feb 3, 2017

Member

Nit: Have this be the start of its own sentence.

If you want to improve tests that have been imported this way, please send a...

@Trott

Trott Feb 3, 2017

Member

Nit: Have this be the start of its own sentence.

If you want to improve tests that have been imported this way, please send a...

Show outdated Hide outdated doc/guides/writing-tests.md
```
and want to improve it, please send a PR to the upstream project
first, and when it's merged, send another PR to this project

This comment has been minimized.

@Trott

Trott Feb 3, 2017

Member

Nit: End the sentence after first and begin a new sentence after that:

first. When your proposed change is merged in the upstream project, send another PR to...

@Trott

Trott Feb 3, 2017

Member

Nit: End the sentence after first and begin a new sentence after that:

first. When your proposed change is merged in the upstream project, send another PR to...

Show outdated Hide outdated doc/guides/writing-tests.md
and want to improve it, please send a PR to the upstream project
first, and when it's merged, send another PR to this project
to update the code accordingly (be sure to update the hash in the

This comment has been minimized.

@Trott

Trott Feb 3, 2017

Member

Nit:

to update the code accordingly. Be sure to update the hash in the...

(Also, if you do that, remove the closing parenthesis on the next line.)

@Trott

Trott Feb 3, 2017

Member

Nit:

to update the code accordingly. Be sure to update the hash in the...

(Also, if you do that, remove the closing parenthesis on the next line.)

@Trott

This comment has been minimized.

Show comment
Hide comment
@joyeecheung

This comment has been minimized.

Show comment
Hide comment
@joyeecheung

joyeecheung Feb 4, 2017

Member

@Trott Thanks for the review, PTAL.

Member

joyeecheung commented Feb 4, 2017

@Trott Thanks for the review, PTAL.

@jasnell

jasnell approved these changes Feb 4, 2017

Show outdated Hide outdated doc/guides/writing-tests.md
@@ -209,6 +209,35 @@ const assert = require('assert');
const freelist = require('internal/freelist');
```
### Assertions
When writing assertions for comparison, prefer the strict versions:

This comment has been minimized.

@Trott

Trott Feb 4, 2017

Member

Nit: Probably clearer to have singular/plural agreement: writing an assertion for comparison or writing assertions for comparisons.

@Trott

Trott Feb 4, 2017

Member

Nit: Probably clearer to have singular/plural agreement: writing an assertion for comparison or writing assertions for comparisons.

Show outdated Hide outdated doc/guides/writing-tests.md
If you want to improve tests that have been imported this way, please send
a PR to the upstream project first. When your proposed change is merged in
the upstream project, send another PR to update the code accordingly.

This comment has been minimized.

@Trott

Trott Feb 4, 2017

Member

Nit: another PR to update Node.js accordingly or something else that indicates that the second PR should be against nodejs/node and not against the upstream project.

@Trott

Trott Feb 4, 2017

Member

Nit: another PR to update Node.js accordingly or something else that indicates that the second PR should be against nodejs/node and not against the upstream project.

Show outdated Hide outdated doc/guides/writing-tests.md
If you want to improve tests that have been imported this way, please send
a PR to the upstream project first. When your proposed change is merged in
the upstream project, send another PR to update the code accordingly.
Be sure to update the hash in the hash in the URL following `WPT Refs:`.

This comment has been minimized.

@Trott

Trott Feb 4, 2017

Member

Looks like there's one too many hash in the.

@Trott

Trott Feb 4, 2017

Member

Looks like there's one too many hash in the.

@joyeecheung

This comment has been minimized.

Show comment
Hide comment
@joyeecheung

joyeecheung Feb 6, 2017

Member

@Trott Updated, PTAL.

Member

joyeecheung commented Feb 6, 2017

@Trott Updated, PTAL.

Show outdated Hide outdated doc/guides/writing-tests.md
@@ -209,6 +209,35 @@ const assert = require('assert');
const freelist = require('internal/freelist');
```
### Assertions
When writing an assertions for comparison, prefer the strict versions:

This comment has been minimized.

@thefourtheye

thefourtheye Feb 6, 2017

Contributor

an assertions should be an assertion or simply assertions.

@thefourtheye

thefourtheye Feb 6, 2017

Contributor

an assertions should be an assertion or simply assertions.

This comment has been minimized.

@joyeecheung

joyeecheung Feb 6, 2017

Member

Oops, thanks for catching that.

@joyeecheung

joyeecheung Feb 6, 2017

Member

Oops, thanks for catching that.

Show outdated Hide outdated doc/guides/writing-tests.md
@@ -209,6 +209,35 @@ const assert = require('assert');
const freelist = require('internal/freelist');
```
### Assertions
When writing an assertion for comparison, prefer the strict versions:

This comment has been minimized.

@thefourtheye

thefourtheye Feb 6, 2017

Contributor

Strictly speaking, assertion is not for comparison.

@thefourtheye

thefourtheye Feb 6, 2017

Contributor

Strictly speaking, assertion is not for comparison.

This comment has been minimized.

@joyeecheung

joyeecheung Feb 6, 2017

Member

Hmmm, yes, the word "for" does sound wrong here, or the order of assertion and comparison here is wrong. Probably assertion with comparison?

@joyeecheung

joyeecheung Feb 6, 2017

Member

Hmmm, yes, the word "for" does sound wrong here, or the order of assertion and comparison here is wrong. Probably assertion with comparison?

This comment has been minimized.

@thefourtheye

thefourtheye Feb 6, 2017

Contributor

You can simply drop for comparison.. "When asserting, prefer strict versions" - How does it sound?

@thefourtheye

thefourtheye Feb 6, 2017

Contributor

You can simply drop for comparison.. "When asserting, prefer strict versions" - How does it sound?

This comment has been minimized.

@joyeecheung

joyeecheung Feb 6, 2017

Member

Yeah I think that works too.

@joyeecheung

joyeecheung Feb 6, 2017

Member

Yeah I think that works too.

* `assert.strictEqual()` over `assert.equal()`
* `assert.deepStrictEqual()` over `assert.deepEqual()`
When using `assert.throws()`, if possible, provide the full error message:

This comment has been minimized.

@thefourtheye

thefourtheye Feb 6, 2017

Contributor

We could encourage people to always use the full message, so that we can avoid incidents like #11162

@thefourtheye

thefourtheye Feb 6, 2017

Contributor

We could encourage people to always use the full message, so that we can avoid incidents like #11162

This comment has been minimized.

@joyeecheung

joyeecheung Feb 6, 2017

Member

So this would better be "provide the full error message unless it is impossible"? (I am trying to think of a scenario where it is impossible, which I believe I've seen before, but nothing come out of my head at the moment)

@joyeecheung

joyeecheung Feb 6, 2017

Member

So this would better be "provide the full error message unless it is impossible"? (I am trying to think of a scenario where it is impossible, which I believe I've seen before, but nothing come out of my head at the moment)

This comment has been minimized.

@thefourtheye

thefourtheye Feb 6, 2017

Contributor

@Trott Any thoughts, how to handle this?

@thefourtheye

thefourtheye Feb 6, 2017

Contributor

@Trott Any thoughts, how to handle this?

This comment has been minimized.

@Trott

Trott Feb 7, 2017

Member

I'm OK with the text the way it is. We can always improve it in a subsequent pull request if someone comes up with better wording. I think "if possible, provide the full error message" is about as good as anything I can come up with right now....

@Trott

Trott Feb 7, 2017

Member

I'm OK with the text the way it is. We can always improve it in a subsequent pull request if someone comes up with better wording. I think "if possible, provide the full error message" is about as good as anything I can come up with right now....

This comment has been minimized.

@Trott

Trott Feb 7, 2017

Member

If you want to drop "if possible" and just say "provide the full error message", that works for me too. I have no strong opinion either way. Adding either one would be an improvement to the doc, so 👍 from me whichever way you go on it.

@Trott

Trott Feb 7, 2017

Member

If you want to drop "if possible" and just say "provide the full error message", that works for me too. I have no strong opinion either way. Adding either one would be an improvement to the doc, so 👍 from me whichever way you go on it.

This comment has been minimized.

@edsadr

edsadr Feb 7, 2017

Member

well... I wouldn't say impossible, but still requires a lot of work, right now I am having a hard time with this problem here: #11096 , error messages change across platforms, so I would say make the suggestion but put a note saying that this could happen

@edsadr

edsadr Feb 7, 2017

Member

well... I wouldn't say impossible, but still requires a lot of work, right now I am having a hard time with this problem here: #11096 , error messages change across platforms, so I would say make the suggestion but put a note saying that this could happen

This comment has been minimized.

@joyeecheung

joyeecheung Feb 7, 2017

Member

Or should I just say: the regular expression used to validate error messages should be as concrete as possible?

@joyeecheung

joyeecheung Feb 7, 2017

Member

Or should I just say: the regular expression used to validate error messages should be as concrete as possible?

This comment has been minimized.

@joyeecheung

joyeecheung Feb 7, 2017

Member

Or

When using `assert.throws()`, prefer using regular expressions to validate error messages,
and the regular expressions provided should be as concrete as possible:
@joyeecheung

joyeecheung Feb 7, 2017

Member

Or

When using `assert.throws()`, prefer using regular expressions to validate error messages,
and the regular expressions provided should be as concrete as possible:

This comment has been minimized.

@Trott

Trott Feb 7, 2017

Member

It may just be terminology I'm unfamiliar with, but without the context of this conversation, I wouldn't know what this would mean. If someone told me, "That's pretty good, but could you make the regular expression more concrete?" ... I would have to ask what they meant.

I think the existing wording in this PR is the best we've managed to come up with yet. I'm OK with leaving it as is and saving any bike-shedding for subsequent revisions, but if we want to tweak it now maybe this?:

When using assert.throws(), the second argument should be either a regular expression or a function that validates the received error. If using a regular expression, make the match as specific as possible. Match the entire error string by starting the regular expression with ^ and ending with $.

@Trott

Trott Feb 7, 2017

Member

It may just be terminology I'm unfamiliar with, but without the context of this conversation, I wouldn't know what this would mean. If someone told me, "That's pretty good, but could you make the regular expression more concrete?" ... I would have to ask what they meant.

I think the existing wording in this PR is the best we've managed to come up with yet. I'm OK with leaving it as is and saving any bike-shedding for subsequent revisions, but if we want to tweak it now maybe this?:

When using assert.throws(), the second argument should be either a regular expression or a function that validates the received error. If using a regular expression, make the match as specific as possible. Match the entire error string by starting the regular expression with ^ and ending with $.

This comment has been minimized.

@joyeecheung

joyeecheung Feb 7, 2017

Member

It's a little bit more wordy, but this should be less confusing to new contributors so I think it's worth it.

I'll go ahead and land this PR first, and make the improvement a good first contributon issue linking to this discussion, so we can get a fresh angle from a new contributor.

@joyeecheung

joyeecheung Feb 7, 2017

Member

It's a little bit more wordy, but this should be less confusing to new contributors so I think it's worth it.

I'll go ahead and land this PR first, and make the improvement a good first contributon issue linking to this discussion, so we can get a fresh angle from a new contributor.

@joyeecheung joyeecheung referenced this pull request Feb 6, 2017

Closed

test: add regex #11193

3 of 3 tasks complete
@joyeecheung

This comment has been minimized.

Show comment
Hide comment
@joyeecheung

joyeecheung Feb 7, 2017

Member

Linter is green, landed in 7ba847d

Member

joyeecheung commented Feb 7, 2017

Linter is green, landed in 7ba847d

@joyeecheung joyeecheung closed this Feb 7, 2017

italoacasas added a commit to italoacasas/node that referenced this pull request Feb 9, 2017

doc: improve testing guide
Add guide on choice of assertions, use of ES.Next features,
and the WPT upstream.

PR-URL: nodejs#11150
Ref: nodejs#11142
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>

italoacasas added a commit to italoacasas/node that referenced this pull request Feb 14, 2017

doc: improve testing guide
Add guide on choice of assertions, use of ES.Next features,
and the WPT upstream.

PR-URL: nodejs#11150
Ref: nodejs#11142
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>

@joyeecheung joyeecheung deleted the joyeecheung:testing-guide branch Feb 19, 2017

KryDos added a commit to KryDos/node that referenced this pull request Feb 25, 2017

doc: improve testing guide
Add guide on choice of assertions, use of ES.Next features,
and the WPT upstream.

PR-URL: nodejs#11150
Ref: nodejs#11142
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>

jasnell added a commit that referenced this pull request Mar 7, 2017

doc: improve testing guide
Add guide on choice of assertions, use of ES.Next features,
and the WPT upstream.

PR-URL: #11150
Ref: #11142
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>

jasnell added a commit that referenced this pull request Mar 7, 2017

doc: improve testing guide
Add guide on choice of assertions, use of ES.Next features,
and the WPT upstream.

PR-URL: #11150
Ref: #11142
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>

MylesBorins added a commit that referenced this pull request Mar 9, 2017

doc: improve testing guide
Add guide on choice of assertions, use of ES.Next features,
and the WPT upstream.

PR-URL: #11150
Ref: #11142
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>

@MylesBorins MylesBorins referenced this pull request Mar 9, 2017

Merged

v6.10.1 proposal #11759

MylesBorins added a commit that referenced this pull request Mar 9, 2017

doc: improve testing guide
Add guide on choice of assertions, use of ES.Next features,
and the WPT upstream.

PR-URL: #11150
Ref: #11142
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>

@MylesBorins MylesBorins referenced this pull request Mar 9, 2017

Merged

v4.8.1 proposal #11760

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment