test: Enable optional coverage reporting#8306
Conversation
aarongable
left a comment
There was a problem hiding this comment.
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.
|
Hi @aarongable, 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. |
|
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. |
|
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:
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. |
|
@aarongable I've incorporated all the changes you suggested. Can you please review? |
aarongable
left a comment
There was a problem hiding this comment.
LGTM! I've tested this and it seems to work nicely. I have a few small style comments below, but mostly this looks great!
|
@aarongable Thanks for reviewing and for your helpful suggestions. @jsha When you have a chance, could you review and approve? |
|
LGTM, needs a second review + approval to merge. |
|
Hi @jsha, 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! |
35508f3 to
06f4e5f
Compare
06f4e5f to
a87b760
Compare
|
Hi @beautifulentropy, thanks for the update! |
Add new flag
-c/--coverageto enable optional coverage reporting for all the test types.GOFLAGS="-cover -covermode=atomic -coverprofile=...for go unit tests-cover, as described in https://go.dev/blog/integration-test-coverage, for the end-to-end go integration tests