Skip to content

test: Enable optional coverage reporting#8306

Merged
jprenken merged 1 commit intoletsencrypt:mainfrom
akshithg:test-measurements-on-main
Aug 26, 2025
Merged

test: Enable optional coverage reporting#8306
jprenken merged 1 commit intoletsencrypt:mainfrom
akshithg:test-measurements-on-main

Conversation

@akshithg
Copy link
Copy Markdown
Contributor

@akshithg akshithg commented Jul 13, 2025

Add new flag -c / --coverage to enable optional coverage reporting for all the test types.

@akshithg akshithg requested a review from a team as a code owner July 13, 2025 19:06
@akshithg akshithg requested a review from jsha July 13, 2025 19:06
Copy link
Copy Markdown
Contributor

@aarongable aarongable left a comment

Choose a reason for hiding this comment

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

Can you speak more to your motivation for adding coverage support, especially to the integration tests? We don't believe in using coverage as a metric for code quality; we find that it often drive perverse incentives and leads to worse code and worse tests just as often as it leads to improvements. I think that adding a ./t.sh -uc mode for coverage reports on the unit tests makes a lost of sense, as it can be useful during one's own development cycle, but the rest of this PR feels extraneous to me.

I've left small technical comments below, but mostly I think my feedback is this: pare this down to just a new -c flag in test.sh which adds coverage output for the Go unittests, and we'll gladly accept it.

Comment thread test.sh Outdated
Comment thread test.sh
Comment thread test.sh Outdated
Comment thread test.sh Outdated
Comment thread test.sh Outdated
Comment thread test/integration-test.py Outdated
Comment thread test/integration-test.py Outdated
Comment thread test/startservers.py Outdated
Comment thread test/startservers.py Outdated
@akshithg
Copy link
Copy Markdown
Contributor Author

Hi @aarongable,
Thanks for the feedback --- I understand and agree with your concern that coverage is not a direct measure of code quality, and I'm not looking to introduce it as a metric for pass/fail or as a target to game.

My motivation for adding coverage support to the integration (chisel) tests is tied to a separate effort: building a set of auto-generated, end-to-end ACMEv2 protocol tests directly from the RFC's state machine (including error states, transitions, and lesser-used valid flows). This is an early step toward a protocol-level fuzzer for ACME-compliant CAs.

The reason I wanted coverage here is to be able to compare these auto-generated tests against existing Boulder test suites --- both unit tests and chisel tests --- to see what new paths, error cases, or protocol states they hit. For example, our initial runs already exercise certain missing cases (e.g., account deactivation flow, some invalid transitions) that aren't currently covered in chisel.

I see coverage here as a diagnostic tool rather than a quality metric --- a way to measure complementarity between different classes of tests and identify protocol flows that aren't currently exercised. Once we know the gaps, we can make more targeted improvements to the tests without relying solely on human recall of the RFC.

Do you think this reframing --- using integration test coverage purely as a comparative diagnostic --- would address your concern? If so, I'm happy to make the PR minimal for now and keep the broader protocol-test work in a follow-up proposal.

@aarongable
Copy link
Copy Markdown
Contributor

Thanks for the explanation! I think that's a fantastic goal, and I'm totally in favor of adding the ability to easily get coverage reports from our unit and integration tests.

One minor concern: tracking Boulder code-level coverage is a very different beast from tracking ACME protocol-level coverage. Huge portions of the Boulder codebase are dedicated to handling the difference between a dns-01 challenge and a tls-alpn-01 challenge, or the difference between a DNS identifier and an IP identifier, or the difference between an RSA pubkey and an ECDSA pubkey... but none of those differences appear anywhere in the ACME state diagrams. So I'm not totally sure how tracking code-level coverage helps with this effort. But that's relatively minor, and I'm still in favor of easy-to-access coverage reports as a diagnostic and development tool.

That said, I'd like this to take a slightly different shape: I'd like to only track coverage from our Go-based integration tests. We are actively working on getting rid of the python-based chisel tests (see #8258, #8085, and #8082, for example). If we want to improve protocol-level coverage of the Boulder integration tests, I want all of that improvement to happen within the Go integration tests, not the python ones.

@akshithg
Copy link
Copy Markdown
Contributor Author

Got it --- thanks for clarifying, and I see your point about the difference between code-level and protocol-level coverage. For my purposes, I'm fine using code-level coverage as a proxy in the short term to spot untested flows, and then drilling deeper into protocol-specific coverage metrics separately.

I wasn't aware of the active effort to retire the Python-based chisel tests. That makes sense, and I'm happy to align with that direction.

For this PR, I can pare it back so that:

  • Coverage support applies to Go-based unit tests and Go-based integration tests only.
  • Chisel-specific coverage logic is removed.

That way, we still gain the diagnostic capability you're supportive of, while keeping future improvements in line with the Go-only integration testing plan.

Comment thread test/integration-test.py Outdated
@akshithg akshithg changed the title Adds support for test coverage and minor test improvements Enable coverage reporting for Go unit & integration tests Aug 13, 2025
@akshithg
Copy link
Copy Markdown
Contributor Author

@aarongable I've incorporated all the changes you suggested. Can you please review?

Copy link
Copy Markdown
Contributor

@aarongable aarongable left a comment

Choose a reason for hiding this comment

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

LGTM! I've tested this and it seems to work nicely. I have a few small style comments below, but mostly this looks great!

Comment thread test.sh Outdated
Comment thread test.sh
Comment thread test/integration-test.py Outdated
Comment thread test.sh Outdated
@akshithg
Copy link
Copy Markdown
Contributor Author

@aarongable Thanks for reviewing and for your helpful suggestions. @jsha When you have a chance, could you review and approve?

aarongable
aarongable previously approved these changes Aug 15, 2025
@aarongable
Copy link
Copy Markdown
Contributor

LGTM, needs a second review + approval to merge.

@akshithg
Copy link
Copy Markdown
Contributor Author

Hi @jsha,
I hope you’re doing well. I wanted to kindly follow up on this PR. @aarongable has already reviewed and approved the changes after the revisions, and I’d greatly appreciate it if you could take a look when you have a moment and provide the second review/approval so we can get this merged.

Thank you very much!

@beautifulentropy
Copy link
Copy Markdown
Member

beautifulentropy commented Aug 25, 2025

Hi @jsha, I hope you’re doing well. I wanted to kindly follow up on this PR. @aarongable has already reviewed and approved the changes after the revisions, and I’d greatly appreciate it if you could take a look when you have a moment and provide the second review/approval so we can get this merged.

Thank you very much!

Sorry for the delay on this review. There are some merge conflicts that need to be addressed. Once you've fixed those we'll get this reviewed quickly. Thanks!

@akshithg akshithg force-pushed the test-measurements-on-main branch from 06f4e5f to a87b760 Compare August 25, 2025 21:57
@akshithg
Copy link
Copy Markdown
Contributor Author

Hi @beautifulentropy, thanks for the update!
I've fixed the merge conflicts, rebased, and squashed the commits. The PR should now be ready for review. Please let me know if there's anything else I should adjust.

@beautifulentropy beautifulentropy requested review from jprenken and removed request for jsha August 26, 2025 15:44
@beautifulentropy beautifulentropy changed the title Enable coverage reporting for Go unit & integration tests Test: Enable optional coverage reporting Aug 26, 2025
@beautifulentropy beautifulentropy changed the title Test: Enable optional coverage reporting test: Enable optional coverage reporting Aug 26, 2025
@jprenken jprenken merged commit 2b51075 into letsencrypt:main Aug 26, 2025
12 checks passed
@akshithg akshithg deleted the test-measurements-on-main branch November 20, 2025 05:48
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.

4 participants