test(structure): consolidated all verification under the `test` script while also enabling isolated execution #2964

travi · 2023-09-17T14:55:18Z

as a follow up to #2963, we need to adjust the test job, which i think means moving the lint script back into the test_matrix jobs at minimum. i think that gives us a good opportunity to rethink how we structure the jobs, so putting this together for discussion.

this is very close to the structure i normally use outside of semantic-release, but does introduce a few things that we'd agreed to avoid previously. i'm still open to avoiding those things if we decide to, but wanted to start the conversation here so that the alternative approach is more clear by having a functional example to tinker with.

for semantic-release/.github#19

travi · 2023-09-17T15:05:07Z

package.json

    "mockserver-client": "5.15.0",
    "nock": "13.3.3",
+    "npm-run-all2": "6.0.6",


i've used npm-run-all for a long time with success, but the original version has grown stale. npm-run-all2 is a community maintenance fork. see https://github.com/bcomnes/npm-run-all2?tab=readme-ov-file#npm-run-all2-why for more detail

travi · 2023-09-17T15:06:57Z

package.json

    "semantic-release": "./bin/semantic-release.js",
-    "test": "c8 ava --verbose",
-    "test:ci": "c8 ava --verbose"
+    "test": "npm-run-all --print-label --parallel lint:* --parallel test:*",


this more intentionally makes the test script execute all verification scripts. each of the referenced scripts are still available for isolated execution, but this makes them roll up automatically, even if new scripts are added later.

worth noting that the :* glob only matches scripts at a single level, so scripts like lint:js:fix are not matched, but lint:js, lint:lockfile, and lint:engines are.

travi · 2023-09-17T15:09:42Z

package.json

+    "test:unit": "c8 ava --verbose",
+    "test:integration": "ava --verbose test/integration.test.js"


i normally find value in being able to execute unit and integration tests separately and have found ava being configured to always run all tests together a bit awkward. this way they can be run separately, but both suites still get run as part of the overall test script.

i think this is especially valuable since we've had some instability with the integration tests. i think that instability is somewhat to be expected with the broad scope covered by the integration tests. however, being able to run the unit tests without that complexity makes the feedback loop there quicker and more stable

travi · 2023-09-17T15:10:46Z

package.json

+    "test:unit": "c8 ava --verbose",
+    "test:integration": "ava --verbose test/integration.test.js"


i removed c8 from the integration run since i figure our coverage goal is likely more limited to unit tests and including the integration tests ends up skewing those results

i also suspect that c8 may be introducing some of the instability in the already complicated integration tests

travi · 2023-09-17T15:14:07Z

package.json

