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

Fix undercounting of coverage for lines in multiple blocks. #63

Merged
merged 1 commit into from Mar 20, 2016

Conversation

eaburns
Copy link
Contributor

@eaburns eaburns commented Mar 2, 2016

The coverprofile format uses both lines and columns. It can report coverage for the same line across multiple blocks, if the column ranges do not overlap. The coverage counts should be summed, not assigned.

Here is an example:

c.go

package c

func False() bool { return false }

func F() int {
    if False() {
        return 1
    }
    return 2
}

c_test.go

package c

import "testing"

func TestF(t *testing.T) { F() }

The return 1 is never tested, but the call to False() is tested.

The coverprofile is:

mode: set
test/c/c.go:3.19,3.35 1 1
test/c/c.go:5.14,6.13 1 1
test/c/c.go:9.2,9.10 1 1
test/c/c.go:6.13,8.3 1 0

Line 6, the condition of the if-statement, is accounted for in two blocks. The first refers to the statement False(), the second refers to the beginning { of the block. The count of 0 for the un-covered return 1 was over-writing the count of 1 for the covered False(), causing the coverage to be under-counted.

Fixes #61

@eaburns
Copy link
Contributor Author

eaburns commented Mar 2, 2016

Can you think of a good way to test this?

@eaburns eaburns force-pushed the issue61 branch 3 times, most recently from b1b02b2 to 6449244 Compare March 2, 2016 20:32
The coverprofile format uses both lines and columns. It can report coverage for the same line across multiple blocks, if the column ranges do not overlap. The coverage counts should be summed, not assigned.

Here is an example:

c.go
```
package c

func False() bool { return false }

func F() int {
	if False() {
		return 1
	}
	return 2
}
```

c_test.go
```
package c

import "testing"

func TestF(t *testing.T) { F() }

```

The `return 1` is never tested, but the call to `False()` is tested.

The coverprofile is:
```
mode: set
test/c/c.go:3.19,3.35 1 1
test/c/c.go:5.14,6.13 1 1
test/c/c.go:9.2,9.10 1 1
test/c/c.go:6.13,8.3 1 0
```

Line 6, the condition of the if-statement, is accounted for in two blocks. The first refers to the statement `False()`, the second refers to the beginning `{` of the block. The count of 0 for the un-covered `return 1` was over-writing the count of 1 for the covered `False()`, causing the coverage to be under-counted.

Fixes mattn#61
@eaburns
Copy link
Contributor Author

eaburns commented Mar 2, 2016

I tried it out and it seems to work: https://coveralls.io/builds/5279120/source?filename=edit%2Fedit.go. But it still reports the closing brace as uncovered. I'll see if I can fix that.

@eaburns
Copy link
Contributor Author

eaburns commented Mar 2, 2016

I think that this is the best we will get. Coverall computes line-level coverage, but go tool cover computes a finer-grained statement-level coverage.

@eaburns
Copy link
Contributor Author

eaburns commented Mar 2, 2016

Here's an option that handles the case where there are more lines than statements by only counting the middle block of NumStmt lines: master...eaburns:issue61_statements.

I'm trying it out now.

@eaburns
Copy link
Contributor Author

eaburns commented Mar 2, 2016

I think that it's best not to do master...eaburns:issue61_statements. It's a bit of a hack. Better to have correct line-level coverage than incorrect statement-level coverage.

This is ready for your review.

@mattn
Copy link
Owner

mattn commented Mar 3, 2016

I'm thinking closing brace should not be passed. So I guess your patch works well.

@eaburns
Copy link
Contributor Author

eaburns commented Mar 19, 2016

I think this is good to merge, if you are happy with it.

mattn added a commit that referenced this pull request Mar 20, 2016
Fix undercounting of coverage for lines in multiple blocks.
@mattn mattn merged commit b8787b6 into mattn:master Mar 20, 2016
@mattn
Copy link
Owner

mattn commented Mar 20, 2016

Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants