[go] cmd/go: fix `-coverpkg` not ignoring special directories
Gerrit Bot (Gerrit)
Gerrit Bot has uploaded the change for review![]( "Open in Gerrit")
Commit message
cmd/go: fix `-coverpkg` not ignoring special directories
The pattern passed to `-coverpkg` when running `go test` would not
ignore directories usually ignored by the `go` command, i.e. those
beginning with "." or "_" are ignored by the go tool, as are directories
named "testdata".
Fix this by adding an explicit check for these (by following a similar
check in `src/cmd/doc/dirs.go`[1]) allowing us to ignore them.
Two tests are added for this change, one is a regression test attempting
to directly replicate the behaviour described in the issue, the other is
updating another test I saw fail when trialling other solutions to this
issue so I thought it worthwhile to be explicit about the change there.
See linked issue for a reproduction.
Fixes #66038
[1] https://go.googlesource.com/go/+/16e5d24480dca7ddcbdffb78a8ed5de3e5155dec/src/cmd/doc/dirs.go#136
Change diff
diff --git a/src/cmd/go/go_test.go b/src/cmd/go/go_test.go
index 3282295..01ee420 100644
--- a/src/cmd/go/go_test.go
+++ b/src/cmd/go/go_test.go
@@ -2778,6 +2778,47 @@
tg.grepStdout("coverage: 100", "no coverage")
}
+// regression test for github.com/golang/go/issues/66038
+func TestCoverpkgIngoresSpecialDirs(t *testing.T) {
+ skipIfGccgo(t, "gccgo has no cover tool")
+ tooSlow(t, "links and runs a test binary with coverage enabled")
+
+ tg := testgo(t)
+ defer tg.cleanup()
+
+ wd, err := os.Getwd()
+ tg.check(err)
+ tg.makeTempdir()
+ tg.check(os.Chdir(tg.path(".")))
+ defer func() { tg.check(os.Chdir(wd)) }()
+
+ tg.tempFile("a/a.go", `package a
+ import ( _ "b" )
+ func F(i int) int {
+ return i*i
+ }`)
+ tg.tempFile("a/a_test.go", `
+ package a
+ import ( "testing" )
+ func TestF(t *testing.T) { F(2) }
+ `)
+
+ for _, skipDir := range []string{".dir", "_dir", "testdata", "dir/testdata"} {
+ t.Run(skipDir, func(t *testing.T) {
+ tg.tempFile(fmt.Sprintf("%s/src/b/b.go", skipDir), `package b
+ func G(i int) int {
+ return i*i
+ }`)
+ tg.setenv("GOPATH", tg.path(skipDir))
+
+ tg.run("test", "-coverpkg=./...", "./...")
+
+ tg.grepStderrNot("no packages being tested depend on matches", "bad match message")
+ tg.grepStdout("coverage: 100", "no coverage")
+ })
+ }
+}
+
// Regression test for golang.org/issue/34499: version command should not crash
// when executed in a deleted directory on Linux.
func TestExecInDeletedDir(t *testing.T) {
diff --git a/src/cmd/go/internal/load/flag_test.go b/src/cmd/go/internal/load/flag_test.go
index d3223e1..f5b38f5 100644
--- a/src/cmd/go/internal/load/flag_test.go
+++ b/src/cmd/go/internal/load/flag_test.go
@@ -92,6 +92,7 @@
ppfDirTest("./...", 3, "/my/test/dir", "/my/test/dir/sub", "/my/test/dir/sub/sub", "/my/test/other", "/my/test/other/sub"),
ppfDirTest("../...", 4, "/my/test/dir", "/my/test/other", "/my/test/dir/sub", "/my/test/other/sub", "/my/other/test"),
ppfDirTest("../...sub...", 3, "/my/test/dir/sub", "/my/test/othersub", "/my/test/yellowsubmarine", "/my/other/test"),
+ ppfDirTest("./...", 1, "/my/test/dir", "/my/test/dir/.sub", "/test/dir/_sub", "/test/dir/testdata/sub"),
}
func ppfDirTest(pattern string, nmatch int, dirs ...string) ppfTest {
diff --git a/src/cmd/go/internal/load/search.go b/src/cmd/go/internal/load/search.go
index 565996a..83f469c 100644
--- a/src/cmd/go/internal/load/search.go
+++ b/src/cmd/go/internal/load/search.go
@@ -42,6 +42,19 @@
if rel == ".." || strings.HasPrefix(rel, "../") {
return false
}
+
+ cwdRel, err := filepath.Rel(cwd, p.Dir)
+ if err == nil {
+ if cwdRel != "." && !strings.HasPrefix(cwdRel, "../") {
+ for _, elem := range strings.Split(cwdRel, string(filepath.Separator)) {
+ // Avoid .foo, _foo, and testdata subdirectory trees.
+ if strings.HasPrefix(elem, ".") || strings.HasPrefix(elem, "_") || elem == "testdata" {
+ return false
+ }
+ }
+ }
+ }
+
return matchPath(rel)
}
case pattern == "all":
Change information
- M src/cmd/go/go_test.go
- M src/cmd/go/internal/load/flag_test.go
- M src/cmd/go/internal/load/search.go
Related details
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gopher Robot (Gerrit)
Gopher Robot added 1 comment![]( "Open in Gerrit")
I spotted some possible problems.
These findings are based on simple heuristics. If a finding appears wrong, briefly reply here saying so. Otherwise, please address any problems and update the GitHub PR. When complete, mark this comment as 'Done' and click the [blue 'Reply' button](https://go.dev/wiki/GerritBot#i-left-a-reply-to-a-comment-in-gerrit-but-no-one-but-me-can-see-it) above.
Possible problems detected:
1. Are you using markdown? Markdown should not be used to augment text in the commit message.
2. It looks like you have a properly formated bug reference, but the convention is to put bug references at the bottom of the commit message, even if a bug is also mentioned in the body of the message.
The commit title and commit message body come from the GitHub PR title and description, and must be edited in the GitHub web interface (not via git). For instructions, see [here](https://go.dev/wiki/GerritBot/#how-does-gerritbot-determine-the-final-commit-message). For guidelines on commit messages for the Go project, see [here](https://go.dev/doc/contribute#commit_messages).
(In general for Gerrit code reviews, the change author is expected to [log in to Gerrit](https://go-review.googlesource.com/login/) with a Gmail or other Google account and then close out each piece of feedback by marking it as 'Done' if implemented as suggested or otherwise reply to each review comment. See the [Review](https://go.dev/doc/contribute#review) section of the Contributing Guide for details.)
Related details
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gopher Robot (Gerrit)
Message from Gopher Robot![]( "Open in Gerrit")
Congratulations on opening your first change. Thank you for your contribution!
Next steps:
A maintainer will review your change and provide feedback. See
https://go.dev/doc/contribute#review for more info and tips to get your
patch through code review.
Most changes in the Go project go through a few rounds of revision. This can be
surprising to people new to the project. The careful, iterative review process
is our way of helping mentor contributors and ensuring that their contributions
have a lasting impact.
During May-July and Nov-Jan the Go project is in a code freeze, during which
little code gets reviewed or merged. If a reviewer responds with a comment like
R=go1.11 or adds a tag like "wait-release", it means that this CL will be
reviewed as part of the next development cycle. See https://go.dev/s/release
for more details.
Related details
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Than McIntosh (Gerrit)
Than McIntosh added 4 comments![]( "Open in Gerrit")
Looks good overall, see comments inline.
See linked issue for a reproduction.you can probably leave this out, folks will know to go read the issue for more info.
func TestCoverpkgIngoresSpecialDirs(t *testing.T) {Is there a chance you can write this as a script test instead of a hand-coded test? For this sort of scenario (where you have to build a program and do testing) the script test would probably be a lot more readable. If using the script engine doesn't work for some reason we can fall back to this.
for _, elem := range strings.Split(cwdRel, string(filepath.Separator)) {maybe use filepath.Split here?
Related details
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Than McIntosh (Gerrit)
Than McIntosh voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Michael Matloob
- Sam Thanawalla
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Matthew Hughes (Gerrit)
Matthew Hughes added 4 comments![]( "Open in Gerrit")
Thanks for the review, I had some follow-up questions from it
See linked issue for a reproduction.you can probably leave this out, folks will know to go read the issue for more info.
Good call, I've updated the PR description (from what I understand that will propagate the change here, rather than updating the commit itself)
func TestCoverpkgIngoresSpecialDirs(t *testing.T) {Is there a chance you can write this as a script test instead of a hand-coded test? For this sort of scenario (where you have to build a program and do testing) the script test would probably be a lot more readable. If using the script engine doesn't work for some reason we can fall back to this.
Ah, I didn't know script tests were a thing. From a glance these look to be under src/cmd/go/testdata/script, is that correct? If so I'll have a look over there and try and build something
for _, elem := range strings.Split(cwdRel, string(filepath.Separator)) {maybe use filepath.Split here?
Just as I was preparing this commit I was wondering if it's sufficient to compare just on the directory name: e.g. should `./some/dir/testdata/file.go` be ignored as well as `./testdata/file.go` (or also something like `./some/dir/.dir/file.go`) I think for these nested cases comparing just on the directory is not enough, and each component of the path needs to be checked. Though I've been undecided on this since submitting the change, what are your thoughts?
Related details
- Michael Matloob
- Sam Thanawalla
- Than McIntosh
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gerrit Bot (Gerrit)
Gerrit Bot uploaded new patchset![]( "Open in Gerrit")
Related details
- Michael Matloob
- Sam Thanawalla
- Than McIntosh
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Matthew Hughes (Gerrit)
Matthew Hughes added 1 comment![]( "Open in Gerrit")
At a glance the TryBot failures didn't appear directly related to my changes. I've just rebased on master (I was a couple of hundred commits behind) to see if if that fixes it
Related details
- Michael Matloob
- Sam Thanawalla
- Than McIntosh
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Than McIntosh (Gerrit)
Than McIntosh voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Michael Matloob
- Sam Thanawalla
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gerrit Bot (Gerrit)
Gerrit Bot uploaded new patchset![]( "Open in Gerrit")
- TryBots-Pass: LUCI-TryBot-Result-1 by Go LUCI
Related details
- Michael Matloob
- Sam Thanawalla
- Than McIntosh
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Than McIntosh (Gerrit)
Than McIntosh added 2 comments![]( "Open in Gerrit")
func TestCoverpkgIngoresSpecialDirs(t *testing.T) {Matthew HughesIs there a chance you can write this as a script test instead of a hand-coded test? For this sort of scenario (where you have to build a program and do testing) the script test would probably be a lot more readable. If using the script engine doesn't work for some reason we can fall back to this.
Ah, I didn't know script tests were a thing. From a glance these look to be under src/cmd/go/testdata/script, is that correct? If so I'll have a look over there and try and build something
Yes, testdata/script/<testcase>.txt.
You could use testdata/script/cover_swig.txt or testdata/script/cover_coverpkg_with_init.txt as examples.
for _, elem := range strings.Split(cwdRel, string(filepath.Separator)) {Matthew Hughesmaybe use filepath.Split here?
Just as I was preparing this commit I was wondering if it's sufficient to compare just on the directory name: e.g. should `./some/dir/testdata/file.go` be ignored as well as `./testdata/file.go` (or also something like `./some/dir/.dir/file.go`) I think for these nested cases comparing just on the directory is not enough, and each component of the path needs to be checked. Though I've been undecided on this since submitting the change, what are your thoughts?
On second thought I think you are right, it does make sense to look at all the path elements.
Related details
- Matthew Hughes
- Michael Matloob
- Sam Thanawalla
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Emmanuel Odeke (Gerrit)
Emmanuel Odeke voted and added 4 comments![]( "Open in Gerrit")
Votes added by Emmanuel Odeke
| Hold | +1 |
4 comments
Thank you for this change Matthew! I have added some suggestions to improve the change, please take a look.
cmd/go: fix `-coverpkg` not ignoring special directoriesWe avoid markdown in what gets into the Go tree, so please do this:
cmd/go: make -coverpkg properly ignore special directories
The pattern passed to `-coverpkg` when running `go test` would not
ignore directories usually ignored by the `go` command, i.e. those
beginning with "." or "_" are ignored by the go tool, as are directories
named "testdata".
Fix this by adding an explicit check for these (by following a similar
check in `src/cmd/doc/dirs.go`[1]) allowing us to ignore them.
Two tests are added for this change, one is a regression test attempting
to directly replicate the behaviour described in the issue, the other is
updating another test I saw fail when trialling other solutions to this
issue so I thought it worthwhile to be explicit about the change there.
We can make the commit message much more concise and precise by:
This change fixes "go test -coverpkg" to properly ignore
special directories that are typically ignored by the "go" command
for example directories beginnning with "." or "_" or "testdata"
Fixes #66038
cwdRel, err := filepath.Rel(cwd, p.Dir)
if err == nil {
if cwdRel != "." && !strings.HasPrefix(cwdRel, "../") {
for _, elem := range strings.Split(cwdRel, string(filepath.Separator)) {
// Avoid .foo, _foo, and testdata subdirectory trees.
if strings.HasPrefix(elem, ".") || strings.HasPrefix(elem, "_") || elem == "testdata" {
return false
}
}
}
}
We can refactor by inversion to make this code more legible
```
relToWorkingDir, err := filepath.Rel(cwd, p.Dir)
if err != nil {
return matchPath(rel)
}
hasAnyPrefix := func(dir, prefixes ...string) bool {
for _, prefix := range prefixes {
if strings.HasPrefix(dir, prefix) {
return true
}
}
return false
}if relToWorkingDir == "." || hasAnyPrefix(relToWorkingDir, ".." + string(filepath.Separator)) {
// Not a special directory so can return immediately
return matchPath(rel)
}// Otherwise avoid special directories "testdata" or beginning with ".", "_".
pathComponents := strings.Split(relToWorkingDir, string(filepath.Separator))
for _, elem := range pathComponents {
if hasAnyPrefix(elem, ".", "_") || elem == "testdata" {
return false
}
}
return matchPath(rel)
```
Related details
- Matthew Hughes
- Michael Matloob
- Sam Thanawalla
Code-Review
No-Holds
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gerrit Bot (Gerrit)
Gerrit Bot uploaded new patchset![]( "Open in Gerrit")
- Emmanuel Odeke
- Matthew Hughes
- Michael Matloob
- Sam Thanawalla
- Than McIntosh
Code-Review
No-Holds
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Matthew Hughes (Gerrit)
Matthew Hughes added 5 comments![]( "Open in Gerrit")
Thanks for the reviews!
Apologies for the slowed response, I hadn't realised I never clicked the 'reply' button 😐
cmd/go: fix `-coverpkg` not ignoring special directoriesWe avoid markdown in what gets into the Go tree, so please do this:
cmd/go: make -coverpkg properly ignore special directories
Thanks, updated the PR title
The pattern passed to `-coverpkg` when running `go test` would not
ignore directories usually ignored by the `go` command, i.e. those
beginning with "." or "_" are ignored by the go tool, as are directories
named "testdata".
Fix this by adding an explicit check for these (by following a similar
check in `src/cmd/doc/dirs.go`[1]) allowing us to ignore them.
Two tests are added for this change, one is a regression test attempting
to directly replicate the behaviour described in the issue, the other is
updating another test I saw fail when trialling other solutions to this
issue so I thought it worthwhile to be explicit about the change there.
Fixes #66038
[1] https://go.googlesource.com/go/+/16e5d24480dca7ddcbdffb78a8ed5de3e5155dec/src/cmd/doc/dirs.go#136We can make the commit message much more concise and precise by:
This change fixes "go test -coverpkg" to properly ignore
special directories that are typically ignored by the "go" command
for example directories beginnning with "." or "_" or "testdata"Fixes #66038
Thanks, I've updated the PR description in GitHub. I left the paragraph in describing the reasoning behind the tests added, do you think there's value in this or should I remove it too?
func TestCoverpkgIngoresSpecialDirs(t *testing.T) {Matthew HughesIs there a chance you can write this as a script test instead of a hand-coded test? For this sort of scenario (where you have to build a program and do testing) the script test would probably be a lot more readable. If using the script engine doesn't work for some reason we can fall back to this.
Than McIntoshAh, I didn't know script tests were a thing. From a glance these look to be under src/cmd/go/testdata/script, is that correct? If so I'll have a look over there and try and build something
Yes, testdata/script/<testcase>.txt.
You could use testdata/script/cover_swig.txt or testdata/script/cover_coverpkg_with_init.txt as examples.
Yes, testdata/script/<testcase>.txt.
You could use testdata/script/cover_swig.txt or testdata/script/cover_coverpkg_with_init.txt as examples.
Thanks I gave this a shot with the latest patchset
cwdRel, err := filepath.Rel(cwd, p.Dir)
if err == nil {
if cwdRel != "." && !strings.HasPrefix(cwdRel, "../") {
for _, elem := range strings.Split(cwdRel, string(filepath.Separator)) {
// Avoid .foo, _foo, and testdata subdirectory trees.
if strings.HasPrefix(elem, ".") || strings.HasPrefix(elem, "_") || elem == "testdata" {
return false
}
}
}
}
We can refactor by inversion to make this code more legible
```
relToWorkingDir, err := filepath.Rel(cwd, p.Dir)
if err != nil {
return matchPath(rel)
}hasAnyPrefix := func(dir, prefixes ...string) bool {
for _, prefix := range prefixes {
if strings.HasPrefix(dir, prefix) {
return true
}
}
return false
}if relToWorkingDir == "." || hasAnyPrefix(relToWorkingDir, ".." + string(filepath.Separator)) {
// Not a special directory so can return immediately
return matchPath(rel)
}// Otherwise avoid special directories "testdata" or beginning with ".", "_".
pathComponents := strings.Split(relToWorkingDir, string(filepath.Separator))
for _, elem := range pathComponents {
if hasAnyPrefix(elem, ".", "_") || elem == "testdata" {
return false
}
}return matchPath(rel)
```
Thanks for the suggestion, I've added it with the latest changeset
Related details
- Emmanuel Odeke
- Michael Matloob
- Sam Thanawalla
- Than McIntosh
Code-Review
No-Holds
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Emmanuel Odeke (Gerrit)
Emmanuel Odeke voted and added 3 comments![]( "Open in Gerrit")
Votes added by Emmanuel Odeke
| Code-Review | +1 |
| Commit-Queue | +1 |
| Hold | +0 |
| Run-TryBot | +1 |
3 comments
Thank you Matthew! Just one more change then good to go in my eyes!
The pattern passed to `-coverpkg` when running `go test` would not
ignore directories usually ignored by the `go` command, i.e. those
beginning with "." or "_" are ignored by the go tool, as are directories
named "testdata".
Fix this by adding an explicit check for these (by following a similar
check in `src/cmd/doc/dirs.go`[1]) allowing us to ignore them.
Two tests are added for this change, one is a regression test attempting
to directly replicate the behaviour described in the issue, the other is
updating another test I saw fail when trialling other solutions to this
issue so I thought it worthwhile to be explicit about the change there.
Fixes #66038
[1] https://go.googlesource.com/go/+/16e5d24480dca7ddcbdffb78a8ed5de3e5155dec/src/cmd/doc/dirs.go#136Matthew HughesWe can make the commit message much more concise and precise by:
This change fixes "go test -coverpkg" to properly ignore
special directories that are typically ignored by the "go" command
for example directories beginnning with "." or "_" or "testdata"Fixes #66038
Thanks, I've updated the PR description in GitHub. I left the paragraph in describing the reasoning behind the tests added, do you think there's value in this or should I remove it too?
Not a problem; I think the paragraph below explaining your rationale is useful, please keep it. Thank you
if relToWorkingDir == "." || hasAnyPrefix(relToWorkingDir, ".."+string(filepath.Separator)) {
// Not a special directory so can return immediately
return matchPath(rel)
}
// Otherwise avoid special directories "testdata" or beginning with ".", "_".
pathComponents := strings.Split(relToWorkingDir, string(filepath.Separator))One more nit that I overlooked: could we please extract string(filepath.Separator) to a variable like this:
```go
sep := string(filepath.Separator)
if relToWorkingDir == "." || hasAnyPrefix(relToWorkingDir, ".."+sep) {
// Not a special directory so can return immediately
return matchPath(rel)
}
// Otherwise avoid special directories "testdata" or beginning with ".", "_".
- Ian Lance Taylor
- Matthew Hughes
- Michael Matloob
- Sam Thanawalla
- Than McIntosh
Code-Review
Legacy-TryBots-Pass
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Matthew Hughes (Gerrit)
Matthew Hughes added 2 comments![]( "Open in Gerrit")
Matthew HughesThanks for the review, I had some follow-up questions from it
whoops, made this comment in the wrong place...was meant to be on the reply to the view
I spotted some possible problems.
These findings are based on simple heuristics. If a finding appears wrong, briefly reply here saying so. Otherwise, please address any problems and update the GitHub PR. When complete, mark this comment as 'Done' and click the [blue 'Reply' button](https://go.dev/wiki/GerritBot#i-left-a-reply-to-a-comment-in-gerrit-but-no-one-but-me-can-see-it) above.
Possible problems detected:
1. Are you using markdown? Markdown should not be used to augment text in the commit message.
2. It looks like you have a properly formated bug reference, but the convention is to put bug references at the bottom of the commit message, even if a bug is also mentioned in the body of the message.The commit title and commit message body come from the GitHub PR title and description, and must be edited in the GitHub web interface (not via git). For instructions, see [here](https://go.dev/wiki/GerritBot/#how-does-gerritbot-determine-the-final-commit-message). For guidelines on commit messages for the Go project, see [here](https://go.dev/doc/contribute#commit_messages).
(In general for Gerrit code reviews, the change author is expected to [log in to Gerrit](https://go-review.googlesource.com/login/) with a Gmail or other Google account and then close out each piece of feedback by marking it as 'Done' if implemented as suggested or otherwise reply to each review comment. See the [Review](https://go.dev/doc/contribute#review) section of the Contributing Guide for details.)
- Ian Lance Taylor
Gerrit Bot (Gerrit)
Gerrit Bot uploaded new patchset![]( "Open in Gerrit")
- Legacy-TryBots-Pass: TryBot-Result-1 by Gopher Robot, Run-TryBot+1 by Emmanuel Odeke
Related details
- Emmanuel Odeke
- Ian Lance Taylor
- Michael Matloob
- Sam Thanawalla
- Than McIntosh
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Matthew Hughes (Gerrit)
Matthew Hughes added 1 comment![]( "Open in Gerrit")
Gopher RobotTryBots beginning. Status page: https://farmer.golang.org/try?commit=4f35b335
Matthew Hughes1 of 48 TryBots failed.
Failed on openbsd-amd64-72: https://storage.googleapis.com/go-build-log/4f35b335/openbsd-amd64-72_1c3aaacc.logConsult https://build.golang.org/ to see whether they are new failures. Keep in mind that TryBots currently test *exactly* your git commit, without rebasing. If your commit's git parent is old, the failure might've already been fixed.
I see similar looking failures on older builds. I've rebased my changed on master (it looks like recent builds are running ok on this arch)
Related details
- Emmanuel Odeke
- Ian Lance Taylor
- Michael Matloob
- Sam Thanawalla
- Than McIntosh
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Than McIntosh (Gerrit)
Than McIntosh voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Emmanuel Odeke
- Ian Lance Taylor
- Michael Matloob
- Sam Thanawalla
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Matthew Hughes (Gerrit)
Matthew Hughes added 1 comment![]( "Open in Gerrit")
if relToWorkingDir == "." || hasAnyPrefix(relToWorkingDir, ".."+string(filepath.Separator)) {
// Not a special directory so can return immediately
return matchPath(rel)
}
// Otherwise avoid special directories "testdata" or beginning with ".", "_".
pathComponents := strings.Split(relToWorkingDir, string(filepath.Separator))One more nit that I overlooked: could we please extract string(filepath.Separator) to a variable like this:
```go
sep := string(filepath.Separator)
if relToWorkingDir == "." || hasAnyPrefix(relToWorkingDir, ".."+sep) {
// Not a special directory so can return immediately
return matchPath(rel)
}// Otherwise avoid special directories "testdata" or beginning with ".", "_".
pathComponents := strings.Split(relToWorkingDir, sep)
```
sorry! I managed to miss the comment when looking over your review. Addressed this with the latest change
Related details
- Emmanuel Odeke
- Ian Lance Taylor
- Michael Matloob
- Sam Thanawalla
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gerrit Bot (Gerrit)
Gerrit Bot uploaded new patchset![]( "Open in Gerrit")
- Emmanuel Odeke
- Ian Lance Taylor
- Michael Matloob
- Sam Thanawalla
- Than McIntosh
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Emmanuel Odeke (Gerrit)
Emmanuel Odeke voted and added 1 comment![]( "Open in Gerrit")
Votes added by Emmanuel Odeke
| Code-Review | +1 |
1 comment
if relToWorkingDir == "." || hasAnyPrefix(relToWorkingDir, ".."+string(filepath.Separator)) {
// Not a special directory so can return immediately
return matchPath(rel)
}
// Otherwise avoid special directories "testdata" or beginning with ".", "_".
pathComponents := strings.Split(relToWorkingDir, string(filepath.Separator))Matthew HughesOne more nit that I overlooked: could we please extract string(filepath.Separator) to a variable like this:
```go
sep := string(filepath.Separator)
if relToWorkingDir == "." || hasAnyPrefix(relToWorkingDir, ".."+sep) {
// Not a special directory so can return immediately
return matchPath(rel)
}// Otherwise avoid special directories "testdata" or beginning with ".", "_".
pathComponents := strings.Split(relToWorkingDir, sep)
```
sorry! I managed to miss the comment when looking over your review. Addressed this with the latest change
Not quite addressed: sep := string(filepath.Separator) removes the need to always invoke string(sep) every time.
Related details
- Matthew Hughes
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Matthew Hughes (Gerrit)
Matthew Hughes added 1 comment![]( "Open in Gerrit")
if relToWorkingDir == "." || hasAnyPrefix(relToWorkingDir, ".."+string(filepath.Separator)) {
// Not a special directory so can return immediately
return matchPath(rel)
}
// Otherwise avoid special directories "testdata" or beginning with ".", "_".
pathComponents := strings.Split(relToWorkingDir, string(filepath.Separator))Matthew HughesOne more nit that I overlooked: could we please extract string(filepath.Separator) to a variable like this:
```go
sep := string(filepath.Separator)
if relToWorkingDir == "." || hasAnyPrefix(relToWorkingDir, ".."+sep) {
// Not a special directory so can return immediately
return matchPath(rel)
}// Otherwise avoid special directories "testdata" or beginning with ".", "_".
pathComponents := strings.Split(relToWorkingDir, sep)
```
Emmanuel Odekesorry! I managed to miss the comment when looking over your review. Addressed this with the latest change
Not quite addressed: sep := string(filepath.Separator) removes the need to always invoke string(sep) every time.
yep, I just realised I missed that 😄, fixed up with latest patch
Related details
- Emmanuel Odeke
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gerrit Bot (Gerrit)
Gerrit Bot uploaded new patchset![]( "Open in Gerrit")
- Emmanuel Odeke
- Ian Lance Taylor
- Michael Matloob
- Sam Thanawalla
- Than McIntosh
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Emmanuel Odeke (Gerrit)
Emmanuel Odeke voted and added 1 comment![]( "Open in Gerrit")
Votes added by Emmanuel Odeke
| Code-Review | +1 |
| Commit-Queue | +1 |
| Run-TryBot | +1 |
1 comment
if relToWorkingDir == "." || hasAnyPrefix(relToWorkingDir, ".."+string(filepath.Separator)) {
// Not a special directory so can return immediately
return matchPath(rel)
}
// Otherwise avoid special directories "testdata" or beginning with ".", "_".
pathComponents := strings.Split(relToWorkingDir, string(filepath.Separator))Matthew HughesOne more nit that I overlooked: could we please extract string(filepath.Separator) to a variable like this:
```go
sep := string(filepath.Separator)
if relToWorkingDir == "." || hasAnyPrefix(relToWorkingDir, ".."+sep) {
// Not a special directory so can return immediately
return matchPath(rel)
}// Otherwise avoid special directories "testdata" or beginning with ".", "_".
pathComponents := strings.Split(relToWorkingDir, sep)
```
Emmanuel Odekesorry! I managed to miss the comment when looking over your review. Addressed this with the latest change
Matthew HughesNot quite addressed: sep := string(filepath.Separator) removes the need to always invoke string(sep) every time.
yep, I just realised I missed that 😄, fixed up with latest patch
- Ian Lance Taylor
- Matthew Hughes
- Michael Matloob
- Sam Thanawalla
- Than McIntosh
Code-Review
Legacy-TryBots-Pass
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Matloob (Gerrit)
Michael Matloob added 1 comment![]( "Open in Gerrit")
This also affects the behavior of -gcflags and the other per package flags, right? That's probably the behavior we want but I want to double check that we're doing the right thing. I think this could be relevant because we do allow leading dots in import paths (as long as they're not in the module name https://github.com/golang/go/issues/43985) even though we won't match them when doing builds. So potentially I think (and I know this is a rare case) if a user is building something that imports a leading dot package and want to set gcflags for it, that package will be ignored.
Related details
- Emmanuel Odeke
- Ian Lance Taylor
- Matthew Hughes
- Sam Thanawalla
- Than McIntosh
Code-Review
Legacy-TryBots-Pass
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Matthew Hughes (Gerrit)
Matthew Hughes added 1 comment![]( "Open in Gerrit")
This also affects the behavior of -gcflags and the other per package flags, right? That's probably the behavior we want but I want to double check that we're doing the right thing. I think this could be relevant because we do allow leading dots in import paths (as long as they're not in the module name https://github.com/golang/go/issues/43985) even though we won't match them when doing builds. So potentially I think (and I know this is a rare case) if a user is building something that imports a leading dot package and want to set gcflags for it, that package will be ignored.
This also affects the behavior of -gcflags and the other per package flags, right?
Yes I believe it will
That's probably the behavior we want but I want to double check that we're doing the right thing
Maybe it's worth looking to see if we can limit this functionality to just the -coverpkg flag?
Related details
- Emmanuel Odeke
- Ian Lance Taylor
- Michael Matloob
- Sam Thanawalla
- Than McIntosh
Gerrit Bot (Gerrit)
Gerrit Bot uploaded new patchset![]( "Open in Gerrit")
- Legacy-TryBots-Pass: TryBot-Result+1 by Gopher Robot, Run-TryBot+1 by Emmanuel Odeke
- TryBots-Pass: LUCI-TryBot-Result-1 by Go LUCI
Related details
- Emmanuel Odeke
- Ian Lance Taylor
- Michael Matloob
- Sam Thanawalla
- Than McIntosh
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gerrit Bot (Gerrit)
Gerrit Bot uploaded new patchset![]( "Open in Gerrit")
Related details
- Emmanuel Odeke
- Ian Lance Taylor
- Michael Matloob
- Sam Thanawalla
- Than McIntosh
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Matthew Hughes (Gerrit)
Matthew Hughes added 1 comment![]( "Open in Gerrit")
Matthew HughesThis also affects the behavior of -gcflags and the other per package flags, right? That's probably the behavior we want but I want to double check that we're doing the right thing. I think this could be relevant because we do allow leading dots in import paths (as long as they're not in the module name https://github.com/golang/go/issues/43985) even though we won't match them when doing builds. So potentially I think (and I know this is a rare case) if a user is building something that imports a leading dot package and want to set gcflags for it, that package will be ignored.
This also affects the behavior of -gcflags and the other per package flags, right?
Yes I believe it will
That's probably the behavior we want but I want to double check that we're doing the right thing
Maybe it's worth looking to see if we can limit this functionality to just the -coverpkg flag?
I've added the latest change (which also rebased on top of master) to limit the changes in package matching to just the -coverpkg flag
Related details
- Emmanuel Odeke
- Ian Lance Taylor
- Michael Matloob
- Sam Thanawalla
- Than McIntosh
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Than McIntosh (Gerrit)
Than McIntosh voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Emmanuel Odeke
- Ian Lance Taylor
- Michael Matloob
- Sam Thanawalla
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gerrit Bot (Gerrit)
Gerrit Bot uploaded new patchset![]( "Open in Gerrit")
- TryBots-Pass: LUCI-TryBot-Result+1 by Go LUCI
- Emmanuel Odeke
- Ian Lance Taylor
- Michael Matloob
- Sam Thanawalla
- Than McIntosh
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gerrit Bot (Gerrit)
Gerrit Bot uploaded new patchset![]( "Open in Gerrit")
Related details
- Emmanuel Odeke
- Ian Lance Taylor
- Michael Matloob
- Sam Thanawalla
- Than McIntosh
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Funda Secgin (Gerrit)
Funda Secgin voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Emmanuel Odeke
- Ian Lance Taylor
- Michael Matloob
- Sam Thanawalla
- Than McIntosh
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Funda Secgin (Gerrit)
Funda Secgin added 2 comments![]( "Open in Gerrit")
Gopher RobotTryBots beginning. Status page: https://farmer.golang.org/try?commit=4f35b335
Matthew Hughes1 of 48 TryBots failed.
Failed on openbsd-amd64-72: https://storage.googleapis.com/go-build-log/4f35b335/openbsd-amd64-72_1c3aaacc.logConsult https://build.golang.org/ to see whether they are new failures. Keep in mind that TryBots currently test *exactly* your git commit, without rebasing. If your commit's git parent is old, the failure might've already been fixed.
I see similar looking failures on older builds. I've rebased my changed on master (it looks like recent builds are running ok on this arch)
Done
func TestCoverpkgIngoresSpecialDirs(t *testing.T) {Matthew HughesIs there a chance you can write this as a script test instead of a hand-coded test? For this sort of scenario (where you have to build a program and do testing) the script test would probably be a lot more readable. If using the script engine doesn't work for some reason we can fall back to this.
Than McIntoshAh, I didn't know script tests were a thing. From a glance these look to be under src/cmd/go/testdata/script, is that correct? If so I'll have a look over there and try and build something
Matthew HughesYes, testdata/script/<testcase>.txt.
You could use testdata/script/cover_swig.txt or testdata/script/cover_coverpkg_with_init.txt as examples.
Yes, testdata/script/<testcase>.txt.
You could use testdata/script/cover_swig.txt or testdata/script/cover_coverpkg_with_init.txt as examples.
Thanks I gave this a shot with the latest patchset
- Emmanuel Odeke
- Ian Lance Taylor
- Matthew Hughes
- Michael Matloob
- Sam Thanawalla
- Than McIntosh
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Matthew Hughes (Gerrit)
Matthew Hughes added 1 comment![]( "Open in Gerrit")
I've just rebased these changes on latest master (didn't require any changes/conflict resolutions0. I think I've addressed all the feedback, but let me know if there's anything I've missed/anything else outstanding on this changeset
Related details
- Emmanuel Odeke
- Ian Lance Taylor
Gerrit Bot (Gerrit)
Gerrit Bot uploaded new patchset![]( "Open in Gerrit")
- Emmanuel Odeke
- Funda Secgin
- Ian Lance Taylor
- Michael Matloob
- Sam Thanawalla
- Than McIntosh
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Matloob (Gerrit)
Michael Matloob added 1 comment![]( "Open in Gerrit")
// Otherwise avoid special directories "testdata" or beginning with ".", "_".
pathComponents := strings.Split(rel, sep)
for _, elem := range pathComponents {
if hasAnyPrefix(elem, ".", "_") || elem == "testdata" {
return true
}
}We should try to keep the logic here in line with the logic in modload.matchPackages (look for the comment `// Avoid .foo, _foo, and testdata subdirectory trees.`)
We'll need to also make sure that we avoid 'ignored' directories (see the `ignorePatternsMap` code in that block referenced above)
Related details
- Emmanuel Odeke
- Funda Secgin
- Ian Lance Taylor
- Sam Thanawalla
- Than McIntosh
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |