It's not possible to generate coverage for testonly libraries.

[alxn]

What version of rules_go are you using?

v0.24.9

What version of gazelle are you using?

bazel-contrib/bazel-gazelle@9c04579

What version of Bazel are you using?

3.7.0

Does this issue reproduce with the latest releases of all the above?

Yes.

What operating system and processor architecture are you using?

OS-X Catalina / Debian Buster, x86_64.

Any other potentially useful information about your toolchain?

What did you do?

bazel coverage :go_default_test for a go_library marked with testonly = True.

What did you expect to see?

Coverage file go_default_test/coverage.dat generated.

What did you see instead?

Test pass but no coverage file was generated.

Discussion

This is related to the discussion in bazelbuild/bazel#12176, especially bazelbuild/bazel#12176 (comment):

I wonder if another option for this info is to use a features = ["test_target"] (or whatever string)? The interesting thing about testonly is you could have testonly targets that you still want coverage info for. In our case this is running the unit tests against a "test helpers" type module. The "test helpers" module is testonly = True, but we still want to see coverage for it when running its own unit tests. In this case we don't want to change the semantics by flipping it to testonly = False.

This is exactly the issue we are running into. We had noticed test helper code was slipping into production code, including some references to testing.T. We refactored and marked these libraries as testonly. Only afterwards did we notice that coverage was not being generated.

I've got a PR for this fix, which we are testing locally in our mono-repo, but I wonder if it won't be acceptable to turn coverage on for any testonly package, rather we need some WORKSPACE option to enable this.

However, I suspect there is a high correlation between those who have the due diligence to mark packages as testonly, and those who want coverage for these packages.

[achew22]

However, I suspect there is a high correlation between those who have the due diligence to mark packages as testonly, and those who want coverage for these packages.

I suspect this to be true as well.

I think a good next step would be to see the concrete details of this is by uploading it as a PR. That way we can patch it into a few projects and see the implications. WDYT?

[alxn]

I've got it patched into a fairly enormous go mono repo. Tests still passed.

[jayconrod]

Walking back through the repo history, it looks like the exclusion of testonly code was added a few years ago in #1131. I have no recollection of why that was. The Bazel documentation for testonly doesn't say anything about coverage, and Go doesn't have a concept of testonly, so I don't know of any reason why testonly libraries shouldn't be instrumented.

Is there precedent for this in other languages? If other languages instrument testonly code, I'm fine with merging #2787.

[alxn]

I've spent > 30 minutes grokking bazelbuild+testonly issues, but nothing that interesting (other than bazel-contrib/bazel-gazelle#513).

FWIW I'll get the patch into our mono repo this evening, and see how it soaks.

[jayconrod]

Just curious: why is it useful to have coverage info for testonly packages? I think of testonly as being things that only make sense in tests like fakes or mocking libraries, not just packages that are used by tests but not in production code. If I were testing with a fake database for example, I'd actually prefer not to have coverage for that package.

You mentioned bazelbuild/bazel#12176, but that seems like a limitation of ios_unit_test that doesn't apply to go_test, since normally test code goes in the go_test itself.

[achew22]

I can provide an example of this. I used to work on a database. We had an in memory implementation of our server (with a convenient way to get an object that implemented the client interface). That object was testonly, but was also a part of our contract to consumers. Therefore we wanted to validate that it had high levels of test coverage so people wouldn't be surprised by silly mistakes.

[rabbbit]

+1, we have tests for our test_utils.

…

On Fri, 15 Jan 2021 at 12:43, Andrew Z Allen ***@***.***> wrote: I can provide an example of this. I used to work on a database. We had an in memory implementation of our server (with a convenient way to get an object that implemented the client interface). That object was testonly, but was also a part of our contract to consumers. Therefore we wanted to validate that it had high levels of test coverage so people wouldn't be surprised by silly mistakes. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#2786 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACG3UU2LZI6ZFZGXBQIVSLS2BWCJANCNFSM4WCWN5AA> .

[alxn]

We have a fake implementation of Zookeeper used to trigger race conditions that could occur in production. It's complex enough that it needs its own tests.

I think I would also emphasize that all code needs to be production quality code. We code review tests for their quality, as well as the code they are actually testing. Specific bad examples are line-hitter tests, that get 100% coverage, but don't validate a single condition.

If you are providing a fake for someone to validate their production code, that fake needs to be robust, otherwise you are not building that production code on a good foundation.

[jayconrod]

Makes sense, thanks all for good examples. I'll merge #2787.

A workaround for this would be to define two go_library targets built from the same sources: one with testonly = True for users to consume and one without. Maybe that's more of a hassle than it's worth though, since it won't work with tools like Gazelle.

In any case, it should be possible to gather coverage data for testonly libraries, and it's currently not. People can still control which packages are covered and opt out with --instrumentation_filter.

[alxn]

We also have wrappers on top of the build to enforce coverage limits. The just enables the ability to generate coverage. Not the requirement nor enforcement.

[alxn]

As a final comment, I present "testing"

• https://golang.org/pkg/testing/
• https://github.com/golang/go/tree/master/src/testing

With somewhat extensive tests.