Skip to content

Add instrumentation for test coverage#28

Merged
msimerson merged 15 commits into
haraka:masterfrom
lnedry:master
Mar 9, 2026
Merged

Add instrumentation for test coverage#28
msimerson merged 15 commits into
haraka:masterfrom
lnedry:master

Conversation

@lnedry
Copy link
Copy Markdown
Contributor

@lnedry lnedry commented Feb 11, 2026

Changes proposed in this pull request:

  • added node v22 requirement
  • refactored tests to use local plugin instead of this.plugin
  • added comments to clarify how the config.get closure works
  • added nyc instrumentation for test coverage

Checklist:

  • docs updated
  • tests updated
  • Changes.md updated
  • package.json.version bumped

@msimerson
Copy link
Copy Markdown
Member

  1. Have you somehow surmounted a long standing impediment to getting plugin test coverage? We've used coverage testers in areas of the Haraka code but not plugins, because it doesn't work, because plugins run under vm.runInNewContext().
  2. I've moved Haraka's testing away from nyc to c8, mostly because nyc was really really thinly supported.
  3. I've moved all the coverage dependencies out of the repos and into a GitHub Action. Coverage tools tend not to be kept up-to-date and they bloat the size of installs when most users don't ever use them.

@msimerson
Copy link
Copy Markdown
Member

refactored tests to use local plugin instead of this.plugin

Why?

It's not so important in tests, but storing and accessing stuff on this in plugins is a very useful pattern. We use it all the time so that tests can "reach into" plugins and modify state.

@lnedry
Copy link
Copy Markdown
Contributor Author

lnedry commented Feb 12, 2026

I bypassed the vm just for testing coverage. npm test runs the tests, as expected, in vm. And once all of my tests pass, then I run npm run test:coverage. While it's been helpful for me, it sounds like you might have a better work flow.

For the sake of consistency, I'll change let plugin back to using this.plugin. I'm curious, do you have an example of where using a variable called "plugin", in the test code, would not be able to "reach into" a plugin? I've been using it this way in a couple of my plugins for the last couple of months.

@msimerson
Copy link
Copy Markdown
Member

If you change c8 npm test to npx c8 npm test, then you don't need to declare c8 as a dev dependency in package.json. If you have c8 globally installed on your dev machine, then it'll use that version. If not, it'll install it locally and run it, in one fell swoop. If you have a bunch of modules, installing your dev tools (like eslint, c8, etc..) once globally is a lot faster than installing them every time you npm install in a module.

A feature of c8 is that it has good defaults. Going from memory, I think nearly all of your c8 config is the default. If you add your desired reporter on the CLI (npx c8 --reporter=**** npm test), then there's no need for any of the c8 config.

I'm curious, do you have an example of where using a variable called "plugin", in the test code, would not be able to "reach into" a plugin?

The dkim plugin. We overload this.plugin.config so that in the tests, the config is loaded from ./test/config/ instead of ./config/. A fair number of plugins use this convention.

@lnedry
Copy link
Copy Markdown
Contributor Author

lnedry commented Feb 13, 2026

The dkim plugin. We overload this.plugin.config so that in the tests, the config is loaded from ./test/config/ instead of ./config/. A fair number of plugins use this convention.

FWIW, I forked a copy of the DKIM plugin, modified it to use let plugin instead of this.plugin, and it passed every test. I even added few tests. PR (for the tests) coming later.

@lnedry lnedry changed the title Add nyc instrumentation for test coverage Add instrumentation for test coverage Mar 8, 2026
@lnedry
Copy link
Copy Markdown
Contributor Author

lnedry commented Mar 8, 2026

Coverage testing now uses c8 and it is only loaded when testing coverage.

Comment thread package.json
"prettier": "npx prettier . --check",
"prettier:fix": "npx prettier . --write --log-level=warn",
"test": "node --test",
"test:coverage": "HARAKA_COVERAGE=1 npx c8 npm test",
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Before I moved coverage testing into a GitHub action, I did initiate the convention of npm run cover and dropped that into a few plugins:

➜ grep cover `ghfind` | grep package.json
./plugin/dovecot/package.json:    "coveralls": "DOVECOT_COVERAGE=1 npx mocha --require blanket --reporter mocha-lcov-reporter | npx coveralls.js",
./plugin/geoip/package.json:    "cover": "NODE_ENV=cov npx nyc --reporter=lcovonly npm run test",
./plugin/log-reader/package.json:    "cover": "npx istanbul cover npm test",
./plugin/known-senders/package.json:    "cover": "npx nyc --reporter=lcov --hook-run-in-context npm run test",
./plugin/p0f/package.json:    "cover": "NODE_ENV=cov npx nyc --reporter=lcovonly npm run test",

I like the clarity of the test:coverage name. I also like the finger friendliness of 'cover'. I'm not going to police what script names folks use in their Haraka plugins (like the dovecot plugin above) but if we're making suggestions to authors, I prefer them to be consistent. If you feel strongly about using test:coverage, this PR should be paired with:

  • updating at least the script name in the other plugins
  • preferably updating the script name and invocation to use c8
  • extra credit: getting the coverage testing in each module to work

Comment thread test/index.js Outdated
@msimerson msimerson merged commit faf2a0d into haraka:master Mar 9, 2026
11 checks passed
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.

2 participants