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

Jump to bottom

doc: improve testing guide #11150

Closed
wants to merge 6 commits into from
Closed

doc: improve testing guide #11150

Changes from 5 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
55 changes: 55 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,35 @@ const assert = require('assert');
const freelist = require('internal/freelist');
```

### Assertions

When writing an assertion for comparison, prefer the strict versions:
Copy link
Contributor

Choose a reason for hiding this comment

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

Strictly speaking, assertion is not for comparison.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

For performance considerations, we only use a selected subset of ES.Next
features in JavaScript code in the `lib` directory. However, when writing
tests, it is encouraged to use ES.Next features that have already landed
in the ECMAScript specification. For example:

* `let` and `const` over `var`
* Template literals over string concatenation
* 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 +249,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).
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 */
```

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 here to update Node.js accordingly.
Be sure to update the hash in the URL following `WPT Refs:`.