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

cmd/go: test -cover confused by import "." in xtest #15795

Closed
leighmcculloch opened this issue May 23, 2016 · 5 comments
Closed

cmd/go: test -cover confused by import "." in xtest #15795

leighmcculloch opened this issue May 23, 2016 · 5 comments
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Milestone
Go1.8

Comments

@leighmcculloch
Copy link
Contributor

leighmcculloch commented May 23, 2016 •
edited

Version of Go

1.6, also replicated on devel +4b6e560.

Operating system and processor architecture

Operating System: darwin
Processor Architecture: amd64

What I did

  • git clone https://github.com/leighmcculloch/static.git
  • git checkout with-xtests
  • go test -cover
  • git diff with-xtests..with-tests – change the tests package from static_test to static
  • git checkout with-tests
  • go test -cover

What I expected to see

I expected to see a coverage percentage for the static package that included the test files that are in the same directory but have their package at the top of their files named static_test. I expected that it wouldn't matter if the tests were in the package static_test or static, the coverage should be the same.

$ git checkout with-xtests
Switched to branch 'with-xtests'
Your branch is up-to-date with 'origin/with-xtests'.

$ go test -cover
PASS
coverage: 93.9% of statements
ok      github.com/leighmcculloch/static        0.023s

$ git checkout with-tests
Switched to branch 'with-tests'
Your branch is up-to-date with 'origin/with-tests'.

$ go test -cover
PASS
coverage: 93.9% of statements
ok      github.com/leighmcculloch/static        0.028s

Additionally expected that looking at the coverage in the HTML format would show most lines green.

What I saw instead

The coverage percentage for the static package is only 20.4% and is not including tests that are named as being in the static_test package, even though those tests are in the same directory as the static package and are being executed.

When I change the package the tests live in from static_test to static, they are included in the coverage and the coverage is 93.9%.

$ git checkout with-xtests
Switched to branch 'with-xtests'
Your branch is up-to-date with 'origin/with-xtests'.

$ go test -cover
PASS
coverage: 20.4% of statements
ok      github.com/leighmcculloch/static        0.023s

$ git checkout with-tests
Switched to branch 'with-tests'
Your branch is up-to-date with 'origin/with-tests'.

$ go test -cover
PASS
coverage: 93.9% of statements
ok      github.com/leighmcculloch/static        0.028s

Additionally saw that looking at the coverage in the HTML format showed most lines red.

What I've attempted so far to fix it

I've looked (naively) at the source of cmd/go/test.go and I can see the tests grouped in pxtest are not recompiled, there are no coverVars associated with pxtest. I attempted to fix this by adding the p.GoFiles to the pxtests coverVars but I ended up with errors like the below. I'm guessing this is because there's an assumption being made somewhere that files included in coverage are in the same package as the tests.

/var/folders/yr/llg1db9x72v7dgycypwy9pyc0000gq/T/go-build725799557/github.com/leighmcculloch/static/_test/_testmain.go:109: undefined: static_test.GoCover_6 in static_test.GoCover_6.Count

I plan to continue looking at this in more depth, but hoped I could get some suggestions, pointers or confirmation of where I should be looking.
@leighmcculloch leighmcculloch changed the title cmd/go: test -cover does not include xtests (in _test package) in coverage percentage or profile cmd/go: test -cover does not include xtests (_test package) in coverage percentage or profile May 23, 2016
@ianlancetaylor ianlancetaylor added this to the Go1.8 milestone May 24, 2016
@bradfitz
Copy link
Contributor

@robpike, are you still the cmd/cover maintainer?

@quentinmit quentinmit added the NeedsFix The path to resolution is known, but the work has not been done. label Oct 6, 2016
@robpike robpike added WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. and removed NeedsFix The path to resolution is known, but the work has not been done. labels Oct 16, 2016
@robpike
Copy link
Contributor

robpike commented Oct 18, 2016

Where is the comment I wrote when I added the WaitingForInfo tag?

What it said was this: Cover handles this situation correctly in packages such as fmt. And I tried changing encoding/binary to have both internal and external tests, and cover got the same result both times. So I don't understand the problem.

