Fix coverage blocks overlapping #3575

Slava0135 · 2024-09-06T16:55:51Z

Problem

Resolves #3559.

Solution

Remove block if it includes any other block.

codecov · 2024-09-10T07:20:00Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 85.35%. Comparing base (d9a6a7c) to head (794f204).
Report is 15 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3575      +/-   ##
==========================================
- Coverage   85.79%   85.35%   -0.45%     
==========================================
  Files         330      333       +3     
  Lines       38536    39006     +470     
==========================================
+ Hits        33062    33292     +230     
- Misses       3928     4144     +216     
- Partials     1546     1570      +24

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

pkg/neotest/coverage.go

Slava0135 · 2024-09-12T08:03:38Z

Fixed issues and made it a single commit

AnnaShaleva · 2024-09-13T11:57:40Z

pkg/neotest/coverage_test.go

+)
+
+func resetCoverage() {
+	rawCoverage = make(map[util.Uint160]*scriptRawCoverage)


This map is protected by mutex, so at least we need to use mutex. Also, these tests prohibit NeoGo from using contract coverage in the project; we either need to document this or to change the test structure to skip them if contract coverage is enabled.

We could make new map, save old map and then restore original map in Cleanup

It won’t work if tests run in parallel.

pkg/neotest/coverage.go

AnnaShaleva · 2024-09-13T12:26:35Z

pkg/neotest/coverage.go

+				continue
+			}
+			// outer interval start must be before inner interval start.
+			if !(outer.StartLine < inner.StartLine || outer.StartLine == inner.StartLine && outer.StartCol <= inner.StartCol) {


The problem with this algorithm is that we can't just remove all outer intervals, otherwise coverage for some parts of code is lost and the result looks strange a bit. Consider neofs-contracts as an example, here's the result with the old implementation:

And here's the result with the overlaps removal:

return runtime.CheckWitness( is an outer interval and it's just removed.

Also, I'm not quite sure why the first call to runtime.CheckWitness is marked as not covered, although it should be.

Also, I'm not quite sure why the first call to runtime.CheckWitness is marked as not covered, although it should be.

Pretty sure, its because there is no SeqPoint for it

I have checked how Go itself handles statement coverage and it seems like it takes a line as a whole (except for conditions where paths can diverge, though still counts for outer function if inner function panics). So we could keep outer block if its one the same line as inner (both are one-liners), but remove outer block if its multi-line?

Could you give an example of contract with test where coverage blocks are overlapping?

PutNamed function in container contract.

I'm finally back to this issue, let's locate the problem more precisely: consider PutNamed function of container contract and the coverage generated by master of NeoGo. I got this html coverage for the function:

In the resulting coverage profile I got several batches of records for container contract:
coverage.txt
The resulting blocks look like:

github.com/nspcc-dev/neofs-contract/contracts/container/contract.go:90.65,91.2 0 0 github.com/nspcc-dev/neofs-contract/contracts/container/contract.go:94.39,97.14 3 0 github.com/nspcc-dev/neofs-contract/contracts/container/contract.go:97.14,102.25 4 0 github.com/nspcc-dev/neofs-contract/contracts/container/contract.go:102.25,109.40 2 0 github.com/nspcc-dev/neofs-contract/contracts/container/contract.go:109.40,112.5 2 0 github.com/nspcc-dev/neofs-contract/contracts/container/contract.go:115.4,115.64 1 0 github.com/nspcc-dev/neofs-contract/contracts/container/contract.go:115.64,118.5 2 0 github.com/nspcc-dev/neofs-contract/contracts/container/contract.go:125.3,125.23 1 0 github.com/nspcc-dev/neofs-contract/contracts/container/contract.go:125.23,127.4 1 0 github.com/nspcc-dev/neofs-contract/contracts/container/contract.go:129.3,129.9 1 0 github.com/nspcc-dev/neofs-contract/contracts/container/contract.go:132.2,143.76 2 0 github.com/nspcc-dev/neofs-contract/contracts/container/contract.go:143.76,145.3 1 0 github.com/nspcc-dev/neofs-contract/contracts/container/contract.go:145.8,147.3 1 0 github.com/nspcc-dev/neofs-contract/contracts/container/contract.go:149.2,149.76 1 0 github.com/nspcc-dev/neofs-contract/contracts/container/contract.go:149.76,151.3 1 0 github.com/nspcc-dev/neofs-contract/contracts/container/contract.go:151.8,153.3 1 0 github.com/nspcc-dev/neofs-contract/contracts/container/contract.go:155.2,155.76 1 0 github.com/nspcc-dev/neofs-contract/contracts/container/contract.go:155.76,157.3 1 0 github.com/nspcc-dev/neofs-contract/contracts/container/contract.go:157.8,159.3 1 0

There are some duplicating records, in particular for PutNamed, but I suppose it's OK since multiple tests call PutNamed method.

Every record has the form of: filename:StartLine.StartCol,EndLine.EndCol NumOfStmts ExecutionCnt. Let's take the following record as an example:

github.com/nspcc-dev/neofs-contract/contracts/container/contract.go:254.21,259.87 4 0

This record covers the following selected contract statements:

First thing I should notice is that every record has 0 number of executions. It seems like a bug to me, but it's a separate issue that may be resolved in another PR.

Secondly, this record covers multiple statements (both in terms of code and in terms of VM instructions. Please, clarify is this a problem we're trying to solve? Do you think that coverage tool should produce a single record per a single code statement or per a single line of code?

If going further, the next coverage line is:

github.com/nspcc-dev/neofs-contract/contracts/container/contract.go:259.87,260.26 1 0

Which matches the following selected part of code:

So this record is not intersected with the first one.

Could you please give the exact example of two pverlapping coverage records and the expected (correct) result for them?

In vscode you can see that panic is not executed, but its "shadowed" by outside block. But in html it seems to be all green, which is worse.

Attach VSCode screen, please. And submit the desired output for these coverage records.

Slava0135 · 2024-09-14T08:19:04Z

In container.go many statements (functions calls) are not tracked. Probably would need to generate more SeqPoints for these.

Signed-off-by: Slava0135 <[email protected]>

AnnaShaleva · 2024-09-14T15:53:57Z

Probably would need to generate more SeqPoints for these.

We firstly need to check the debug info for this contract, and if debug info is really missing these sequence points then it’s a separate issue.

Slava0135 · 2024-10-07T17:12:24Z

I am talking about different panic statement, actually

Slava0135 · 2024-10-07T17:15:38Z

... but its all green in html

Slava0135 · 2024-10-07T17:16:04Z

In container.go many statements (functions calls) are not tracked. Probably would need to generate more SeqPoints for these.

and this is how it looks with fix

AnnaShaleva · 2024-10-07T17:36:52Z

and this is how it looks with fix

The fix presented in the PR does not extend the list of sequence points, it just tries to remove some of outer intervals and it does not work in some cases. That’s why I’m trying to understand what is the desired result. For the PutNamed method could you attach the desired list of coverage records?

AnnaShaleva · 2024-10-17T16:36:22Z

As discussed in #3559 (comment), this solution is not a good fix. It helps to remove overlapping blocks, but in improper way. We need to fix the problem itself, not its consequences.

I'm closing this PR in favour of #3622 and some better solution for remaining overlap problems described in #3559 (comment).

Slava0135 requested review from AnnaShaleva and roman-khimov as code owners September 6, 2024 16:55

AnnaShaleva reviewed Sep 12, 2024

View reviewed changes

pkg/neotest/coverage.go Outdated Show resolved Hide resolved

pkg/neotest/coverage.go Outdated Show resolved Hide resolved

Slava0135 force-pushed the fix-coverage-overlap branch from 71bd0be to 7729e65 Compare September 12, 2024 08:02

Slava0135 force-pushed the fix-coverage-overlap branch from 7729e65 to 794f204 Compare September 12, 2024 08:08

AnnaShaleva reviewed Sep 13, 2024

View reviewed changes

neotest: fix coverage blocks overlapping

650f9a3

Signed-off-by: Slava0135 <[email protected]>

Slava0135 force-pushed the fix-coverage-overlap branch from 794f204 to 650f9a3 Compare September 14, 2024 08:22

AnnaShaleva mentioned this pull request Oct 15, 2024

Neotest coverage blocks are overlapping sometimes #3559

Open

Slava0135 mentioned this pull request Oct 16, 2024

neotest: exclude RET statements from coverage #3617

Closed

AnnaShaleva closed this Oct 17, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix coverage blocks overlapping #3575

Fix coverage blocks overlapping #3575

Slava0135 commented Sep 6, 2024

codecov bot commented Sep 10, 2024 •

edited

Loading

Slava0135 commented Sep 12, 2024

AnnaShaleva Sep 13, 2024

Slava0135 Sep 14, 2024

AnnaShaleva Sep 14, 2024

AnnaShaleva Sep 13, 2024

AnnaShaleva Sep 13, 2024

Slava0135 Sep 14, 2024

Slava0135 Sep 14, 2024 •

edited

Loading

AnnaShaleva Sep 14, 2024 •

edited

Loading

Slava0135 Sep 14, 2024

AnnaShaleva Oct 7, 2024 •

edited

Loading

Slava0135 Oct 7, 2024

AnnaShaleva Oct 7, 2024 •

edited

Loading

Slava0135 commented Sep 14, 2024

AnnaShaleva commented Sep 14, 2024

Slava0135 commented Oct 7, 2024

Slava0135 commented Oct 7, 2024

Slava0135 commented Oct 7, 2024

AnnaShaleva commented Oct 7, 2024

AnnaShaleva commented Oct 17, 2024

Fix coverage blocks overlapping #3575

Fix coverage blocks overlapping #3575

Conversation

Slava0135 commented Sep 6, 2024

Problem

Solution

codecov bot commented Sep 10, 2024 • edited Loading

Codecov Report

Slava0135 commented Sep 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Slava0135 Sep 14, 2024 • edited Loading

Choose a reason for hiding this comment

AnnaShaleva Sep 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AnnaShaleva Oct 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AnnaShaleva Oct 7, 2024 • edited Loading

Choose a reason for hiding this comment

Slava0135 commented Sep 14, 2024

AnnaShaleva commented Sep 14, 2024

Slava0135 commented Oct 7, 2024

Slava0135 commented Oct 7, 2024

Slava0135 commented Oct 7, 2024

AnnaShaleva commented Oct 7, 2024

AnnaShaleva commented Oct 17, 2024

codecov bot commented Sep 10, 2024 •

edited

Loading

Slava0135 Sep 14, 2024 •

edited

Loading

AnnaShaleva Sep 14, 2024 •

edited

Loading

AnnaShaleva Oct 7, 2024 •

edited

Loading

AnnaShaleva Oct 7, 2024 •

edited

Loading