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

ci: automatically test on all LTS node versions #134

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

msimerson
Copy link

@msimerson msimerson commented Mar 27, 2024

  • automatically test on all LTS node versions (currently: 18, 20)
    • example for testing on extra versions (21)
  • adds windows testing (win tests take ~30 seconds longer)
  • replace codecov dependency (in package.json) with codecov-action (in GHA workflow)
    • has working config for codecov & coveralls (commented out)
  • test: prefix mocha with npx

I'm happy to amend this PR in any way that suits the maintainer(s).

fixes #119

Previews

Screenshot 2024-03-26 at 5 53 07 PM -- Screenshot 2024-03-26 at 6 29 52 PM

@msimerson msimerson marked this pull request as draft March 27, 2024 00:02
@msimerson msimerson force-pushed the ci-lts branch 2 times, most recently from 2220f66 to daefa50 Compare March 27, 2024 00:20
@msimerson msimerson changed the title ci: automatically test on all LTS node ci: automatically test on all LTS node versions Mar 27, 2024
@msimerson msimerson marked this pull request as ready for review March 27, 2024 01:45
- with example for testing on extra versions
- add node 21, as a "not LTS" example
- ci: test windows too, for curiousity
- ci: add coverage workflow
- package.json: replace devDeps with npx
@msimerson
Copy link
Author

Is there anything I can do to nudge this along?

package.json Outdated
"codecov": "^3.8.3",
"nyc": "^15.1.0",
"mocha": "^10.2.0"
},
Copy link
Owner

Choose a reason for hiding this comment

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

With these changes, you can no longer git clone + npm install to get everything you need to work offline. Please revert these changes.

Copy link
Author

@msimerson msimerson Apr 18, 2024

Choose a reason for hiding this comment

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

Will do. Done.

Food for thought:

  • the codecov module is officially deprecated
  • nyc is almost unsupported (last release on npm, 4 years ago).
    • nyc only supports es6 via babel transpilation to es5 😬
    • the bare-bones support is has received is from bcoe, the author of c8, which remains the future of JS coverage reporting, using node's built-in coverage features.
    • The GHA workflow uses nyc, for consistency
  • the big coverage reporters (codecov, coveralls, etc.) are nudging everyone to move to GHA style processing
  • the included GitHub action will report the coverage results in PRs
  • moving the code coverage entirely to a GitHub Action improves the default install (npm -i), because 99.9% 1 of the time, nobody benefits from installing dev modules they'll never use.
    • npm i output now is 28 MB: added 237 packages, and audited 238 packages
    • w/o nyc & codecov: 6.4M ./node_modules
    • w/o nyc, codecov, & mocha: 0M ./node_modules
  • I don't know about you but many of my PRs come from folks that still greatly struggle with git, let alone running npm test.
  • if mocha is installed globally on your machine (as may be the case for a developer who prefers it), running npx mocha will use the globally installed (via npm i -g) version. Not only does this work great offline, but it saves a lot of wasted developer time when doing installs and reinstalls of modules.
  • (aside: mocha is my favorite test runner), removing mocha as a dep pairs nicely with test: replace mocha with node.js builtin #135
  1. Made up statistic, but lets be honest: so few people used npm i --production that all 68 of us noticed when npm changed it to npm i --omit=dev

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.

proposed v3 maintenance
2 participants