feat: testing guidelines #377

ghinks · 2020-07-02T14:57:54Z

Update, fix and add to the current draft of the testing guidelines.

As part of issue 373 here are my updates to the testing guidelines.

It is essential we have some guidelines, although I believe that a whole thesis could be written on this one topic.

Update, fix and add to the current draft of the testing guidelines.

thescientist13

Do you think it's worth moving this out of the /draft directory and removing the "draft" label from the Table of Contents here? Or do think there is still more to be done? (isn't there always 🙃 )

larson-carter

These are great changes!

ghinks · 2020-07-02T18:02:32Z

I think we should move it out. I have meeting later today and I’m hoping to get more contributors to go in after us. It’s never done done.

mhdawson

LGTM

ljharb · 2020-07-03T21:52:29Z

docs/drafts/testing-guidelines.md

@@ -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.


Perhaps something a bit stronger than "free to"? I'd personally prefer "encouraged to".

docs/drafts/testing-guidelines.md

Alter the text so that we do not mention mocking and refer to remote services rather than external.

dominykas · 2020-07-05T13:48:24Z

docs/drafts/testing-guidelines.md

@@ -25,9 +28,31 @@ 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


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

I am happy to add a link

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

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.

dominykas · 2020-07-05T13:51:59Z

docs/drafts/testing-guidelines.md

+
+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
+a CICD pipeline. Several integrations are available for GitHub based repositories. (There are also alternatives to GitHub.)


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.

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.

@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.

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.

docs/drafts/testing-guidelines.md

Co-authored-by: Jordan Harband <[email protected]>

thescientist13

@ghinks
LGTM, I think we should promote this guide and then I think it's good to merge!

ghinks · 2020-07-14T20:54:52Z

@thescientist13 moved from drafts to docs as requested in the meeting

bnb · 2020-07-14T20:57:18Z

docs/testing-guidelines.md

