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

Jump to bottom

proposal: testing: add a way to provide additional coverage information #30306

Closed
rogpeppe opened this issue Feb 18, 2019 · 9 comments
Closed

proposal: testing: add a way to provide additional coverage information #30306

rogpeppe opened this issue Feb 18, 2019 · 9 comments
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. Proposal WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided.
Milestone
Proposal

Comments

@rogpeppe
Copy link
Contributor

rogpeppe commented Feb 18, 2019 •
edited

The current test coverage mechanism works well when the test binary runs only once with all the tests running within that process, but sometimes it is useful to be able to run the test binary itself within the tests. For example, we may be testing a command line program that has global variables and we want to avoid inter-test pollution. The go tool uses this kind of approach, for example, by compiling itself, but it could potentially also use its own test binary.. Another example is when some code we might wish to exercise is in the form of a top level "main" function. For example, see analysis/singlechecker.

By default, although coverage information is extremely useful, it's not easy to obtain when a test suite runs the test binary as part of its tests. It is just about possible (this package does it), but only by relying on fragile parts of the system that may change in the future.

To solve this issue, I propose two new mechanisms:

  1. a way to tell the testing package to read a coverage file and add it to the total coverage
  2. a way to ask the testing package to write coverage report information even when M.Run is not called.

A possible API might look like this:

package testing

// AddCoverage adds coverage information from the given file to the total test coverage.
// The file must have been produced by the current test binary. This can
// be used to add additional coverage information written by M.WriteCoverage.
// This is a no-op if the binary was not built with coverage information enabled.
func AddCoverage(coverageFile string) error

// WriteCoverage writes coverage information to the given file.
// This is usually done automatically, but is useful if the test
// binary is re-executed as part of testing.
// This is a no-op if the binary was not built with coverage information enabled.
func (m *testing.M) WriteCoverage(file string) error

See also issue #30231
@gopherbot gopherbot added this to the Proposal milestone Feb 18, 2019
@mvdan
Copy link
Member

mvdan commented Feb 19, 2019

/cc @alandonovan

@alandonovan
Copy link
Contributor

alandonovan commented Feb 19, 2019 •
edited

I would prefer to decouple this mechanism from the testing package, because the need to call WriteCoverage explicitly seems to arise particularly within standalone programs used as part of a test that are not the test executable itself. With the Go build system, one tends to fork/exec os.Args[0] using the same binary but a different entry point, but with Bazel/Blaze, it's easy for one program (the test) to depend on a completely different program (the helper), where the code subject to coverage is in the helper, and the helper doesn't depend on testing. Perhaps we split testing/cover.go into a separate "cover" package and expose its Register and Report functions?

FWIW, Blaze provides coverage through a different pipeline and file format (LCOV). The structure is similar but the details vary. There's a package called coverdata that lives at the very bottom of the dependency graph, and which acts as a register of all coverage counters, and it exposes functions to register counters for a file and to write a report. There are on the order of a dozen calls to the report function from helper programs. (Many of those are made indirectly through a mechanism for deferred cleanup calls similar to C's atexit(3).) The implementation of WriteReport for Go in Blaze would have to be patched to write in the LCOV format. The need for the patch is no worse that what we have today, but it would at least standardize for users the API by which a helper flushes its coverage report.

@bcmills, @jayconrod to ensure that whatever API we come up with works for Go, Bazel, and Blaze.

@josharian
Copy link
Contributor

Depending on the details, go-fuzz could also benefit from standardization. It currently mimics Go, with some minor incompatibilities. (I am unsure offhand whether those incompatibilities are bugs or due to the fact that go-fuzz gathers some expression-level coverage and Go is statement-level only.)

@rogpeppe
Copy link
Contributor Author

@alandonovan I agree in principle, but care will be needed to avoid accidentally mixing incompatible coverage reports. I think it's probably OK if we're using modules and all the modules used to build the binary have the same versions as the ones in the parent, but currently the coverage file doesn't provide any way of verifying this.

The way that Blaze does it sounds like a decent approach - could we change Go's stdlib support to be a bit more like that, or would that founder on backward-compatibility issues?

@alandonovan
Copy link
Contributor

What I'm proposing is essentially that testing's coverage machinery be split into another package on which it depends; the existing API can remain and delegate. The new cover package would expose a CoverReport function; all other things that it exposes would be excluded from the API stability guarantee, just as they are excluded today in the testing package. This would provides a standard way to flush a coverage report, whether or not an application depends on testing. I don't see any obvious compatibility problems other than those of depending on new function from the standard library.

What do you mean by incompatible coverage reports? I would expect the cover-transformed source files to ephemeral, never checked in, so the cover tool and the testing (or cover) packages can always assume they are at the same Go version.

@bcmills bcmills added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Feb 28, 2019
@ianlancetaylor ianlancetaylor mentioned this issue Mar 27, 2019
Open
@andybons
Copy link
Member

@rogpeppe any thoughts on @alandonovan's question?

@andybons andybons added the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label May 14, 2019
@andybons
Copy link
Member

Ping @rogpeppe

@gopherbot
Copy link

Timed out in state WaitingForInfo. Closing.

(I am just a bot, though. Please speak up if this is a mistake or you have the requested information.)

@mvdan
Copy link
Member

mvdan commented Jun 23, 2019

I think this fell under @rogpeppe's radar as he's recently switched jobs and participated in larger proposals, like the error ones. Hopefully he can chime in at some point soon and we can reopen this issue.

@golang golang locked and limited conversation to collaborators Jun 22, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. Proposal WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided.
Projects
None yet
Milestone
Proposal
Development

No branches or pull requests
7 participants
@rogpeppe @josharian @andybons @mvdan @bcmills @alandonovan @gopherbot