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.

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

cmd/cover: go test coverage report incorrect when using loop in conjunction with parallel #19845

Closed
tyranteon opened this issue Apr 5, 2017 · 11 comments

Comments

@tyranteon
Copy link

@tyranteon tyranteon commented Apr 5, 2017

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

1.8

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

darwin/amd64

What did you do?

main.go:

func someFunction(value bool) int {
	if value {
		return 1
	} else {
		return 2
	}
}

main_test.go:

import (
	"testing"
)

func TestSomeFunction(t *testing.T) {
	someList := []bool {
		true, false,
	}

	for _, v := range someList {
		t.Run("testx", func(t *testing.T) {
			t.Parallel()
			someFunction(v)
		})
	}
}

run the command go test -coverprofile cover.out -covermode atomic

What did you expect to see?

Full coverage

What did you see instead?

The line with the return 1 is displayed as not covered.

Additional info

Manually unrolling the for-loop in TestSomeFunction fixes the problem:

func TestSomeFunction(t *testing.T) {
	t.Run("testx", func(t *testing.T) {
		t.Parallel()
		someFunction(true)
	})

	t.Run("testx", func(t *testing.T) {
		t.Parallel()
		someFunction(false)
	})
}

Removing the t.Parallel() fixes the problem as well.

Switching the order of false and true (i.e. false first and true second) also switches the line that is displayed as not being covered.

@ALTree ALTree changed the title go test coverage report incorrect when using loop in conjunction with parallel cmd/cover: go test coverage report incorrect when using loop in conjunction with parallel Apr 5, 2017
@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Apr 5, 2017

Pretty sure this is because you're missing -covermode=atomic. If you race under the race detector you'd probably see races right now too.

See cmd/go docs:

-covermode set,count,atomic
    Set the mode for coverage analysis for the package[s]
    being tested. The default is "set" unless -race is enabled,
    in which case it is "atomic".
    The values:
	set: bool: does this statement run?
	count: int: how many times does this statement run?
	atomic: int: count, but correct in multithreaded tests;
		significantly more expensive.
    Sets -cover.
@bradfitz bradfitz closed this Apr 5, 2017
@tyranteon
Copy link
Author

@tyranteon tyranteon commented Apr 5, 2017

@bradfitz this is not true. The test above has been done using -covermode=atomic, it doesn't fix the problem. I tripple checked that one. Look, I even mentioned it in the OP.

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Apr 5, 2017

Sorry, missed that.

@bradfitz bradfitz reopened this Apr 5, 2017
@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Apr 5, 2017

Do you see races, though?

@dhananjay92
Copy link
Contributor

@dhananjay92 dhananjay92 commented Apr 5, 2017

This looks like cmd/cover is either interpreting or displaying the coverage profile in incorrect way.

In the same example, if you use -func mode, it says 100% coverage, which is what we expect. But -html mode is displaying the same incorrectly.

$ go tool cover -func=cover.out
test/19845-cover-parallel/p.go:3: someFunction  100.0%
total:          (statements)  100.0%

$ go tool cover -html=cover.out
... shows wrong coverage in browser ...

Looking into why -html shows wrong lines.

@dhananjay92
Copy link
Contributor

@dhananjay92 dhananjay92 commented Apr 5, 2017

@SmaugTheGreat : When you say it's shown as not covered, do you mean it's shown in red color? Or grey?

In my local run, line return 1 is shown in grey. But that means low-coverage. Not covered lines are shown in red. (It's a heat map apparently; gradually going from grey to green).

@tyranteon
Copy link
Author

@tyranteon tyranteon commented Apr 5, 2017

@dhananjay92 It's shown in red color and the cover.out file is also listing the lines as not covered (not just the HTML representation)

I just tested it on my Windows machine at home with the same result (OP is Mac OS X). Here's a screen-shot with all the relevant data: http://imgur.com/vyjuQkK

@ALTree
Copy link
Member

@ALTree ALTree commented Apr 5, 2017

I can reproduce this both on go1.8 and on tip on a Linux system. Coverage is 66% and html shows the first return line in red (= no coverage).

@dhananjay92
Copy link
Contributor

@dhananjay92 dhananjay92 commented Apr 5, 2017

Thanks. I can also repro 66.7% coverage now. (Silly me; Earlier, I had commented t.Parallel() and forgot to uncomment that! 😅 )

Looking closely at the TestSomeFunction(), it calls someFunction() both times with false (last value in list): https://play.golang.org/p/xUrtBXiSOP

If you copy v in the loop (i.e before passing to concurrent code), problem will go away.

func TestSomeFunction(t *testing.T) {
  someList := []bool {
    true, false,
  }

  for _, v := range someList {
    v2 := v
    t.Run("testx", func(t *testing.T) {
      t.Parallel()
      someFunction(v2)
    })
  }
}
@ALTree
Copy link
Member

@ALTree ALTree commented Apr 5, 2017

Ah! This is also noted in the testing package documentation:

https://golang.org/pkg/testing/#hdr-Subtests_and_Sub_benchmarks

func TestGroupedParallel(t *testing.T) {
    for _, tc := range tests {
        tc := tc // capture range variable
        t.Run(tc.Name, func(t *testing.T) {
            t.Parallel()
            ...
        })
    }
}

Looks like this is working as intended; the error is in the code.

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Apr 5, 2017

Indeed.

@bradfitz bradfitz closed this Apr 5, 2017
@golang golang locked and limited conversation to collaborators Apr 5, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.