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.

Sign up for GitHub

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

neotest: exclude RET statements from coverage #3617

Closed
wants to merge 1 commit into from
Closed

neotest: exclude RET statements from coverage #3617

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 29 additions & 3 deletions pkg/neotest/coverage.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,8 +50,14 @@
)

type scriptRawCoverage struct {
debugInfo *compiler.DebugInfo
offsetsVisited []int
debugInfo *compiler.DebugInfo
// methodStartOffsets holds the set of script offsets that correspond to method
// declaration start.
methodStartOffsets map[uint16]struct{}
// methodStartOffsets holds the set of script offsets that correspond to method
// declaration end.
methodEndOffsets map[uint16]struct{}
offsetsVisited []int
}

type coverBlock struct {
Expand Down Expand Up @@ -174,6 +180,12 @@
documentSeqPoints := documentSeqPoints(di, documentName)

for _, point := range documentSeqPoints {
// Don't track RET statements since their bounds overlap the whole function
// which break the resulting coverage report.
_, isRET := scriptRawCoverage.methodEndOffsets[uint16(point.Opcode)]
if isRET {
continue

Check warning on line 187 in pkg/neotest/coverage.go

View check run for this annotation

Codecov / codecov/patch

pkg/neotest/coverage.go#L185-L187 

Added lines #L185 - L187 were not covered by tests
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was my best attempt. I also tried to keep return statements in the coverage but "shorten" their bounds to a single "end" line, but it's not displayed properly and sometimes even wrong when there's no explicit "return" statement in the code. Hence, I decided to remove these sequence points from coverage.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although I'm still not sure if it's a good solution because: it does not exclude those RET statements that are not in the end of the method. Although such RET statements have proper bounds. So may be it's even bug in the compiler and RET statements shouldn't actually have such method-body-width bounds.

Copy link
Member Author

@AnnaShaleva AnnaShaleva Oct 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And if it's a compiler bug:

neo-go/pkg/compiler/codegen.go

Lines 539 to 543 in 86ed214

// This can be the case with void and named-return functions.
if !isInit && !isDeploy && !lastStmtIsReturn(decl.Body) {
c.processDefers()
c.saveSequencePoint(decl.Body)
emit.Opcodes(c.prog.BinWriter, opcode.RET)

Then it's not clear what are the correct bounds for this "RET" opcode, because there's actually no return statement in the contract code itself.

Copy link
Member Author

@AnnaShaleva AnnaShaleva Oct 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally, if we keep everything as is on the compiler level, then on the neotest level we need some way to distinguish some "real" return statements that are presented in the code in the end of every method from "RET" statements that belong to void and named-return functions. Currently we can't distinguish them based on the given debug info format, hence this PR removes all RET statements that belong to the end of the function.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And I probably also need to check that no other statements except "RET" may be excluded by this code.

Copy link
Member Author

@AnnaShaleva AnnaShaleva Oct 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And I probably also need to check that no other statements except "RET" may be excluded by this code.

OK, it’s not required, because our compiler always emit RET in the end of functions, and there's no function that ends with another instruction.

}
b := coverBlock{
startLine: uint(point.StartLine),
startCol: uint(point.StartCol),
Expand All @@ -193,6 +205,10 @@
for _, offset := range scriptRawCoverage.offsetsVisited {
for _, point := range documentSeqPoints {
if point.Opcode == offset {
_, ok := mappedBlocks[point.Opcode]
if !ok {
continue

Check warning on line 210 in pkg/neotest/coverage.go

View check run for this annotation

Codecov / codecov/patch

pkg/neotest/coverage.go#L208-L210 

Added lines #L208 - L210 were not covered by tests
}
mappedBlocks[point.Opcode].counts++
}
}
Expand Down Expand Up @@ -239,6 +255,16 @@
coverageLock.Lock()
defer coverageLock.Unlock()
if _, ok := rawCoverage[c.Hash]; !ok {
rawCoverage[c.Hash] = &scriptRawCoverage{debugInfo: c.DebugInfo}
startOffsets := make(map[uint16]struct{})
endOffsets := make(map[uint16]struct{})
for _, m := range c.DebugInfo.Methods {
startOffsets[m.Range.Start] = struct{}{}
endOffsets[m.Range.End] = struct{}{}

Check warning on line 262 in pkg/neotest/coverage.go

View check run for this annotation

Codecov / codecov/patch

pkg/neotest/coverage.go#L258-L262 

Added lines #L258 - L262 were not covered by tests
}
rawCoverage[c.Hash] = &scriptRawCoverage{
debugInfo: c.DebugInfo,
methodStartOffsets: startOffsets,
methodEndOffsets: endOffsets,

Check warning on line 267 in pkg/neotest/coverage.go

View check run for this annotation

Codecov / codecov/patch

pkg/neotest/coverage.go#L264-L267 

Added lines #L264 - L267 were not covered by tests
}
}
}
Loading