Add initial coverage support to neotest

[Slava0135]

Requires #3460

Problem

Neotest doesn't provide coverage collection support when running contract tests

Solution

Executed opcodes collected by using OnExecHook for each script.
Then, using DebugSeqPoints, coverage for source file is evaluated and saved in file after each test.

Current implementation has some quirks with it:

• File is written during each test cleanup, which can be heavy / slow down test execution significantly
• In order to save coverage, coverProfile flag is reset, so go tooling doesn't overwrite coverage file. This can be a problem when running both contract and non-contract tests in the same project.
• Scripts from dependencies are added to coverage file too, which is overkill.
• Only set mode is supported.

[Slava0135]

Showcase (using https://github.com/nspcc-dev/neofs-contract)

coverage3.mp4

It seems like some coverage data is lost when running all package tests at once, need more testing...

[Slava0135]

Forgot to add reporting if script was already compiled. It does fix problem in the test file on video, but for some reason in other files almost all coverage data is lost.

[Slava0135]

Fixed the issue.

Also the question is, where do we need add the coverage instrumentation? Currently, its only in TestInvoke() method, but there are other methods where VM is run.

[Slava0135]

rebased branch on master

[Slava0135]

It seems to work, but I found out that when running this test, coverage profile for ListContainerSizes will be empty, even though it was clearly run inside the test.

Need to write unit tests for this feature somehow, to make sure it works as expected.

[AnnaShaleva]

Please, consider reformatting commit structure according to the https://github.com/nspcc-dev/.github/blob/master/git.md#logical-separation.

A really nice feature.

[AnnaShaleva]

Need to write unit tests for this feature somehow

That would be nice.

[codecov]

Codecov Report

Attention: Patch coverage is 26.40000% with 92 lines in your changes missing coverage. Please review.

Project coverage is 85.86%. Comparing base (7766168) to head (d0c4547).
Report is 8 commits behind head on master.

Files	Patch %	Lines
pkg/neotest/coverage.go	18.08%	75 Missing and 2 partials ⚠️
pkg/neotest/basic.go	42.10%	9 Missing and 2 partials ⚠️
pkg/neotest/client.go	0.00%	2 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3462      +/-   ##
==========================================
- Coverage   86.05%   85.86%   -0.20%     
==========================================
  Files         330      331       +1     
  Lines       38692    38806     +114     
==========================================
+ Hits        33297    33319      +22     
- Misses       3849     3934      +85     
- Partials     1546     1553       +7     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[Slava0135]

Before I fix anything, we need to agree on some details:

1. Collect coverage explicitly (context) or implicitly (when compiling), I picked second option because I though people wouldn't bother to enable coverage manually for every test. However, when writing tests for contracts, you have to do quite a lot of setup already.
2. Enable coverage using external tool (sh + bat / python?) or using go test cleanup + unsetting flag, I picked second option because it requires no effort from user, but this relies on some assumptions about Go test infrastructure that may be changed in future. For first option you would need to put custom PATH for go when creating project. VSCode will probably handle it fine, not sure about GoLand and vim/neovim etc.

[AnnaShaleva]

1. Collect coverage explicitly (context) or implicitly (when compiling)

As I said in review, to me the first option is more preferable because it's explicit, i.e. allows users of neotest to pick those tests that will affect coverage. I think that sometimes there might be a situation when you need to exclude some test from the coverage if this test is designated for some other purpose. For example, our TestCreateBasicChain should not affect the coverage of our test contracts. Also, with this approach an explicit context is passed to the user of neotest so that it's clear that coverage is being collected (or not) for this test. And finally, implicit coverage collection bothers compilation step, so this step is actually not a compilation step anymore. But I'd like to hear @fyfyrchik and @roman-khimov opinion on this topic.

2. Enable coverage using external tool (sh + bat / python?) or using go test cleanup + unsetting flag

To me the second approach also looks better because cleanups are native for Go, it's easy-to-go way, and I don't feel negative about the unsetting flag "hack".

but this relies on some assumptions about Go test infrastructure that may be changed in future

I think we'll be able to modify coverage module accordingly if something is changed. And for now this approach works fine.

[roman-khimov]

Collect coverage explicitly (context) or implicitly (when compiling)

Some explicit controls are needed, but I'd default them to "enabled". Most of the time we need it when doing things with neotest. But sometimes we don't care or explicitly need it to be disabled.

Enable coverage using external tool (sh + bat / python?) or using go test cleanup + unsetting flag

Cleanups are OK.

[Slava0135]

Collect coverage explicitly (context) or implicitly (when compiling)

Some explicit controls are needed, but I'd default them to "enabled". Most of the time we need it when doing things with neotest. But sometimes we don't care or explicitly need it to be disabled.

In the case we do this by adding optional argument (via struct reference) to neotest's CompileFile / CompileSource? CompileSource already accepts compiler.opts (but not CompileFile). Though it is just forwarded to compiler itself, so maybe its not the place to add new flag for coverage.

[roman-khimov]

I've looked at the code around once more and what really bugs me is that all things are currently package-level (which was noted previously by @fyfyrchik and @AnnaShaleva), so to have some control over this functionality we'll inevitably make some package-level neotest.EnableCoverage() and neotest.DisableCoverage() and while this would work for a single test it can be problematic for a set of tests in the same package. So my proposal here would be:

1. Extend Contract with debug info.
2. Make coverage state per-Executor, not per-package, initialize in NewExecutor, provide Executor.DisableCoverage() and Executor.EnableCoverage().
3. Set tracked contracts in DeployContract*, not in Compile* (if coverage is enabled).
4. Wrap neotest.TestInvoke with Executor.TestInvoke (hooking into the VM as needed)
5. Use this TestInvoke where appropriate (state-changing transactions should go through some VM with coverage enabled).
6. Document all of the details of different functions involved.

An alternative could be tying coverage hooks to Blockchain (which can allow to track real on-chain invocations, btw), but it's a bit too invasive for now. Adding flags to Compile* and/or TestInvoke isn't elegant either. Global package-level setters can lead to unexpected results. Also, from what I've seen nothing uses TestInvoke() directly.

@AnnaShaleva?

[AnnaShaleva]

Agree with the proposed plan, especially with the second point, since it's good to have an explicit coverage enabling/disabling functionality.

[Slava0135]

It seems to work, but I found out that when running this test, coverage profile for ListContainerSizes will be empty, even though it was clearly run inside the test.

just needed to add coverage hook into TestInvoke* functions from client.go

[Slava0135]

1. Extend `Contract` with debug info.

2. Make coverage state per-Executor, not per-package, initialize in `NewExecutor`, provide `Executor.DisableCoverage()` and `Executor.EnableCoverage()`.

3. Set tracked contracts in `DeployContract*`, not in `Compile*` (if coverage is enabled).

4. Wrap `neotest.TestInvoke` with `Executor.TestInvoke` (hooking into the VM as needed)

5. Use this `TestInvoke` where appropriate (state-changing transactions should go through some VM with coverage enabled).

6. Document all of the details of different functions involved.

1. Done
2. Done? You want to store executed Ops per executor (and then merge them somehow?)
3. Done
4. There is problem, AddSystemFee uses TestInvoke but doesn't provide executor
5. ^^^
6. Later, when implementation will be ready

[Slava0135]

Also, these changes improved performance, because coverage is reported only once per contract deployed (before, it was per Invoke call). So I think using t.Cleanup() is fine.

[AnnaShaleva]

@Slava0135, the current implementation is very close to the desired one. Also please, consider reformatting/reordering commits according to https://github.com/nspcc-dev/.github/blob/master/git.md#commits.

Done? You want to store executed Ops per executor (and then merge them somehow?)

It's done in the current implementation. I think we don't need per-executor Ops, we're fine with per-executor collectCoverage variable. @roman-khimov, right?

There is problem, AddSystemFee uses TestInvoke but doesn't provide executor

Just provide it to AddSystemFee, nothing prevents you from improving this API (but please, make it in a separate commit since it's a separate logical change):

@@ -298,12 +298,12 @@ func NewDeployTxBy(t testing.TB, bc *core.Blockchain, signer Signer, c *Contract
 
 // AddSystemFee adds system fee to the transaction. If negative value specified,
 // then system fee is defined by test invocation.
-func AddSystemFee(bc *core.Blockchain, tx *transaction.Transaction, sysFee int64) {
+func AddSystemFee(e *Executor, tx *transaction.Transaction, sysFee int64) {
        if sysFee >= 0 {
                tx.SystemFee = sysFee
                return
        }
-       v, _ := TestInvoke(bc, tx) // ignore error to support failing transactions
+       v, _ := TestInvoke(e, tx) // ignore error to support failing transactions
        tx.SystemFee = v.GasConsumed()
 }
@@ -395,7 +395,8 @@ func (e *Executor) AddBlockCheckHalt(t testing.TB, txs ...*transaction.Transacti
 }
 
 // TestInvoke creates a test VM with a dummy block and executes a transaction in it.
-func TestInvoke(bc *core.Blockchain, tx *transaction.Transaction) (*vm.VM, error) {
+func TestInvoke(e *Executor, tx *transaction.Transaction) (*vm.VM, error) {
+       bc := e.Chain
        lastBlock, err := bc.GetBlock(bc.GetHeaderHash(bc.BlockHeight()))
        if err != nil {
                return nil, err
@@ -412,8 +413,8 @@ func TestInvoke(bc *core.Blockchain, tx *transaction.Transaction) (*vm.VM, error
        ttx := *tx
        ic, _ := bc.GetTestVM(trigger.Application, &ttx, b)
 
-       if isCoverageEnabled() {
-               ic.VM.SetOnExecHook(coverageHook())
+       if e.collectCoverage {
+               ic.VM.SetOnExecHook(coverageHook)
        }
 
        defer ic.Finalize()

[Slava0135]

Some thoughts on parallel execution:

We need mutex for:

1. isCoverageEnabled
2. reportCoverage
3. rawCoverage when adding new scripts in Deploy*

But also we need mutex for coverageHook, because rawCoverage is modified there. But this will (probably) make execution slower, so maybe we should leave coverageHook a function that accepts Executor in which OPs are collected and stored, then merged in reportCoverage. Unless tests can call Executor in parallel.

[Slava0135]

Just provide it to AddSystemFee, nothing prevents you from improving this API (but please, make it in a separate commit since it's a separate logical change)

So we are making it breaking change?

[Slava0135]

For some reason collecting coverage doesn't work for _deploy even if I add hook into AddNetworkFee which is called by NewDeployTxBy.

[AnnaShaleva]

But this will (probably) make execution slower

Only if coverage enabled.

leave coverageHook a function that accepts Executor in which OPs are collected and stored, then merged in reportCoverage.

This approach binds OPs to executor, as a result we're going to have per-Executor rawCoverage field. It's similar to the approach that was discussed earlier, but the problem is that we still can't remove global rawCoverage variable because we need to merge the results of all tests and store them somewhere. I consider we still have to introduce mutex for this global to hold it during processCover, but as an optimisation of coverageHook the proposed option can be implemented.

So we are making it breaking change?

Yes, neotest is developing and I don't see anything critical in this. The migration is rather trivial for the users.

For some reason collecting coverage doesn't work for _deploy even if I add hook into AddNetworkFee which is called by NewDeployTxBy.

It happens because NewDeployTxBy does not calculate system fee for the deployment transaction, and hence, no test invocation is being performed for deploying script. NewDeployTxBy uses a fixed system fee for deploy transaction:

neo-go/pkg/neotest/basic.go

Line 276 in ffcbe6a

tx := transaction.New(script, 100*native.GASFactor)

I think we should change this behaviour and evaluate system fee by test invocation, exactly like it's done for ordinary transactions. Note, you should use AddSystemFee, not AddNetworkFee.

[Slava0135]

Should be fixed. (for some reason flag.VisitAll included coverProfile flag with value "", even when coverage is not enabled in go test)

[Slava0135]

It seems like there are issues with CI coverage, because neotest replaces original coverage data.

[Slava0135]

maybe we should use some ENV variable that disables neotest coverage and use it in CI

[Slava0135]

maybe we should use some ENV variable that disables neotest coverage and use it in CI

or having a flag would be better?

[AnnaShaleva]

It seems like there are issues with CI coverage, because neotest replaces original coverage data.

Yep, and we definitely need some way to store the original coverage reports.

maybe we should use some ENV variable that disables neotest coverage and use it in CI

Let's do this. I firstly thought about storing coverage results in a separate file, but overwriting is not the only problem, because we need to actually gather normal coverage for these packages, and it's turned off while gathering neotest coverage.

[AnnaShaleva]

or having a flag would be better?

The problem with flag is that we can't directly integrate it into go test, thus let's firstly try with env var, it should be sufficient.

[Slava0135]

Naming gets really bad with coverage package, we now have 3 different flags that enable/disable coverage coverageEnabled, isDisabled + collectCoverage in Executor.

[Slava0135]

I added env variable, it works locally when running tests - i think it should work in workflow too.

[Slava0135]

Are we good with "inverse" env variables? Right now if you set DISABLE_NEOTEST_COVER=0 it will enable coverage which might not be perfect.

[AnnaShaleva]

Naming gets really bad with coverage package, we now have 3 different flags that enable/disable coverage coverageEnabled, isDisabled + collectCoverage in Executor.

The global coverageEnabled is still the only source of truth. The only purpose of DISABLE_NEOTEST_COVERAGE is to turn coverageEnabled off. And collectCoverage is a different per-executor setting which serves its own purpose and should be preserved. We just need all these things to be documented properly.

Are we good with "inverse" env variables?

I'm good with inverse meaning, because usually smart-contracts are a separate go package (or even a separate project) that contains nothing than contracts and thus, supposed to be tested not in a standard Go way. But may be @roman-khimov or @fyfyrchik have different opinion. For now I'd suggest to keep it inverted.

Right now if you set DISABLE_NEOTEST_COVER=0 it will enable coverage which might not be perfect.

It's not quite correct, the implementation should be adjusted, take a look at the #3462 (comment).

[Slava0135]

ok, i disabled coverage in makefile for test and cover, maybe add new target for contract coverage?

[AnnaShaleva]

maybe add new target for contract coverage?

All contracts in NeoGo serve testing needs, I doubt we need coverage for them. It may be enabled for example contracts, but only neo-go/examples/nft-nd-nns has proper tests. Let's do this in a separate PR.

What really deserves a separate coverage target is neofs-contracts repo. Please, take a look at nspcc-dev/neofs-contract#429.

[AnnaShaleva]

@Slava0135, also ref. #3558 and #3559 for further work, if you'd like to.

[AnnaShaleva]

Otherwise LGTM. May be checked with the help of nspcc-dev/neofs-contract#429.

[AnnaShaleva]

@Slava0135, could you please update the Makefile (#3462 (comment))? It's the only conversation left from my side, and after that we can merge if there are no more comments from @roman-khimov or @fyfyrchik.

[Slava0135]

@Slava0135, could you please update the Makefile (#3462 (comment))? It's the only conversation left from my side, and after that we can merge if there are no more comments from @roman-khimov or @fyfyrchik.

done

[fyfyrchik]

Oh my god, I am so happy it is finally in the main repo and can be used.