PRO-2913: add writing tests for modules documentation #237

haroun · 2022-08-31T08:56:12Z

Summary

Add Writing tests for modules documentation

What are the specific steps to test this change?

Check the documentation for clarity and typo(s) https://github.com/apostrophecms/a3-docs/blob/pro-2913-writing-tests-for-modules/docs/guide/writing-tests-for-modules.md

What kind of change does this PR introduce?

(Check at least one)

Make sure the PR fulfills these requirements:

It includes a) the existing issue ID being resolved, b) a convincing reason for adding this feature, or c) a clear description of the bug it resolves
The changelog is updated
Related documentation has been updated
Related tests have been updated

If adding a new feature without an already open issue, it's best to open a feature request issue first and wait for approval before working on it.

Other information:

linear · 2022-08-31T08:56:14Z

PRO-2913 Add documentation about testing modules

At the moment there is no documentation explaining how to test apostrophe 3 modules. The goal is to add it to the apostrophe guides or cookbooks.

BoDonkey

Overall, this is a preliminary review. I've indicated areas where I think this needs additional content. What you have written so far will need some light editing, but I didn't want to go through it too carefully until you've fleshed out the additional content, potentially changing the existing content.

docs/guide/writing-tests-for-modules.md

BoDonkey · 2022-09-02T13:17:29Z

docs/guide/writing-tests-for-modules.md

