Refactor CA unittests to focus on top-level IssueCertificate

[aarongable]

Rewrite the vast majority of the CA unittests so that they test the exported functions (NewCertificateAuthorityImpl and IssueCertificate) rather than the non-exported helper functions (issuePrecertificate and issueCertificateForPrecertificate). This makes the tests more robust to future refactoring of the CA code itself, and will increase confidence in the correctness of those refactors.

Part of #8390

[jsha]

Generally looks good! A few small tweaks.

You say in the PR description the vast majority of tests now test IssueCertificate instead of the unexported functions. But I see some tests of the unexported functions do remain - is there a pattern to which ones had to stay?

[aarongable]

You say in the PR description the vast majority of tests now test IssueCertificate instead of the unexported functions. But I see some tests of the unexported functions do remain - is there a pattern to which ones had to stay?

None of the old tests had to stay. The pattern is that I left the ones which felt like they would provide a barrier against misuse of the helper functions in the brief period before the helpers are deleted. I'd be happy to delete those extra tests in this PR if you think it's appropriate to do so; it would make #8424 even cleaner.

[jsha]

I'm okay with landing this as-is. That said:

The pattern is that I left the ones which felt like they would provide a barrier against misuse of the helper functions in the brief period before the helpers are deleted. I'd be happy to delete those extra tests in this PR if you think it's appropriate to do so;

I think deleting the extra tests in this PR makes sense. We've replaced low-level tests with tests of the publicly visible functionality, which I think is generally good. The exception of course is when an internal-only function is factored out because it makes a particularly good target for narrowly targeted unittesting. But that doesn't apply here; these internal helper functions are a matter of RPC structure, not optimizing for testability.

There's a wacky world out there somewhere in which between this change and the subsequent change landing, someone tries to add another call to issueCertificateForPrecertificate somewhere bad.

Hopefully in such a world we'd also be adding new tests of the top-level functionality that suffice to cover the new calls. :D

[aarongable]

I think deleting the extra tests in this PR makes sense. We've replaced low-level tests with tests of the publicly visible functionality, which I think is generally good. The exception of course is when an internal-only function is factored out because it makes a particularly good target for narrowly targeted unittesting. But that doesn't apply here; these internal helper functions are a matter of RPC structure, not optimizing for testability.

Fair enough, done! I do think it is good to keep the follow-up PR wholly focused on code changes, not test changes. It makes this PR's diff slightly harder to review, but going through it commit-by-commit should still be easy enough.