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

chore(deps): bump node version #1193

Draft
wants to merge 2 commits into
base: dev
Choose a base branch
from
Draft

chore(deps): bump node version #1193

wants to merge 2 commits into from

Conversation

rhettjay
Copy link

started with bumping the node version then realized it might cut down on dependencies to move it to fastify. It was fast enough that I figured it was worth the effort. I won't be offended if we want to go another direction.

Copy link
Member

@GaryGSC GaryGSC left a comment

Choose a reason for hiding this comment

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

Thanks for the Node.js version update.

I'm also supportive of the switch to Fastify as our general recommendation.

We likely need further discussion about switching to the native test runner.


I added some thoughts about node --test usage. I like the direction, but it has its quirks and differences. It's a little annoying that coverage is still behind a flag. One other feature loss is that we no longer generate the pretty HTML coverage report that we used to get from Jest's LCOV reporter. I'm personally okay with that if we retain the Codecov bits. Here's what the HTML report looked like:
image
image

"scripts": {
"lint": "standard --verbose | snazzy",
"test": "jest"
"test": "node --test server.test.js"
Copy link
Member

@GaryGSC GaryGSC May 1, 2024

Choose a reason for hiding this comment

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

Suggested change
"test": "node --test server.test.js"
"test": "node --test --test-reporter=spec --test-reporter-destination=stdout --experimental-test-coverage --test-reporter=lcov --test-reporter-destination=lcov.info"

This is actually 4 suggestions in 1.

  • Let's stop specifying server.test.js so that additional test files will likely be picked up automatically without needing to update this line. The docs state:

    By default, Node.js will run all files matching these patterns:

    • **/*.test.?(c|m)js
    • **/*-test.?(c|m)js
    • **/*_test.?(c|m)js
    • **/test-*.?(c|m)js
    • **/test.?(c|m)js
    • **/test/**/*.?(c|m)js

    In my limited experience with the native test runner, tests inside of node_modules dependencies aren't picked up, so the default pattern matching should likely work for us.

  • CI is defaulting to tap results instead of the more human-readable spec (due to some auto-detection). We can switch that by specifying --test-reporter=spec.

  • We were previously summarizing test coverage data and reporting it at the end of the test results. We can do that by adding the --experimental-test-coverage flag while using tap or spec.

  • We were previously sending test coverage details to Codecov if a token was provided (see this) by generating an LCOV report and placing it inside a coverage directory. The Codecov action recursively searches for lcov.info, ignoring directories like node_modules, and uploads it to report on diffs and track coverage trends. In order to generate that file and print the human-readable stuff to stdout, we need to do something like --test-reporter=spec --test-reporter-destination=stdout --experimental-test-coverage --test-reporter=lcov --test-reporter-destination=lcov.info. A slightly awkward thing is that we can't specify coverage/lcov.info as the destination without the coverage directory already existing, so we'll have to update our .gitignore.

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.

None yet

3 participants