+describe('%module-name%', function () {
+  let apos;
+
+  this.timeout(t.timeout);


Do you need to set the environment variable to use t.timeout?

Yes, there is more on that below in the file https://github.com/apostrophecms/a3-docs/pull/237/files#diff-fb2950150785db8f273073724f8a9db63c2d3425609707858da4688a226ea278R138

Can you add another code comment if this is optional or not and maybe be more explicit in your 'Output' section about the steps to run the test. So do you normally use export TEST_TIMEOUT=5000, and then npm test, or feed it in directly?

docs/guide/writing-tests-for-modules.md

haroun · 2022-09-13T13:25:39Z

@BoDonkey I've changed the example from sitemap to login-hcaptcha.

I didn't work as expected with sitemap for some reasons.

docs/guide/writing-tests-for-modules.md

BoDonkey · 2022-09-26T20:07:05Z

docs/guide/writing-tests-for-modules.md

+
+This documentation focuses on how to test Apostrophe modules, not Apostrophe itself using integration testing.
+
+Modules should be stand-alone npm packages.


Can we expand this to something like: "Modules should be stand-alone npm packages. They need to be hosted somewhere that our testing can access. For this example, we are hosting this module in a github repo. We will use a fictitious module named article to illustrate how to write tests for modules."

My bad -> "github" -> "GitHub"

BoDonkey

Getting closer. Just needs a little fleshing out to help people understand how to better extend tests.

docs/guide/writing-tests-for-modules.md

* main: (28 commits) fix missing async small corrections add further comment edits add images and change text rearrangement and corrections based on comments comment changes Add "showQuery" documentation in docs/reference/modules/piece-page-type.md add missing word update with comment corrections fix typo text changes from second comments further text changes comment changes additional editing change admin addition changes based on comments and additional content finish TOC and end section fix sidebar Add to sidebar add initial page ...

BoDonkey · 2022-10-04T12:21:15Z

docs/guide/writing-tests-for-modules.md

+  };
+
+  // describe and it functions are provided by mocha
+  // Usually use to "structure" the test file


"use" -> "used"

BoDonkey · 2022-10-04T12:23:49Z

docs/guide/writing-tests-for-modules.md

+  // Usually use to "structure" the test file
+  // They don't have a strict meaning from mocha's point of view
+
+  // describe is a test suite and used to group tests


"...suite and is used..."

BoDonkey · 2022-10-04T12:25:46Z

docs/guide/writing-tests-for-modules.md

+  describe('article', function () {
+    let apos;
+
+    this.timeout(t.timeout);


Can you add a comment about whether this is optional and that you need to set an environment variable? I know you have a description of setting it below, but a reminder here would be good.

BoDonkey

Almost there, just a few grammatical errors (some my fault) and I think a need for a little clarification. There is also the topic I started on Slack with respect to where in the Guide we should have your article.

boutell · 2022-10-04T12:46:50Z

docs/guide/writing-tests-for-modules.md

+Tests are useful to guarantee that the requirements of your application are met over time.
+They also ensure that code changes don't generate breaking artifacts.
+
+Automating tests for expected scenarios saves significant time over having to mock those same scenarios manually each


"Mocking" is usually a thing that happens inside automated tests, so this sentence doesn't make sense to me as written. Maybe just end with "... over having to manually execute a regression test plan each time there are code changes."

boutell · 2022-10-04T12:50:48Z

docs/guide/writing-tests-for-modules.md

+
+This documentation focuses on how to test Apostrophe modules, not Apostrophe itself using integration testing.
+
+Modules should be stand-alone npm packages. They need to be hosted somewhere that our testing can access. For this


I don't think our docs cover turning Apostrophe modules into npm packages in general yet. And I think a lot of people reading this page will want to know about how to write integration tests for their project-level modules. Maybe we're not ready to address that yet but I think it's probably the more common use case for our audience.

I'm not saying we can't get this article out there, but we should follow up with a page about how to publish your open source apostrophe module, and an update to this page to cover integration tests for project level modules.

boutell · 2022-10-04T12:52:11Z

docs/guide/writing-tests-for-modules.md

+- [eslint-config-apostrophe](https://www.npmjs.com/package/eslint-config-apostrophe): *An ESLint configuration for ApostropheCMS core and officials modules*
+- [mocha](https://www.npmjs.com/package/mocha): *Simple, flexible, fun JavaScript test framework for Node.js & The Browser*
+```sh
+npm install --save-dev apostrophe eslint eslint-config-apostrophe mocha


Linting and integration testing aren't the same thing. Should we acknowledge this somewhere, maybe mention we're including linting too and what that gets us?

boutell · 2022-10-04T12:52:29Z

docs/guide/writing-tests-for-modules.md

+  </template>
+</AposCodeBlock>
+
+Add the following to `.eslintrc.json` to tell eslint to use ApostropheCMS ESLint confiuration.


Good chance to say why and what this gets us.

I think an additional sentence here or below the code block at around line 77 should be added. Something like:
"While not strictly part of integration testing, running ESLint alongside the Mocha test(s) reduces potential code error and ensures your module code conforms to the style of other Apostrophe modules."

Note: I'm not sure if this is the most accurate statement. You might want to edit it.

boutell · 2022-10-04T13:00:44Z

docs/guide/writing-tests-for-modules.md

+  ```json
+  {
+    "/**": "This package.json file is not actually installed.",
+    " * ": "Apostrophe requires that all npm modules to be loaded by moog",


Don't mention moog. Just say "npm modules to be loaded"

Our v3 docs don't talk about moog, it's an internal detail for us to worry about; they talk about apostrophe modules and how they are loaded but this is no longer a "brand" we use.

* main: (61 commits) Resolve addition to conditional fields section of A3 docs good doc link fixed broken links, added missing link further comment edits Updates based on first comments changes based on comments slight edit add documentation for rebundleModules modifies the guide page edit based on comments response to comments and adds stream monor change change login reference add apiRoutes section fix heroku link errata all but apiRoutes further edits sending email init changes in response to comments ...

* main: (28 commits) rearrange and add new option edit in response to comments edits in response to comments edits in response to first comments add link and anchor info minor correction to code example and file location changed wording minor wording change remainder of command menu doc add new page to sidebar response to comments updates example and link for authentication API Further comment responses further comment edits further corrections changes in response to comments add new GA4 tag and remove unsupported tag manager adds clarity tag corrects template slot required comment ...

BoDonkey

I have a few minor changes/points. I'm not sure if Tom will chime back in with additional chages.

BoDonkey · 2023-01-05T11:23:08Z

docs/guide/writing-tests-for-modules.md

+  </template>
+</AposCodeBlock>
+
+Add the following to `.eslintrc.json` to tell eslint to use ApostropheCMS ESLint confiuration.


I think an additional sentence here or below the code block at around line 77 should be added. Something like:
"While not strictly part of integration testing, running ESLint alongside the Mocha test(s) reduces potential code error and ensures your module code conforms to the style of other Apostrophe modules."

Note: I'm not sure if this is the most accurate statement. You might want to edit it.

BoDonkey · 2023-01-05T12:37:33Z

docs/guide/writing-tests-for-modules.md

+
+We will start with a single test file `test/index.js`.
+
+As your test grows, you can break them down into multiple files based on your liking.


"As your test grows, you can break it down into multiple files based on your liking."

BoDonkey · 2023-01-05T13:09:13Z

docs/guide/writing-tests-for-modules.md

+
+### Output
+
+By running the test using the example `@apostrophecms/login-hcaptcha` module details and the `npm test` command, you should see the output below


I think this is confusing. First, you've set up to run the tests using the hypothetical article module. I realize that you include the example URL for '@apostrophecms/login-hcaptcha`, but sticking with the one example the whole way through is less confusing. Second, that module has a lot of tests. I'm worried that if the end reader attempts to run the tests for themselves with the actual module, the output will be significantly different and confusing. I would actually mockup the expected results from running this on the example module.

I'm OK with you guys taking this the rest of the way.

boutell · 2023-02-01T15:55:51Z

What happened to this PR?

add writing tests for modules documentation

3ee8009

haroun requested a review from BoDonkey August 31, 2022 08:56

haroun self-assigned this Aug 31, 2022

haroun added 2 commits August 31, 2022 11:01

use fixed anchor for link to changelog

ff764f4

cosmetic changes

43a841b

haroun changed the title ~~add writing tests for modules documentation~~ PRO-2913: add writing tests for modules documentation Aug 31, 2022

BoDonkey requested changes Sep 2, 2022

View reviewed changes

haroun added 2 commits September 13, 2022 11:29

add test output and missing detail links

f0c2af5

use login hcaptcha as a reference

9d73c0d

haroun requested a review from BoDonkey September 13, 2022 13:24

use fictitious article module

a4a8f4e

BoDonkey reviewed Sep 26, 2022

View reviewed changes

docs/guide/writing-tests-for-modules.md Outdated Show resolved Hide resolved

BoDonkey reviewed Sep 26, 2022

View reviewed changes

docs/guide/writing-tests-for-modules.md Outdated Show resolved Hide resolved

BoDonkey reviewed Sep 26, 2022

View reviewed changes

BoDonkey requested changes Sep 26, 2022

View reviewed changes

haroun added 2 commits October 3, 2022 11:59

add context to function call and AposCodeBlock to snippets

d6fd36e

haroun requested a review from BoDonkey October 3, 2022 13:00

BoDonkey reviewed Oct 4, 2022

View reviewed changes

BoDonkey requested changes Oct 4, 2022

View reviewed changes

boutell requested changes Oct 4, 2022

View reviewed changes

haroun added 4 commits November 11, 2022 09:58

fix typos

195c2e2

adding linting details

9c6df58

haroun requested a review from BoDonkey January 2, 2023 10:57

BoDonkey requested changes Jan 5, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PRO-2913: add writing tests for modules documentation #237

PRO-2913: add writing tests for modules documentation #237

haroun commented Aug 31, 2022 •

edited

Loading

linear bot commented Aug 31, 2022

BoDonkey left a comment

BoDonkey Sep 2, 2022

haroun Sep 7, 2022

BoDonkey Oct 4, 2022

haroun commented Sep 13, 2022

BoDonkey Sep 26, 2022

BoDonkey Oct 4, 2022

BoDonkey left a comment

BoDonkey Oct 4, 2022

BoDonkey Oct 4, 2022

BoDonkey Oct 4, 2022

BoDonkey left a comment

boutell Oct 4, 2022

boutell Oct 4, 2022

boutell Oct 4, 2022

boutell Oct 4, 2022

BoDonkey Jan 5, 2023

boutell Oct 4, 2022

boutell Oct 4, 2022

BoDonkey left a comment

BoDonkey Jan 5, 2023

BoDonkey Jan 5, 2023

BoDonkey Jan 5, 2023

boutell Jan 5, 2023

boutell commented Feb 1, 2023


		This documentation focuses on how to test Apostrophe modules, not Apostrophe itself using integration testing.

		Modules should be stand-alone npm packages.


		We will start with a single test file `test/index.js`.

		As your test grows, you can break them down into multiple files based on your liking.


		### Output

		By running the test using the example `@apostrophecms/login-hcaptcha` module details and the `npm test` command, you should see the output below

PRO-2913: add writing tests for modules documentation #237

Are you sure you want to change the base?

PRO-2913: add writing tests for modules documentation #237

Conversation

haroun commented Aug 31, 2022 • edited Loading

Summary

What are the specific steps to test this change?

What kind of change does this PR introduce?

Make sure the PR fulfills these requirements:

linear bot commented Aug 31, 2022

BoDonkey 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

haroun commented Sep 13, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BoDonkey 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

BoDonkey 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

BoDonkey 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

boutell commented Feb 1, 2023

haroun commented Aug 31, 2022 •

edited

Loading