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.

Sign up for GitHub

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
Closed

doc: improve testing guide #11150

Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
47235f0
doc: update test writing guide
joyeecheung Feb 3, 2017
4f6f235
doc: update testing guide regarding WPT upstream
joyeecheung Feb 3, 2017
1d9c9f0
fix style
joyeecheung Feb 4, 2017
f236299
fix styles
joyeecheung Feb 6, 2017
7ce8f5d
fix typo
joyeecheung Feb 6, 2017
61fb2e8
drop 'for comparison'
joyeecheung Feb 6, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 56 additions & 0 deletions doc/guides/writing-tests.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -209,6 +209,36 @@ const assert = require('assert');
const freelist = require('internal/freelist');
```

### Assertions

When writing assertions for object comparison, prefer the strict versions,
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Remove object.

When writing assertions for comparisons, prefer the strict versions:

namely:
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Get rid of namely.

...prefer the strict versions:

  • assert.strictEqual()...


* `assert.strictEqual()` over `assert.equal()`
* `assert.deepStrictEqual()` over `assert.deepEqual()`

When using `assert.throws()`, if possible, provide the full error message:
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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)

Copy link
Contributor

Choose a reason for hiding this comment

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

@Trott Any thoughts, how to handle this?

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@joyeecheung joyeecheung Feb 7, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Or

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

Copy link
Member

@Trott Trott Feb 7, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.


```js
assert.throws(
() => {
throw new Error('Wrong value');
},
/^Error: Wrong value$/ // Instead of something like /Wrong value/
);
```

### 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,
Copy link
Member

Choose a reason for hiding this comment

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

Nit: under -> in

Copy link
Member

Choose a reason for hiding this comment

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

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

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

but when writing tests, it is encouraged to use ES.Next features that have
Copy link
Member

Choose a reason for hiding this comment

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

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

However, when writing tests, it is...

already landed in the ECMAScript specification. For example:

* `let` and `const` over `var`
* Template literals over string concatenation
* Arrow functions over normal functions, if appropriate
Copy link
Member

Choose a reason for hiding this comment

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

Nit:

  • Arrow functions when appropriate


## Naming Test Files

Test files are named using kebab casing. The first component of the name is
Expand All @@ -220,3 +250,29 @@ For example, a test for the `beforeExit` event on the `process` object might be
named `test-process-before-exit.js`. If the test specifically checked that arrow
functions worked correctly with the `beforeExit` event, then it might be named
`test-process-before-exit-arrow-functions.js`.

## Imported Tests

### Web Platform Tests

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
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Replace this line with:

These imported tests will be wrapped like this:


```js
/* eslint-disable */
/* WPT Refs:
https://github.com/w3c/web-platform-tests/blob/8791bed/url/urlsearchparams-stringifier.html
License: http://www.w3.org/Consortium/Legal/2008/04-testsuite-copyright.html
*/

// Test code

/* eslint-enable */
```

and want to improve it, please send a PR to the upstream project
Copy link
Member

Choose a reason for hiding this comment

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

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

first, and when it's merged, send another PR to this project
Copy link
Member

Choose a reason for hiding this comment

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

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

to update the code accordingly (be sure to update the hash in the
Copy link
Member

Choose a reason for hiding this comment

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

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

URL following `WPT Refs:`).