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

testing: Coverage() returns different values in 1.21 than 1.19.X #62212

Open
lbergesio opened this issue Aug 22, 2023 · 14 comments
Open

testing: Coverage() returns different values in 1.21 than 1.19.X #62212

lbergesio opened this issue Aug 22, 2023 · 14 comments
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.

Comments

@lbergesio
Copy link

What version of Go are you using (go version)?

$ go version
go version go1.19.12 linux/amd64

Does this issue reproduce with the latest release?

yes

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/leo/.cache/go-build"
GOENV="/home/leo/.config/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/home/leo/.gvm/pkgsets/go1.19.12/global/pkg/mod"
GONOPROXY="github.ibm.com"
GONOSUMDB="github.ibm.com"
GOOS="linux"
GOPATH="/home/leo/.gvm/pkgsets/go1.19.12/global"
GOPRIVATE="github.ibm.com"
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/home/leo/.gvm/gos/go1.19.12"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/home/leo/.gvm/gos/go1.19.12/pkg/tool/linux_amd64"
GOVCS=""
GOVERSION="go1.19.12"
GCCGO="/usr/bin/gccgo"
GOAMD64="v1"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/home/leo/Code/gateway/go.mod"
GOWORK=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -Wl,--no-gc-sections -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build470846844=/tmp/go-build -gno-record-gcc-switches"

What did you do?

Call testing.Coverage() and comparing the resulting value with the expected vaue that is working in 1.19.X

What did you expect to see?

testing.Coverage() to return the same value as 1.19.X.

I did not update to 1.20.X due to #59590, testing.Coverage() was totally broken in 1.20.X and in principle it was fixed for 1.21.0 but the return value is not the same as 1.19.X, it is quite lower

What did you see instead?

Different values:

coverage: 89.1% of statements
Tests passed but coverage at  55.0% <  88.3%

coverage: 73.5% of statements
Tests passed but coverage at  49.1% <  76.3%

coverage: 90.0% of statements
Tests passed but coverage at  25.0% <  80.0%

coverage: 94.4% of statements
Tests passed but coverage at  39.3% <  92.6%

coverage: 96.7% of statements
Tests passed but coverage at  54.8% <  94.5%
@seankhliao
Copy link
Member

please include code that demonstrates the difference in coverage.

@seankhliao seankhliao added the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Aug 22, 2023
@lbergesio
Copy link
Author

lbergesio commented Aug 22, 2023 •
edited

