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

feat: testing guidelines #377

Merged
merged 17 commits into from
Aug 25, 2020
Merged

feat: testing guidelines #377

Changes from 1 commit
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
4df0997
feat: testing guidelines
ghinks Jul 2, 2020
ab1e8f7
feat: testing guidelines
ghinks Jul 5, 2020
211f0b2
Update docs/drafts/testing-guidelines.md
ghinks Jul 8, 2020
3946f71
Update docs/drafts/testing-guidelines.md
ghinks Jul 8, 2020
ccec763
Update docs/drafts/testing-guidelines.md
ghinks Jul 8, 2020
b00aed2
Update docs/drafts/testing-guidelines.md
ghinks Jul 8, 2020
9f88640
move from drafts to docs, as per meeting discussion
ghinks Jul 14, 2020
b0a6452
Update docs/testing-guidelines.md
ghinks Jul 17, 2020
60ee830
Update docs/testing-guidelines.md
ghinks Jul 17, 2020
94b1236
Update docs/testing-guidelines.md
ghinks Jul 18, 2020
c2e3f2f
docs: address @bnb comments
ghinks Jul 18, 2020
62a490b
docs: address @bnb comments
ghinks Jul 18, 2020
276a0d1
docs: address @bnb comments
ghinks Jul 18, 2020
cc8f19d
Update docs/testing-guidelines.md
ghinks Jul 21, 2020
431fcf5
docs: address @bnb comments
ghinks Jul 28, 2020
ce38727
docs: update readme links
ghinks Jul 29, 2020
13b2ee5
Merge branch 'master' into feature/testing-guidelines-update-and-review
ghinks Jul 29, 2020
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
36 changes: 33 additions & 3 deletions docs/drafts/testing-guidelines.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,10 @@ The minimal versions you should focus are:
* LTS (long time support)
* Current

Of course, you are freely to maintain a package that run also with older versions of Node.js that reach the "end-of-life" stage.
Of course, you are free to maintain a package that also runs with older versions of Node.js that have reached the "end-of-life" stage.
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 a bit stronger than "free to"? I'd personally prefer "encouraged to".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

agreed.

ghinks marked this conversation as resolved.
Show resolved Hide resolved

For browser based modules and modules that are designed to work equally both client and server side it is essential to test on various
ghinks marked this conversation as resolved.
Show resolved Hide resolved
versions of your target browser and to publish which environments are supported.

### How?
It is a good idea to have unit tests, coverage that matches most use cases for the module, and make sure things work in all supported environments.
Expand All @@ -25,9 +28,32 @@ It is a good idea to have unit tests, coverage that matches most use cases for t
* **integration test**: test your code with other applications dependencies
* **acceptance test**: test your application sticks in performance, heavy load, etc..

Here are some things to consider as you develop your package.
#### Unit Testing
Copy link
Member

Choose a reason for hiding this comment

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

Should we not just link to something like https://martinfowler.com/bliki/TestPyramid.html?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am happy to add a link

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, wasn't sure - did you mean to do it in this PR or later?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will add a link later. If that is ok. This document is going to be revisited after I do the CI/CD document that this document will link to aswell.

The initial and fundamental start of any testing strategy should start with a unit testing strategy. For the JavaScript language
the unit test may be the first place to catch code that will not parse. It is also the way to define the expectation for a particular
ghinks marked this conversation as resolved.
Show resolved Hide resolved
function or code block. The unit test should describe what to expect from that code when given a defined set of inputs.

For a test to be considered a unit test it should consume no external services and only the code block under test should be considered part of that test. All external services that the code block under test consumes should be
ghinks marked this conversation as resolved.
Show resolved Hide resolved
stubbed. There are many mocking tools available and this document will not favor one set of tools over any other.

It is generally considered good practice to run units as part of any commit or build strategy.

#### Integration Testing
Integration testing in general requires that the code under test be in a package.

For many packages integration testing requires that the built package be run via a testing tool upon several environments. The
field of continuous integration and deployment (CICD) is complex. The package being built shall be run through
ghinks marked this conversation as resolved.
Show resolved Hide resolved
a CICD pipeline. Several integrations are available for GitHub based repositories. (There are also alternatives to GitHub.)
ghinks marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

Pet peeve, but CI/CD pipeline is so incredibly vague, that it can mean pretty much anything... While I'm not sure we need to include these details at all (nor that the definitions of unit vs integration matter that much at all, fwiw), I think we should still link to some examples or in depth articles on this.

I'd very much be in favor of how this is done in JS world, though! Possibly with a distinction on how you do certain things in node vs browser.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed. We even have the whole document on CI/CD dedicated to this subject.

For the testing document maybe we should reference our CI/CD draft.

What both yourself and @bnb have contributed to this document too.

I think we do need to have CI/CD Options documented.

  • what can we do for free for an open source project
  • what providers are there?
  • what does each give us?

This is actually a pretty big area in both open source and the enterprise and I think we should talk about this as an item on 14th July in the Package Maintenance meeting.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dominykas are we ok referring to the CI/CD document. I agree that we need to give concrete examples of what is currently possible in that document and I intend to start that document next.

Copy link
Member

Choose a reason for hiding this comment

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

Sure, we can link and extend the existing doc. I'm just not entirely sure about the term "CI/CD pipeline" - it can mean a gazillion things to different people, so possibly we should simplify the wording towards something like "your tests should run automatically on pr/merge/commit/etc" and link to the doc?

It should not matter if it's a "pipeline" or a single job on Travis. It does not need to be complex. People need not be scared of testing and automation. The simpler the better, esp in the context of oss node packages.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

agreed.


For many open source projects on Github free integration environments are available.

#### Acceptance Testing
Every package will have different criteria for acceptance. The maintenance committee has particular interest in making sure
that dependents of packages are not impacted by the release of a new version. We encourage liaison between both the
publisher and the dependents where possible.

### Links to some useful tools

Some useful tools:
* [c8](https://www.npmjs.com/package/c8)
* [citgm](https://www.npmjs.com/package/citgm)
* [Codecov](https://www.npmjs.com/package/codecov)
Expand All @@ -37,3 +63,7 @@ Some useful tools:
* [Nock](https://www.npmjs.com/package/nock)
* [nyc](https://www.npmjs.com/package/nyc)
* [tape](https://www.npmjs.com/package/tape)
* [Github CICD](https://docs.github.com/en/actions/building-and-testing-code-with-continuous-integration/about-continuous-integration)
* [Gitlab](https://about.gitlab.com/)
* [Circle CI](https://circleci.com/product/)
* [Travis CI](https://travis-ci.com/)