+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.
+
+### Updates
+Node.js has a [strict release pipeline](https://nodejs.org/en/about/releases/) focused on continuous improvement and update. In order to guarantee to your users that the module you made works correctly with the newer version of Node.js, it is a good practice to run your tests across multiple Node.js versions.


focused on continuous improvement and update

this seems like it's slightly incorrect English.

In order to guarantee to your users that the module you made works correctly with the newer version of Node.js

how this is presented, "working correctly with the newer version of Node.js" is a value that's assumed but I don't think it's one that can in fact reasonably be assumed. From the project perspective yes we want things to work but we've not substantiated here why a maintainer would care.

Also, the phrasing is slightly confusing again.

it is a good practice to run your tests across multiple Node.js versions.

I'm confused by this. The previous statement was future looking but how this is approached it's historical or at the very least in the present. Maybe this is just a prose issue, but the sentiment doesn't seem to match the previous section.

After re-reading this after a break of a few days I agree with you @bnb I think some changes are required here. The first sentence about node's strict release schedule should actually be removed and some re-phrasing after wards will be required.

docs/testing-guidelines.md

bnb · 2020-07-14T21:00:54Z

docs/testing-guidelines.md

+* LTS (long time support)
+* Current


How does someone find out what these are? We should provide guidance here.

agreed, there should be a link here.

docs/testing-guidelines.md

bnb · 2020-07-14T21:02:08Z

docs/testing-guidelines.md

+For browser-based modules and modules that are designed to work equally both client and server side it is essential to test on various 
+versions of your target browser and to publish which environments are supported.


Publish where?

bnb · 2020-07-14T21:03:22Z

docs/testing-guidelines.md

+* **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..


Capitalize bullets and don't use etc..

Also given that you have headings under this further describing what's outlined here, you should probably make the bold parts also be relative links.

bnb · 2020-07-14T21:06:43Z

docs/testing-guidelines.md

+#### 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 /


Suggested change

For many packages integration testing requires that the built package be run via a testing tool upon several environments. The /

For many packages, integration testing requires that the built package be run via a testing tool upon several environments. The `/`

not sure if this / is intentional or not?

typo, not sure how the slash got there. Will remove.

@bnb I think that we have satisfied your questions, could we have another review.

bnb · 2020-07-14T21:07:33Z

docs/testing-guidelines.md

+
+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 (CI/CD) is complex. The package being built shall be run through
+a CI/CD pipeline. Several integrations are available for GitHub based repositories. (There are also alternatives to GitHub.)


Several integrations are available for GitHub based repositories. (There are also alternatives to GitHub.)

we should try to substantiate this beyond just saying it exists.

Agreed. Do we want more alternatives for Code like GitLabs or both code and CI/CD ?

IMO both code and CI/CD.

docs/testing-guidelines.md

Co-authored-by: Tierney Cyren <[email protected]>

LTS current references lines 16 Capitalize, bold and link lines 27-28 CI/CD pipeline reference 45, we will link once we do the CI/CD document. That I am sure will be subject to much debate.

address stray slash character.

addressing comment about node's strict release schedule by removing it as it is not pertinent to the maintainer.

ghinks · 2020-07-18T13:26:47Z

I have made a number of changes to address comments and am asking for a further review. I am acknowledging that in a number of places we do need to link to our CI/CD document. But this is a chicken and egg relationship and both are required. It is my intent to come back here to make changes once we complete the CI/CD document.

docs/testing-guidelines.md

thescientist13

@ghinks
Great work so far and thanks for moving out of drafts/ folder. 👍

I think in this case now we can also remove the (draft) from this in the Table of Contents as well

dominykas

Approving to get this moving, assuming followups to be done together with the ci/cd doc.

Co-authored-by: Rodion Abdurakhimov <[email protected]>

docs/testing-guidelines.md

Lower the wording from guarantee to **indication** that a module is intended to work with newer versions.

change readme - remove draft

thescientist13 · 2020-07-29T20:05:42Z

@ghinks
Oops, just noticed you have a conflict in the docs/README. 🙈

ghinks · 2020-07-29T21:40:14Z

I got it.

thescientist13 · 2020-08-06T14:12:22Z

I think this almost good to go now? Any final / outstanding feedback? @bnb do the latest changes look good to you?

thescientist13 · 2020-08-25T20:04:17Z

Discussed in the Package Maintenance meeting on the 8/25/2020 and there was consensus that we should land since there had not be a response from the originator's last objection and we believe the objection had been addressed.

bnb · 2020-08-27T13:49:06Z

Apologies for the delay. Glad to see it landed ❤️

feat: testing guidelines

4df0997

Update, fix and add to the current draft of the testing guidelines.

thescientist13 reviewed Jul 2, 2020

View reviewed changes

larson-carter approved these changes Jul 2, 2020

View reviewed changes

mhdawson approved these changes Jul 3, 2020

View reviewed changes

ljharb requested changes Jul 3, 2020

View reviewed changes

feat: testing guidelines

ab1e8f7

Alter the text so that we do not mention mocking and refer to remote services rather than external.

dominykas reviewed Jul 5, 2020

View reviewed changes

ljharb reviewed Jul 5, 2020

View reviewed changes

docs/drafts/testing-guidelines.md Outdated Show resolved Hide resolved

ghinks requested a review from ljharb July 6, 2020 15:38

ghinks and others added 4 commits July 8, 2020 15:34

Update docs/drafts/testing-guidelines.md

211f0b2

Co-authored-by: Jordan Harband <[email protected]>

Update docs/drafts/testing-guidelines.md

3946f71

Co-authored-by: Jordan Harband <[email protected]>

Update docs/drafts/testing-guidelines.md

ccec763

Co-authored-by: Jordan Harband <[email protected]>

Update docs/drafts/testing-guidelines.md

b00aed2

Co-authored-by: Jordan Harband <[email protected]>

ljharb approved these changes Jul 9, 2020

View reviewed changes

ghinks requested review from thescientist13 and bnb July 10, 2020 14:53

thescientist13 reviewed Jul 11, 2020

View reviewed changes

ghinks requested a review from dominykas July 14, 2020 13:09

move from drafts to docs, as per meeting discussion

9f88640

bnb requested changes Jul 14, 2020

View reviewed changes

ghinks and others added 6 commits July 17, 2020 18:40

Update docs/testing-guidelines.md

b0a6452

Co-authored-by: Tierney Cyren <[email protected]>

Update docs/testing-guidelines.md

60ee830

Co-authored-by: Tierney Cyren <[email protected]>

Update docs/testing-guidelines.md

94b1236

Co-authored-by: Tierney Cyren <[email protected]>

docs: address @bnb comments

c2e3f2f

LTS current references lines 16 Capitalize, bold and link lines 27-28 CI/CD pipeline reference 45, we will link once we do the CI/CD document. That I am sure will be subject to much debate.

docs: address @bnb comments

62a490b

address stray slash character.

docs: address @bnb comments

276a0d1

addressing comment about node's strict release schedule by removing it as it is not pertinent to the maintainer.

ghinks requested a review from bnb July 18, 2020 13:27

rodion-arr reviewed Jul 18, 2020

View reviewed changes

docs/testing-guidelines.md Outdated Show resolved Hide resolved

thescientist13 reviewed Jul 18, 2020

View reviewed changes

dominykas approved these changes Jul 21, 2020

View reviewed changes

Update docs/testing-guidelines.md

cc8f19d

Co-authored-by: Rodion Abdurakhimov <[email protected]>

bnb reviewed Jul 27, 2020

View reviewed changes

docs/testing-guidelines.md Outdated Show resolved Hide resolved

docs: address @bnb comments

431fcf5

Lower the wording from guarantee to **indication** that a module is intended to work with newer versions.

ghinks mentioned this pull request Jul 28, 2020

[docs] continuous integration / continuous delivery (draft) #340

Open

docs: update readme links

ce38727

change readme - remove draft

thescientist13 approved these changes Jul 29, 2020

View reviewed changes

Merge branch 'master' into feature/testing-guidelines-update-and-review

13b2ee5

thescientist13 mentioned this pull request Aug 6, 2020

[docs] testing guidelines (draft) #344

Closed

AshCripps approved these changes Aug 6, 2020

View reviewed changes

ghinks mentioned this pull request Aug 22, 2020

Node.js Package Maintenance Team Meeting 2020-08-25 #397

Closed

thescientist13 added the package-maintenance-agenda Agenda items for package-maintenance team label Aug 22, 2020

ghinks merged commit abb4baf into nodejs:master Aug 25, 2020

		For browser-based modules and modules that are designed to work equally both client and server side it is essential to test on various
		versions of your target browser and to publish which environments are supported.

	For many packages integration testing requires that the built package be run via a testing tool upon several environments. The /
	For many packages, integration testing requires that the built package be run via a testing tool upon several environments. The `/`

feat: testing guidelines #377

feat: testing guidelines #377

Conversation

ghinks commented Jul 2, 2020

thescientist13 left a comment

Choose a reason for hiding this comment

larson-carter left a comment

Choose a reason for hiding this comment

ghinks commented Jul 2, 2020

mhdawson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thescientist13 left a comment

Choose a reason for hiding this comment

ghinks commented Jul 14, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghinks Jul 17, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghinks commented Jul 18, 2020

thescientist13 left a comment • edited Loading

Choose a reason for hiding this comment

dominykas left a comment

Choose a reason for hiding this comment

thescientist13 commented Jul 29, 2020 • edited Loading

ghinks commented Jul 29, 2020

thescientist13 commented Aug 6, 2020

thescientist13 commented Aug 25, 2020

bnb commented Aug 27, 2020

ghinks Jul 17, 2020 •

edited

Loading

thescientist13 left a comment •

edited

Loading

thescientist13 commented Jul 29, 2020 •

edited

Loading