Venus: Report test coverage metrics in CI builds

[anorth]

Description

I propose we collect test coverage metrics in the CI builds for PRs, initially on an informational basis. This information will help contributors and reviewers understand the test coverage impact of changes, and trigger requests for more thorough tests. With this information, we can progressively raise our test coverage and sophistication.

The project had code coverage metrics as a merge-gating check, which was removed (#1575) not so long ago because it was frequently overridden. At the time, code structure made testing some parts very difficult, a significant amount of which has since been improved. This proposal is not for a strict check, but calls on committers to use their judgement. When code structure makes testing hard, I want us to know about it so that we can fix it. Our current statement-level coverage is about 56%.

Filecoin is a security-critical project, with potentially huge financial stakes involved. There are many aspects to security of the network and we need defense in depth. The layer that go-filecoin most directly controls is the accuracy and robustness of our implementation. It is unimaginable to me that we could be confident launching mainnet without comprehensive test coverage. That will involve a lot of discipline and work, and we should start now.

Acceptance criteria

• Code coverage metrics are automatically available for every PR
• Committers are aware of their responsibility to inspect a PRs test impact and request more/better tests when appropriate

Risks + pitfalls

• Coverage metrics may slow down CI builds

Where to begin

• Reverse some of the steps in #1575

[anorth]

FYI @acruikshank @ognots @travisperson @mishmosh

[acruikshank]

I agree that throrough testing needs to be a high priority for this project and that code coverage statistics are an important tool to advance this goal. Previous experience with code coverage statistics on this project suggests that we should be careful about how we interpret the statistics, because the language and the nature of this project tends to under report coverage. That said we should always try to move the coverage numbers up and it's hard to do that without statistics.

[anorth]

The numbers we're getting are clearly not right. Something about circle and codecov isn't working for us.

I attempted to fix by removing the container parallelism on the unit and integration tests. I don't think we're getting much out of parallelism in either case. The unit tests only take 2min total, and the integration tests all end up on a single container because they're all in the commands package. This failed for mysterious reasons https://github.com/filecoin-project/go-filecoin/issues/3071

[anorth]

@ognots to simplify the data we're passing to codecov, could we disable the coverage upload for functional tests? I'm considering disabling it for integration tests too. I couldn't quickly see how to structure the circle.yml file to achieve that. An additional parameter would be appropriate.

[anorth]

FYI https://github.com/filecoin-project/go-filecoin/pull/3069

[ognots]

the primary blocker to removing parallelism and test splitting is the unit tests OOM.
I attempted doing so in https://github.com/filecoin-project/go-filecoin/pull/3149/commits/fc1b33794775ca03a3d8d8ade1e1eb27335bf5b9 which consistently results in OOMs.
It's the same reason the unit tests failed in #3069
My hypothesis is that the go test automatic parallelism (enabled by removing -parallel flag) functionality is based on number of CPU cores, but our tests our memory bound, which results in the OOMs. We used to use an Xlarge CircleCI container to get around this, but that was causing outside contributor PRs to fail because of the same OOM problem detailed above.
The hypothesis I have about non-deterministic results while using test splitting and parallelism in CircleCI is that the test splitting is currently non-deterministic, so code coverage results could vary depending on what combination of tests are being run together.
I believe there is a way to be more deterministic in test splitting, so going to investigate and test the hypothesis.

[Kubuxu]

Hi, first of all: are you using -covermode=atomic it is slower but correct in a concurrent environment.
Secondly: you can upload as many reports to Codecov as you want (if you are generating coverage in multiple containers). You just have to configure it to wait for N raports.

[anorth]

@Kubuxu can you point to any docs about that configuration? We have been uploading reports from multiple containers, but the resulting reports are wrong.

[Kubuxu]

In go-ipfs we are using https://github.com/ipfs/go-ipfs/blob/master/codecov.yml#L7-L9
https://docs.codecov.io/docs/notifications#section-preventing-notifications-until-after-n-builds

-covermode=atomic is more important AFAIK, also you should probably manually limit GOMAXPROCS to the container limit as IIRC, on Circle it is detected wrong.

[ognots]

I was still having mixed results with some of the suggestions from Kubuxu, but within 1% margin only. Before saw difference of up to 20%
Was trying to only run codecov on unit tests but it was being finicky and ran out of time.
PR with experimental changes https://github.com/filecoin-project/go-filecoin/pull/3149#issuecomment-517388072

[Kubuxu]

I've observed about 1% to be the norm in go-ipfs. It depends on which codepath gets taken in cases of for example context cancel.

[anorth]

I would be easily convinced we have some nondeterminism that could account for ~1% noise, which we can resolve subsequently. Thanks @ognots - could you prepare your PR for review?