Skip to content
This repository was archived by the owner on May 13, 2019. It is now read-only.

[RFC] doc: add How to write a node test guide #30

Closed
wants to merge 2 commits into from

Conversation

santigimeno
Copy link
Member

It's a very basic guide on how to write a node test. It surely needs more work (and syntax corrections!) but it can be a good starting point. Comments are welcome. Thanks.

P.S.: I'm planning to create an accompanying text documenting the common API.

@@ -0,0 +1,139 @@
# How to write a node test
Copy link

Choose a reason for hiding this comment

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

node -> Node.js

@santigimeno
Copy link
Member Author

Updated per @cjihrig comments. Thanks!!

const common = require('../common');
```

These 2 lines are mandatory and should be included on every test.
Copy link

Choose a reason for hiding this comment

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

Can you spell out "two"

@santigimeno
Copy link
Member Author

Updated! Thanks again @cjihrig !

@cjihrig
Copy link

cjihrig commented Apr 21, 2016

LGTM

@jasnell
Copy link
Member

jasnell commented Apr 21, 2016

This should also say something about things like // Flags: --expose-internals ... specifically, adding a // Flags comment in the pre-amble of the test (along with the 'use strict'; allows the test to be run with specific Node.js command line flags set. In the case of --expose-internals, it allows a test to require the various internal/* modules used internally by Node core.

## General recommendations

- The use of timers is discouraged, unless we're testing timers. The reasons for this are multiple. Mainly, they are a source of flakiness. For a thorough explanation go [here](https://github.com/nodejs/testing/issues/27).

Copy link
Member

Choose a reason for hiding this comment

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

Perhaps something about common.platformTimeout() here?

@jasnell
Copy link
Member

jasnell commented Apr 21, 2016

Great work @santigimeno! Great to see this. Left a few suggestions.

@Trott
Copy link
Member

Trott commented Apr 30, 2016

LGTM. Thank you so much for doing this, @santigimeno!

@Trott
Copy link
Member

Trott commented May 12, 2016

Bump! Is this publishable as it stands or does it need a few tweaks?

@santigimeno
Copy link
Member Author

santigimeno commented May 12, 2016

@Trott I wanted to incoporate @jasnell suggestions first, but had no time in the last few weeks :(. I think it can be publishable as it is and can improve it afterwards.
General question: where do you think this doc should live in?

@Trott
Copy link
Member

Trott commented May 12, 2016

General question: where do you think this doc should live in?

It's short enough and important/helpful enough that I would make it part of test/README.md in the main core repository. I think the README needs (at least) three things:

  • How to run the tests
  • How to write tests (what this doc is!)
  • What the various tests are (this is what's there already, although it can use some expansion perhaps)

@Fishrock123
Copy link

Shouldn't this be PR'd to nodejs/node under doc/guides/?

@jasnell
Copy link
Member

jasnell commented May 23, 2016

ah, yes, good point

@santigimeno santigimeno force-pushed the how_to_write branch 3 times, most recently from b7379c2 to d610c5f Compare May 26, 2016 00:23
@santigimeno
Copy link
Member Author

Opened PR @ nodejs/node: nodejs/node#6984, incorporating @jasnell suggestions. Closing this. Thank for all the comments!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants