Skip to content

doc: edit writing-tests.md #10585

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

Closed
Trott wants to merge 2 commits into from
Closed

doc: edit writing-tests.md #10585

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9b2a9b0
doc: edit writing-tests.md
Trott Jan 3, 2017
00fa7e2
squash
Trott Jan 11, 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
Next Next commit
doc: edit writing-tests.md 
* Remove passive voice
* Remove unneeded modifiers
* Minor comma change
  • Loading branch information
@Trott
Trott committed Jan 12, 2017
commit 9b2a9b0607d03483df5f1d49f983e24c63ac351d
61 changes: 31 additions & 30 deletions doc/guides/writing-tests.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,15 +12,15 @@ with code `0` on success. A test will fail if:
- It never exits. In this case, the test runner will terminate the test because
it sets a maximum time limit.

Tests can be added for multiple reasons:
Add tests when:

- When adding new functionality.
- When fixing regressions and bugs.
- When expanding test coverage.
- Adding new functionality.
- Fixing regressions and bugs.
- Expanding test coverage.

## Test structure

Let's analyze this very basic test from the Node.js test suite:
Let's analyze this basic test from the Node.js test suite:

```javascript
1 'use strict';
Expand Down Expand Up @@ -59,11 +59,12 @@ the nature of the test requires that the test run without it.
The second line loads the `common` module. The `common` module is a helper
module that provides useful tools for the tests.

Even if no functions or other properties exported by `common` are used in a
test, the `common` module should still be included. This is because the `common`
module includes code that will cause tests to fail if variables are leaked into
the global space. In situations where no functions or other properties exported
by `common` are used, it can be included without assigning it to an identifier:
Even if a test uses no functions or other properties exported by `common`,
the test should still include the `common` module. This is because the `common`

@thefourtheye thefourtheye Jan 3, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

"include the common module, as the first executable statement" maybe?

@gibfahn gibfahn Jan 3, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Or maybe to be really clear just say that the first two lines of the test must be:

'use strict';
const common = require('../common');

or

'use strict';
require('../common');

@thefourtheye thefourtheye Jan 3, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Generally, this holds good. But in some tests we have comments at the beginning (I mean before common) and that is also acceptable.

@gibfahn gibfahn Jan 3, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is there a reason we couldn't just standardise on what's in this guide?

'use strict';
require('../common');

// Comments after this

@gibfahn gibfahn Jan 3, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If we did that we could lint for it.

@thefourtheye thefourtheye Jan 3, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Lets see what @Trott thinks about it.

@Trott Trott Jan 11, 2017 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

How about include the common module before any other modules. as a reasonable concise compromise that we can expand on later if it is found to be inadequate?

module includes code that will cause tests to fail if there are variables leaked

@gibfahn gibfahn Jan 3, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why the passive->active change here? Is this a general rule we follow? I find if there are variables leaked into the global space less clear than if variables are leaked into the global space.

@Trott Trott Jan 8, 2017 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

How about if the test leaks variables into the global space?

The passive->active change is just following a general writing recommendation for increased clarity, but of course we should stick to passive voice if changing to active does not increase clarity. :-D

into the global space. In situations where a test uses no functions or other
properties exported by `common`, include it without assigning it to an
identifier:

```javascript
require('../common');
Expand All @@ -86,49 +87,49 @@ const assert = require('assert');
const http = require('http');
```

These modules are required for the test to run. Except for special cases, these
modules should only include core modules.
The `assert` module is used by most of the tests to check that the assumptions
for the test are met.
Note that require statements are sorted, in
The test checks functionality in the `http` module.

Most tests use the `assert` module to confirm expectations of the test.

The require statements are sorted in
[ASCII](http://man7.org/linux/man-pages/man7/ascii.7.html) order (digits, upper
case, `_`, lower case).

### **Lines 10-21**

This is the body of the test. This test is quite simple, it just tests that an
This is the body of the test. This test is simple, it just tests that an
HTTP server accepts `non-ASCII` characters in the headers of an incoming
request. Interesting things to notice:

- If the test doesn't depend on a specific port number then always use 0 instead
of an arbitrary value, as it allows tests to be run in parallel safely, as the
operating system will assign a random port. If the test requires a specific
port, for example if the test checks that assigning a specific port works as
expected, then it is ok to assign a specific port number.
- If the test doesn't depend on a specific port number, then always use 0
instead of an arbitrary value, as it allows tests to run in parallel safely,
as the operating system will assign a random port. If the test requires a
specific port, for example if the test checks that assigning a specific port
works as expected, then it is ok to assign a specific port number.
- The use of `common.mustCall` to check that some callbacks/listeners are
called.
- The HTTP server is closed once all the checks have run. This way, the test can
- The HTTP server closes once all the checks have run. This way, the test can
exit gracefully. Remember that for a test to succeed, it must exit with a
status code of 0.

## General recommendations

### Timers

The use of timers is discouraged, unless timers are being tested. There are
multiple reasons for this. Mainly, they are a source of flakiness. For a thorough
Avoid timers unless the test is specifically testing timers. There are multiple
reasons for this. Mainly, they are a source of flakiness. For a thorough
explanation go [here](https://github.com/nodejs/testing/issues/27).

In the event a timer is needed, it's recommended using the
`common.platformTimeout()` method, that allows setting specific timeouts
In the event a test needs a timer, consider using the
`common.platformTimeout()` method. It allows setting specific timeouts
depending on the platform. For example:

```javascript
const timer = setTimeout(fail, common.platformTimeout(4000));
```

will create a 4-seconds timeout, except for some platforms where the delay will
be multiplied for some factor.
will create a 4-seconds timeout on most platforms but a longer timeout on slower

@gibfahn gibfahn Jan 3, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

4-seconds->4-second

platforms.

### The *common* API

Expand Down Expand Up @@ -193,9 +194,9 @@ var server = http.createServer(common.mustCall(function(req, res) {
### Flags

Some tests will require running Node.js with specific command line flags set. To
accomplish this, a `// Flags: ` comment should be added in the preamble of the
accomplish this, add a `// Flags: ` comment in the preamble of the
test followed by the flags. For example, to allow a test to require some of the
`internal/*` modules, the `--expose-internals` flag should be added.
`internal/*` modules, add the `--expose-internals` flag.
A test that would require `internal/freelist` could start like this:

```javascript
Expand Down