this function:

 func TestAndCheckCoverage(m *testing.M, percentage float64, checkGreater CheckGreaterOp) int {
         rc := m.Run()                                                           
                                                                                 
         // Solve rounding issues                                                
         expected := float64(int(percentage*10)) / 1000                          
                                                                                 
         // rc is zero if UT passed.                                             
         // CoverMode will be non empty if run with -cover.                      
         if rc == 0 && testing.CoverMode() != "" {                               
                 // Floor coverage to 1 decimal                                  
                 fmt.Println("REPORTED:", testing.Coverage())                    
                 c := math.Floor(testing.Coverage()*1000) / 1000                 
                 if c < expected {                                               
                         fmt.Printf("Tests passed but coverage at %5.1f%% < %5.1f%%\n", c*100., percentage)
                         os.Exit(1)                                              
                 } else if checkGreater == DoCheckGreater {                      
                         if c > expected {                                       
                                 fmt.Printf("Tests passed but coverage at %5.1f%% > %5.1f%%, please increase    
 expected value\n", c*100., percentage)
                                 os.Exit(1)                                      
                         }                                                       
                 }                                                               
         }                                                                       
         return rc                                                               
 }

make test is equivalent to go test -cover -coverprofile=tmp/coverage.out -trimpath -v -race -covermode=atomic -shuffle=on -count=1 ./cmd/... -timeout 10s

With 1.19.12:

leo:gateway$ make test | grep REPORTED
REPORTED: 0.8648648648648649
REPORTED: 0.8387096774193549
REPORTED: 0.64
REPORTED: 0.9938271604938271
REPORTED: 0.989406779661017
REPORTED: 0.9830508474576272
REPORTED: 0.8913043478260869
REPORTED: 1
REPORTED: 1
REPORTED: 1
REPORTED: 0.9375
REPORTED: 1
REPORTED: 0.8421052631578947
REPORTED: 1
REPORTED: 1
REPORTED: 0.904
REPORTED: 0.9819819819819819
REPORTED: 1
REPORTED: 1
REPORTED: 0.9117647058823529
REPORTED: 0.8588235294117647
REPORTED: 0.9512195121951219
REPORTED: 0.9221238938053097
REPORTED: 1
REPORTED: 0.9428571428571428
REPORTED: 1
REPORTED: 0.9458823529411765
REPORTED: 0.9512893982808023
REPORTED: 0.8833333333333333
REPORTED: 0.7631578947368421
REPORTED: 0.8
REPORTED: 0.926829268292683
REPORTED: 0.9458128078817734
REPORTED: 0.930379746835443

With 1.20.0:

leo:gateway$ make test | grep REPORTED
REPORTED: 0.42105263157894735
REPORTED: 0.43333333333333335
REPORTED: 0.36363636363636365
REPORTED: 0.4014962593516209
REPORTED: 0.4147424511545293
REPORTED: 0.4566929133858268
REPORTED: 0.7884615384615384
REPORTED: 0.52
REPORTED: 0.4230769230769231
REPORTED: 0.375
REPORTED: 0.3409090909090909
REPORTED: 0.375
REPORTED: 0.40336134453781514
REPORTED: 0.35294117647058826
REPORTED: 0.42857142857142855
REPORTED: 0.47478991596638653
REPORTED: 0.5141509433962265
REPORTED: 0.42857142857142855
REPORTED: 0.28125
REPORTED: 0.4105960264900662
REPORTED: 0.51138353765324
REPORTED: 0.4949238578680203
REPORTED: 0.5596133190118152
REPORTED: 0.45714285714285713
REPORTED: 0.43231441048034935
REPORTED: 0.40540540540540543
REPORTED: 0.48668280871670705
REPORTED: 0.5147286821705427
REPORTED: 0.5501730103806228
REPORTED: 0.4915254237288136
REPORTED: 0.25
REPORTED: 0.39378238341968913
REPORTED: 0.5485714285714286
REPORTED: 0.65625

@seankhliao
Copy link
Member

I mean, show us the code that is being tested and why the reported coverage percentage for it is incorrect

@seankhliao seankhliao added WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. and removed WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. labels Aug 22, 2023
@lbergesio
Copy link
Author

lbergesio commented Aug 22, 2023 •
edited

I mean, show us the code that is being tested and why the reported coverage percentage for it is incorrect

Can't do it, it is thousand of lines. I can tell you that I run the same code and the same test suit with different go version with those different results.

We have changed the minor version of go a lot of times since we started with go 1.8 and this never was a problem.

If that is not enough I can try to do some adhoc code at some moment.

@seankhliao
Copy link
Member

changed isn't a clear enough signal, we'll need to see if the old or new numbers are the correct ones.

@seankhliao seankhliao added WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. and removed WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. labels Aug 22, 2023
@lbergesio
Copy link
Author

lbergesio commented Aug 25, 2023 •
edited

Here you go:

leo:coverage$ cat main.go 
package main

import "fmt"

var v int

func f() {
	v = 3
	fmt.Println("V is", v)
}

func main() {
	f()
}
leo:coverage$ cat main_test.go 
package main

import "testing"
import "fmt"

func TestF(t *testing.T) {
	f()
	if v != 3 {
		t.Fatalf("Wrong value %d", v)
	}
	fmt.Println("Coverage:", testing.Coverage())
}
leo:coverage$

According to this I would say that the value from testing.Coverage() of 1.21 is definitely wrong (even though I do not know how this is calculated exactly):

leo:coverage$ gvm use go1.19.12
Now using version go1.19.12
leo:coverage$ go test -cover -race -covermode=atomic -coverprofile=.profile -trimpath -v
=== RUN   TestF
V is 3
Coverage: 0.5
--- PASS: TestF (0.00s)
PASS
coverage: 66.7% of statements
ok  	example.com/m	0.020s
leo:coverage$ go tool cover -func=.profile
example.com/m/main.go:7:	f		100.0%
example.com/m/main.go:13:	main		0.0%
total:				(statements)	66.7%


leo:coverage$ gvm use go1.21.0
Now using version go1.21.0
leo:coverage$ go test -cover -race -covermode=atomic -coverprofile=.profile -trimpath -v
=== RUN   TestF
V is 3
Coverage: 0.16666666666666666
--- PASS: TestF (0.00s)
PASS
coverage: 66.7% of statements
ok  	example.com/m	1.010s
leo:coverage$ go tool cover -func=.profile
example.com/m/main.go:7:	f		100.0%
example.com/m/main.go:13:	main		0.0%
total:				(statements)	66.7%

@lbergesio
Copy link
Author

Without -race different values:

leo:coverage$ gvm use go1.19.12
Now using version go1.19.12
leo:coverage$ go test -cover -covermode=atomic -coverprofile=.profile -trimpath -v
=== RUN   TestF
V is 3
Coverage: 0.5
--- PASS: TestF (0.00s)
PASS
coverage: 66.7% of statements
ok  	example.com/m	0.002s


leo:coverage$ gvm use go1.21.0
leo:coverage$ go test -cover -covermode=atomic -coverprofile=.profile -trimpath -v
=== RUN   TestF
V is 3
Coverage: 0.25
--- PASS: TestF (0.00s)
PASS
coverage: 66.7% of statements
ok  	example.com/m	0.003s

covermode=count same results:

leo:coverage$ gvm use go1.19.12
Now using version go1.19.12
leo:coverage$ go test -cover -covermode=count -coverprofile=.profile -trimpath -v
=== RUN   TestF
V is 3
Coverage: 0.5
--- PASS: TestF (0.00s)
PASS
coverage: 66.7% of statements
ok  	example.com/m	0.002s
leo:coverage$ gvm use go1.21.0
Now using version go1.21.0
leo:coverage$ go test -cover -covermode=count -coverprofile=.profile -trimpath -v
=== RUN   TestF
V is 3
Coverage: 0.25
--- PASS: TestF (0.00s)
PASS
coverage: 66.7% of statements
ok  	example.com/m	0.003s

@seankhliao seankhliao added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. and removed WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. labels Aug 25, 2023
@seankhliao
Copy link
Member

cc @thanm

@lbergesio lbergesio changed the title testing: Coverage returns different values than 1.19.X testing: Coverage() returns different values in 1.21 than 1.19.X Aug 25, 2023
@jehowell
Copy link

I am also seeing this behaviour

--- FAIL: TestConditionInfoTestSuite (259.21s)
    conditioninfo_test.go:953: Tests passed but coverage failed because 49.5% less than threshold of 100.0%
FAIL
coverage: 100.0% of statements

@jehowell
Copy link

Looking at the code... it looks like the snapshot function is wrong.

func snapshot() float64 {
	cl := getCovCounterList()
	if len(cl) == 0 {
		// no work to do here.
		return 0.0
	}

	tot := uint64(0)
	totExec := uint64(0)
	for _, c := range cl {
		sd := unsafe.Slice((*atomic.Uint32)(unsafe.Pointer(c.Counters)), c.Len)
		tot += uint64(len(sd))
		for i := 0; i < len(sd); i++ {
			// Skip ahead until the next non-zero value.
			if sd[i].Load() == 0 {
				continue
			}
			// We found a function that was executed.
			nCtrs := sd[i+coverage.NumCtrsOffset].Load()
			cst := i + coverage.FirstCtrOffset

			if cst+int(nCtrs) > len(sd) {
				break
			}
			counters := sd[cst : cst+int(nCtrs)]
			for i := range counters {
				if counters[i].Load() != 0 {
					totExec++
				}
			}
			i += coverage.FirstCtrOffset + int(nCtrs) - 1
		}
	}
	if tot == 0 {
		return 0.0
	}
	return float64(totExec) / float64(tot)
}

In particular, the tot += uint64(len(sd)) is adding the length of the entire structure which include the length of the counts. As this sd structure looks like this

// struct {
//     numCtrs uint32
//     pkgid uint32
//     funcid uint32
//     counterArray [numBlocks]uint32
// }

@jvidalallende
Copy link

As a potential workaround, running the tests with GOEXPERIMENT=nocoverageredesign yields the old, pre-1.20 results.

AlexanderYastrebov added a commit to AlexanderYastrebov/go that referenced this issue Sep 15, 2023
@AlexanderYastrebov
runtime/coverage: fix snapshot total calculation
f5da9a0 
Fixes golang#62212
@AlexanderYastrebov AlexanderYastrebov mentioned this issue Sep 15, 2023
Closed
@gopherbot
Copy link

Change https://go.dev/cl/528815 mentions this issue: runtime/coverage: fix snapshot total calculation

AlexanderYastrebov added a commit to AlexanderYastrebov/go that referenced this issue Sep 15, 2023
@AlexanderYastrebov
runtime/coverage: fix snapshot total calculation
0151db7 
Sum number of counters to obtain total number of coverable units
instead of using length of the section data.

Updates golang#62212
@lbergesio
Copy link
Author

lbergesio commented Feb 1, 2024 •
edited

Any update on this? The possible fix seems that has been abandoned. Thanks

@AlexanderYastrebov
Copy link
Contributor

The possible fix seems that has been abandoned

It was, see CL discussion

@AlexanderYastrebov AlexanderYastrebov mentioned this issue Feb 8, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
6 participants
@AlexanderYastrebov @jvidalallende @lbergesio @jehowell @gopherbot @seankhliao