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

Add code coverage test-run #1231

Merged
merged 4 commits into from Dec 7, 2022

Conversation

doublethefish
Copy link
Contributor

chore(coverage): adds code coverage to tests

Rather than replace the existing test, which would require merging the
coverage output of the two test-run phases (test:src and test:timeout),
we add a new 'test:src:cov' script.

There's a couple of things we notice doing this; the first is that some
of the tests, especially doctor tests, do not report coverage because
they use sub-procs via spawn. This leads us to realise that the
test-types sometimes veer into 'integration' type tests.

We configure the coverage to:

  • cover test-code in addition to run-time code.
  • use high- and low-tide marks that seem reasonable for the library.
  • set a global pass threshold that currently passes but should catch
    most regressions.
  • print coverage results to the console.
  • create a nice html coverage report in './coverage/index.html'.
  • only show the non-fully covered files in text output

@raineorshine
Copy link
Owner

Hi, thanks for the PR.

You may already be aware, but we currently have test coverage in the github action:

- name: Run tests with coverage
run: npm run c8 -- npm run test
if: startsWith(matrix.os, 'ubuntu') && matrix.node == env.NODE_COV
- name: Run Coveralls
uses: coverallsapp/github-action@v1.1.2
if: startsWith(matrix.os, 'ubuntu') && matrix.node == env.NODE_COV
with:
github-token: '${{ secrets.GITHUB_TOKEN }}'

Can you explain what your PR offers beyond the current coverage? If there are additional benefits, I am open to discussing an integrated solution.

@doublethefish
Copy link
Contributor Author

You may already be aware, but we currently have test coverage in the github action:

Ah, no I had missed that CiCd step entirely. Kicking myself as I am sure I'd seen the badge when I first looked at the project.

Can you explain what your PR offers beyond the current coverage? If there are additional benefits, I am open to discussing an integrated solution.

Absolutely,

The benefits of the ability to get coverage data in the local checkout are:

  • you can check iteratively if you have added un-covered code (and see your progress when improving coverage - as I am)
  • it's possible to add husky pre-push checks to ensure the coverage is above the minimum (before wasting CiCd resources).
  • it's more obvious it exists in the package.

Personally, I have found that having coverage front-and-centre (obvious, easy, quick) mostly helps code quality, with caveats.

@raineorshine
Copy link
Owner

raineorshine commented Dec 6, 2022

You may already be aware, but we currently have test coverage in the github action:

Ah, no I had missed that CiCd step entirely. Kicking myself as I am sure I'd seen the badge when I first looked at the project.

No worries, it's a little out of the way.

The benefits of the ability to get coverage data in the local checkout are:

  • you can check iteratively if you have added un-covered code (and see your progress when improving coverage - as I am)
  • it's possible to add husky pre-push checks to ensure the coverage is above the minimum (before wasting CiCd resources).
  • it's more obvious it exists in the package.

Personally, I have found that having coverage front-and-centre (obvious, easy, quick) mostly helps code quality, with caveats.

That sounds reasonable to me. Now, can we do both local coverage and coveralls with just c8? I don't think we need both c8 and nyc. However if there are features that nyc has that c8 doesn't have I'm happy to switch over. I'm not super familiar with the differences myself.

@doublethefish
Copy link
Contributor Author

can we do both local coverage and coveralls with just c8? I don't think we need both c8 and NYC.

great question, I would hope/assume so, because you're right, we don't need/want both. I've not yet come across c8, but I do find NYC problematic, especially since they restructured everything from istanbul->nyc.

I'll have a look at c8.

This makes code-coverage discoverable.

Rather than replace the existing test, which would require merging the
coverage output of the two test-run phases (test:src and test:timeout),
we add a new 'test:src:cov' script.
We ignore build/ and coverage/ out, making prettier run faster and tidier.
@doublethefish
Copy link
Contributor Author

That was easier than I expected.

I don't know how you feel about covering test code, as well as production code, but I've found that test-code coverage occasionally highlights regressions in tests. I've not reintroduced that configuration change for c8, but it looks straight forward enough to do at some point in the future.

I hope you don't mind, I've thrown some typo fixes in (found via cspell, which might be worth integrating).

@raineorshine
Copy link
Owner

That was easier than I expected.

Great!

I don't know how you feel about covering test code, as well as production code, but I've found that test-code coverage occasionally highlights regressions in tests. I've not reintroduced that configuration change for c8, but it looks straight forward enough to do at some point in the future.

I think with a larger codebase and lots of contributors this could be useful. npm-check-updates is pretty small and I personally review all the code, so it may not be as relevant here. Not that I trust my judgment over automated tools, just that changes to tests are pretty visible with the current PR review process.

I hope you don't mind, I've thrown some typo fixes in (found via cspell, which might be worth integrating).

Thank you!

@raineorshine raineorshine merged commit 5d68cc0 into raineorshine:main Dec 7, 2022
@doublethefish doublethefish deleted the chore/code_coverage branch December 7, 2022 15:03
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

2 participants