Can you please create a reproducing example that is self-contained?

@rsc
Copy link
Contributor

rsc commented Oct 21, 2016

@leighmcculloch, the problem is caused by the import "." in your xtest versions. The go command should really disallow that. The convention for an xtest is to change

package static

to

package static_test

import . "github.com/leighmcculloch/static"

and then you don't need to change anything else (in particular you don't have to put a "static." prefix on every identifier). That produces the right coverage percentages.

If instead I do

package static_test

import . "."

then I get the result you describe, where the coverage percentage does not include all the lines in the test.

I can't remember where the issue is about disallowing import of ".", but this may be an argument one way or the other in that issue.

@robpike, no need to work on this.

@rsc rsc unassigned robpike Oct 21, 2016
@rsc rsc changed the title cmd/go: test -cover does not include xtests (_test package) in coverage percentage or profile cmd/go: test -cover confused by import "." in xtest Oct 21, 2016
@rsc rsc added NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. and removed WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. labels Oct 21, 2016
leighmcculloch added a commit to leighmcculloch/go-static that referenced this issue Oct 21, 2016
@leighmcculloch
Fixed coverage results for xtests.
5890d89 
What
===
Replaced use of `import "."` with `import
"github.com/leighmcculloch/static"` in xtests.

Why
===
The go cover tool does not include tests that have imported the package
into xtests (`static_test`) using the dot syntax. Ref:
golang/go#15795 (comment)
leighmcculloch added a commit to leighmcculloch/go-static that referenced this issue Oct 21, 2016
@leighmcculloch
Converted tests into blackbox tests.
76c815c 
What
===
Replaced `package static` with `package static_test` in tests that are
testing entirely exported functionality.

Why
===
Blackbox testing is preferred as it results in tests that use the
package in the exact same way that another developer would use the
package.

Tests were originally written this way but it was changed when it was
discovered that the go cover tool was not including blackbox tests in
the coverage percentage.

According to golang/go#15795 the tests were not being included in the
package as because the package had been included into the blackbox tests
using the dot syntax (`import "."`). Importing using the fully qualified
package name fixes this, which is how the blackbox testing imports has
been implemented in this commit.
@leighmcculloch leighmcculloch mentioned this issue Oct 21, 2016
Merged
@leighmcculloch
Copy link
Contributor Author

Ah fantastic. Thank you!!!

I can confirm replacing import "." with import "github.com/leighmcculloch/static" (e.g. leighmcculloch/go-static@5890d89) results in the xtests returning the correct coverage.

$ git checkout with-xtests
Switched to branch 'with-xtests'
Your branch is up-to-date with 'origin/with-xtests'.

$ go test -cover
PASS
coverage: 93.9% of statements
ok      github.com/leighmcculloch/static        0.020s

$ git checkout with-tests
Switched to branch 'with-tests'
Your branch is up-to-date with 'origin/with-tests'.

$ go test -cover
PASS
coverage: 93.9% of statements
ok      github.com/leighmcculloch/static        0.020s

I will avoid use of import "." in the future.

leighmcculloch added a commit to leighmcculloch/go-static that referenced this issue Oct 21, 2016
@leighmcculloch
Converted tests into blackbox tests. (#1)
762e825 
What
===
Replaced `package static` with `package static_test` in tests that are
testing entirely exported functionality.

Why
===
Blackbox testing is preferred as it results in tests that use the
package in the exact same way that another developer would use the
package.

Tests were originally written this way but it was changed when it was
discovered that the go cover tool was not including blackbox tests in
the coverage percentage.

According to golang/go#15795 the tests were not being included in the
package as because the package had been included into the blackbox tests
using the dot syntax (`import "."`). Importing using the fully qualified
package name fixes this, which is how the blackbox testing imports has
been implemented in this commit.
@gopherbot
Copy link

CL https://golang.org/cl/31821 mentions this issue.

@golang golang locked and limited conversation to collaborators Oct 28, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Projects
None yet
Milestone
Go1.8
Development

No branches or pull requests
7 participants
@bradfitz @rsc @quentinmit @leighmcculloch @ianlancetaylor @robpike @gopherbot