@@ -6,7 +6,8 @@
  "author": "Stephan Bönnemann <stephan@boennemann.me> (http://boennemann.me)",
  "ava": {
    "files": [
-      "test/**/*.test.js"
+      "test/**/*.test.js",
+      "!test/integration.test.js"


this is what excludes the integration tests from the unit test suite. note that this file is run directly for the script that defines the integration suite. i still am not super strong with this level of detail with ava, so open to feedback if there are better ways to define test suites like the unit and integration that i'm targeting with this change

travi · 2023-09-17T15:17:13Z

.github/workflows/test.yml

+  # verify against the node version defined for development in the .nvmrc
+  test_dev:
+    runs-on: ubuntu-latest
+    timeout-minutes: 5
+
+    steps:
+      - uses: actions/checkout@3df4ab11eba7bda6032a0b82a6bb43b11571feac # v4
+      - run: git config --global user.name github-actions
+      - run: git config --global user.email github-actions@github.com
+      - name: Use Node.js from .nvmrc
+        uses: actions/setup-node@5e21ff4d9bc1a8cf6de233a3057d20ec6b3fb69d # v3
+        with:
+          node-version-file: .nvmrc
+          cache: npm
+      - run: npm clean-install
+      - run: npm audit signatures
+      - run: npm test


this is a duplicate job with the matrix job, but has a separate goal. the matrix suite would be to ensure verification passes for versions defined as supported in engines.node. this job is specific to the version defined in the recently added .nvmrc file, which is intended to define our development node version.

one benefit to this that might not be obvious is that this would prevent a renovate PR to update the node version in the .nvmrc file from being merged if it upgrades to a version where our verification fails

i would totally be open to just adding to the existing matrix list rather than having a separate job, but i've never investigated far enough to find a clean way to read the value from the .nvmrc file and include it in the matrix definition

travi · 2023-09-17T15:19:16Z

.github/workflows/test.yml

@@ -25,7 +26,7 @@ jobs:
          - 20

    runs-on: ubuntu-latest
-    timeout-minutes: 10
+    timeout-minutes: 5


this isnt specific to the restructuring, but noticed it while going through the details.

ava is currently configured to timeout after 2 minutes, so 10 minutes just results in unnecessary waiting

travi · 2023-09-18T22:05:11Z

package.json

+    "lint:prettier:fix": "prettier --write \"*.{js,json,md}\" \".github/**/*.{md,yml}\" \"docs/**/*.md\" \"{bin,lib,test}/**/*.js\"",
+    "lint:lockfile": "lockfile-lint",
+    "lint:engines": "ls-engines",
+    "lint:publish": "publint --strict",


publint is helpful to ensure we get the details right for compatibility with consumers. for example, it would have highlighted that we forgot to define exports before the most recent major version

travi · 2023-09-18T22:10:35Z

.github/workflows/test.yml

      - uses: actions/checkout@3df4ab11eba7bda6032a0b82a6bb43b11571feac # v4
-      - uses: actions/setup-node@5e21ff4d9bc1a8cf6de233a3057d20ec6b3fb69d # v3
+      - run: git config --global user.name github-actions
+      - run: git config --global user.email github-actions@github.com
+      - name: Use Node.js from .nvmrc
+        uses: actions/setup-node@5e21ff4d9bc1a8cf6de233a3057d20ec6b3fb69d # v3
        with:
-          node-version: lts/*
+          node-version-file: .nvmrc
          cache: npm
      - run: npm clean-install
      - run: npm audit signatures
-      - run: npm run lint


since this job is now specifically about summarizing the verification results from the test_dev and test_matrix jobs, it doesnt need to checkout, setup node, or run additional verification

gr2m

looks all good to me so far 👍🏼

travi · 2023-09-20T21:44:37Z

@fgreinacher, @seebeen i'm interested in your thoughts and especially if you have any concerns with this approach. if we do continue down this path, we would probably try to apply a similar approach across at least the core plugins and eventually further.

leaving in draft for now since i still want to update the contributing guide related to these adjustments

fgreinacher · 2023-09-22T14:43:57Z

I like it 💯

and used npm-run-all2 to define verification script groups that can be parallelized while also enabling scripts to be run independantly for semantic-release/.github#19

…deps from malicious registries i thought we'd already taken the step of adding this protection, but it looks like i failed to follow up with getting applied across all repos

…he combined test process

…ased on conversation result

… intended for development to help ensure all verification passes before deciding to update the development version (partly to help automate this process with renovate)

…cific to js

…ackage

…ckout and setup since the purpose of this job is specifically to summarize the verification results

gr2m

great work @travi ❗

while also enabling isolated execution. similar to semantic-release/semantic-release#2964 for semantic-release/.github#19

github-actions · 2023-10-31T01:41:04Z

🎉 This PR is included in version 22.0.6 🎉

The release is available on:

Your semantic-release bot 📦🚀

travi force-pushed the test-structure branch from d43bfcb to cdd67be Compare September 17, 2023 14:59

travi commented Sep 17, 2023

View reviewed changes

travi force-pushed the test-structure branch from 42c2954 to 12fe1d9 Compare September 18, 2023 22:03

travi commented Sep 18, 2023

View reviewed changes

gr2m reviewed Sep 20, 2023

View reviewed changes

This comment has been minimized.

Sign in to view

travi added 10 commits October 8, 2023 10:14

test(structure): split unit tests from integration tests

de41389

and used npm-run-all2 to define verification script groups that can be parallelized while also enabling scripts to be run independantly for semantic-release/.github#19

test(lockfile): added lockfile lint to prevent attacks that download …

a8d8638

…deps from malicious registries i thought we'd already taken the step of adding this protection, but it looks like i failed to follow up with getting applied across all repos

test(engines): moved the ls-engines check into the lint group under t…

65aea33

…he combined test process

docs(contributing): captured notes to update the contributing guide b…

aaed2f4

…ased on conversation result

ci(dev-node): added a job to verify against the version in the .nvmrc…

4739251

… intended for development to help ensure all verification passes before deciding to update the development version (partly to help automate this process with renovate)

test(prettier): adjusted the lint script name since prettier isnt spe…

0586f55

…cific to js

test(publish): added publint to verify the details of the published p…

b5d8041

…ackage

ci(test): removed the lint step from the test job and the related che…

842c0eb

…ckout and setup since the purpose of this job is specifically to summarize the verification results

docs(contributing): updated details about local verification

579259c

docs(contributing): fixed the commitizen config and aligned the docs

d8e16a0

travi force-pushed the test-structure branch from 6cb8ea7 to d8e16a0 Compare October 8, 2023 15:24

travi marked this pull request as ready for review October 8, 2023 15:29

gr2m approved these changes Oct 8, 2023

View reviewed changes

travi merged commit 50cf1aa into master Oct 8, 2023
9 checks passed

travi deleted the test-structure branch October 8, 2023 18:54

travi added a commit to semantic-release/github that referenced this pull request Oct 20, 2023

test(structure): consolidated all verification under the test script

16f9e79

while also enabling isolated execution. similar to semantic-release/semantic-release#2964 for semantic-release/.github#19

travi mentioned this pull request Oct 20, 2023

test(structure): consolidated all verification under the test script semantic-release/github#719

Merged

github-actions bot added the released label Oct 31, 2023

travi mentioned this pull request Nov 1, 2023

refactor(esm): converted the package to esm-only semantic-release/git#491

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test(structure): consolidated all verification under the `test` script while also enabling isolated execution #2964

test(structure): consolidated all verification under the `test` script while also enabling isolated execution #2964

travi commented Sep 17, 2023

travi Sep 17, 2023

travi Sep 17, 2023

travi Sep 17, 2023

travi Sep 17, 2023

travi Sep 17, 2023

travi Sep 17, 2023

travi Sep 17, 2023

travi Sep 17, 2023

travi Sep 17, 2023

travi Sep 17, 2023

travi Sep 17, 2023

travi Sep 18, 2023

fgreinacher Sep 22, 2023

travi Sep 18, 2023

gr2m left a comment

travi commented Sep 20, 2023

This comment has been minimized.

fgreinacher commented Sep 22, 2023

gr2m left a comment

github-actions bot commented Oct 31, 2023

		"test:unit": "c8 ava --verbose",
		"test:integration": "ava --verbose test/integration.test.js"

test(structure): consolidated all verification under the test script while also enabling isolated execution #2964

test(structure): consolidated all verification under the test script while also enabling isolated execution #2964

Conversation

travi commented Sep 17, 2023

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gr2m left a comment

Choose a reason for hiding this comment

travi commented Sep 20, 2023

This comment has been minimized.

fgreinacher commented Sep 22, 2023

gr2m left a comment

Choose a reason for hiding this comment

github-actions bot commented Oct 31, 2023

test(structure): consolidated all verification under the `test` script while also enabling isolated execution #2964

test(structure): consolidated all verification under the `test` script while also enabling isolated execution #2964