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.

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

A first draft of testing guidelines #169

Merged
merged 4 commits into from
Apr 24, 2019

Conversation

craftninja
Copy link
Contributor

@craftninja craftninja commented Mar 1, 2019

Maybe changes:

  • links to good examples not yet
  • discuss move to ci-cd guidelines? or does some of that content move here? how do we differentiate between these to guideline pages? we will have both sections for now

What is this?

Hello friends!

This is a first draft of testing guidelines. I started using the conversation in #157

obligatory gif


Here are some things to consider as you develop your package.

Some useful tools:
Copy link
Member

Choose a reason for hiding this comment

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

It’d be good to clarify browser/engine support of each, paired with a statement higher up that it’s important to test in all supported environments

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ljharb I added something about making sure things work in all supported environments, do you think that is clear enough? Or do we want to go into details about how?

Copy link
Contributor

Choose a reason for hiding this comment

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

@ljharb could you clarify what you mean? I think I have an idea but I'm pretty sure I'm incorrect.

Copy link
Member

Choose a reason for hiding this comment

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

What i mean is, most test runners don’t support older nodes; jest and mocha require 6+, etc. Before choosing a tool, people should be informed as to the support it offers, and ide.ly should be encouraged to choose the tools that allow the widest support possible.

Testing is useful for developing new features, refactoring with confidence, and making sure new things don't break old things. When other people rely on your code for their own applications, testing helps you make sure things don't break for more than just yourself!

Testing is even more important when new maintainers take over a package. Sometimes when we work on a codebase for a long time we forget to articulate all the cognitive load to which we have become accustomed. Good test coverage can alleviate this burden.

Copy link
Member

Choose a reason for hiding this comment

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

Could be useful to add also a line for:

  • unit test: test your code
  • integration test: test your code with other applications dependencies
  • acceptance test: test your application sticks in performance, heavy load, etc..

Copy link
Member

Choose a reason for hiding this comment

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

I’d be careful about unit vs integration; unit tests can also integrate across all dependencies.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Eomm Added your bullet points to the How section, see if that flow looks good for you.
@ljharb - Do you think something needs to be reworded or added to reflect this concern?

Copy link
Member

Choose a reason for hiding this comment

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

I would avoid categorizing types of tests at all.

Copy link
Member

Choose a reason for hiding this comment

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

@ljharb When I said "dependancies" I was meaning a DB or whatever outside of the module domain. I think that this concept let programmers design better their module because if they write all global vars (for example) they will have pain also to write some unit-test. For this example, I think that there is still confusion (for what I see in my daily job) and focus on "target" words could unlock a new design-thinking. Of course, a BIG package doesn't need this information, but when I try to figure out who will read the docs we are writing, I'm thinking of a young dev that wants to write the "perfect module" and I would like to know that exists multiple types of test so I can go deeper ✌
What do you think of this vision?

Copy link
Member

Choose a reason for hiding this comment

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

I think that it’s such a complex subject that we shouldn’t delve into it.

What you mean isn’t “dependencies” (which in this context are the things npm install installs) but low-level i/o - the network, the filesystem, etc. Certainly your tests can either mock these out or exercise them. However, the common case is to mock out low-level i/o and otherwise unit test (by which i mean, fully integration test all of your code, except for that low-level i/o) all of your code’s observable API and semantics.

Copy link
Contributor

Choose a reason for hiding this comment

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

I tend to agree with @ljharb here. If needed, this can be expanded later in a different proposal.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So do we want to delete contents in ### How section but leave Some useful tools: section?

@mhdawson recommended adding some advice around tools like citgm. I might wait and see if we want to pull back from definitions and recommendations for now... but be glad to add/remove if we think we need some or both or none.

@craftninja craftninja added the package-maintenance-agenda Agenda items for package-maintenance team label Mar 1, 2019
@mhdawson
Copy link
Member

mhdawson commented Mar 5, 2019

It would be great if we could include advice that would help the tests be run in tools like CITGM. This might include using a common target and ensuring that the result is easy to identify as passed or failed.

@mhdawson
Copy link
Member

mhdawson commented Mar 5, 2019

Some info on how to generate/track coverage might also be useful (@bcoe FYI)

@mhdawson
Copy link
Member

mhdawson commented Mar 5, 2019

Agree versus the potential overalp with the CI/CD we might start with cross/references but I do think it is important that we have a 'template' whether that be suggestions or templates that can be re-used. Maybe even a script which adds the recommended target to the package.json and any other concreate artifacts that can be generated. The closer we can get to adding what's needed (in the simple case) versus recommeding what to do the better.

Some useful tools:
* [Jest](https://www.npmjs.com/package/jest)
* [Mocha](https://www.npmjs.com/package/mocha)
* [tape](https://www.npmjs.com/package/tape)
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps add these, since they're useful tools (but aren't necessarily testing libs):

* [c8](https://www.npmjs.com/package/c8)
* [nyc](https://www.npmjs.com/package/nyc)
* [coveralls](https://www.npmjs.com/package/coveralls)
* [codecov](https://www.npmjs.com/package/codecov)
* [nock](https://www.npmjs.com/package/nock)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ordering in "importance" seems like it could be a long discussion, so added, and alphabetized, and cased according to documentation / descriptions from npm.

Updated!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mhdawson Do you think these updated links are "enough" for now regarding citgm and tracking test coverage?

@bcoe
Copy link

bcoe commented Mar 25, 2019

I'd be happy to write up a blurb about why I love test coverage. Is this something you'd be open to @craftninja; could be a follow up PR sometime soon.

@craftninja
Copy link
Contributor Author

@bcoe For sure, I would definitely be open to this. I'd be glad to add you to my fork for this PR as a contributor, or feel free to try out the new "suggestion" feature if that works (not sure if you end up on the diff or not 🤔 ).

Screen Shot 2019-03-25 at 2 16 04 PM

@mhdawson
Copy link
Member

mhdawson commented Apr 8, 2019

Removing from the agenda, please put back on if/when you want it to be discussed in the next meeting.

Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM We agreed in the last meeting that this could land as a first step since it is stil lin the drafts section. I think a good next step would be to add the supported Node levels for the tools as suggested by @ljharb

@mhdawson mhdawson merged commit ccbcd56 into nodejs:master Apr 24, 2019
@craftninja craftninja removed the package-maintenance-agenda Agenda items for package-maintenance team label Apr 25, 2019
@craftninja craftninja deleted the testing-guidelines branch April 25, 2019 15:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants