-
Notifications
You must be signed in to change notification settings - Fork 70
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
testscript: remove our code coverage mechanism thanks to Go 1.20 #201
Conversation
I also think this means that our |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Brilliant stuff, thanks! So glad that these hacks can go.
One thing: I wonder if we should remove the flag.Parse
invocation inside exe.go
.
We do add the testWork
flag, but go test
will itself call flag.Parse
, and we don't need the value of the flag until the tests have actually started running (unlike before where we needed the coverage-related flags earlier).
Our code was a fairly hacky version of what Go 1.20 does for us, since we had to externally reach into the testing internals to do the right thing before and after each program execution. With Go 1.20, all we actually need to do is ensure that the GOCOVERDIR environment variable is forwarded properly. With that, test binaries will know how to produce multiple coverage profiles, and "go test" will know how to collect and merge them. We could keep our hacky workaround for the sake of "deep" coverage information for Go 1.19, but that doesn't seem worthwhile. The old mechanism caused test flakes like rogpeppe#130, is incompatible with Go 1.20, and is overall worse than what Go 1.20 can do. If a user wants code coverage with the latest testscript version, they can use Go 1.20. If they are stuck on Go 1.19 and need code coverage, I imagine they can also stick to a slightly older testscript version. On a main package, the old testscript with Go 1.19 reports: PASS coverage: 8.0% of statements total coverage: 90.1% of statements ok mvdan.cc/sh/v3/cmd/shfmt 0.063s The new testscript with Go 1.20 reports: PASS mvdan.cc/sh/v3/cmd/shfmt coverage: 90.1% of statements ok mvdan.cc/sh/v3/cmd/shfmt 0.047s Fixes rogpeppe#130, as the RemoveAll call is no longer present. Fixes rogpeppe#161, as the API is now deprecated. Fixes rogpeppe#199, as "go test -coverprofile" now works on Go 1.20.
You're absolutely right - I had spotted the |
Does this mean the next release of
How would we specify that in the commands map? func TestMain(m *testing.M) {
os.Exit(testscript.RunMain(m, map[string]func() {
"hello": main.main, // 🤷
}))
} |
It still works with 1.18 and 1.19 (CI is green, after all), but the "deep" code coverage will only work correctly on 1.20.
I guess you're right - using |
Thanks, with this commit testscript now works in my tests and no coverage was lost. I do see occasional errors like this:
But it's hard to tell if this is a bad interaction between Go & testscript, or an issue with Go 1.20 itself. |
If you keep seeing that, please file a separate issue. I haven't seen that one myself, yet. |
By this do you mean that the results of That would be a shame, since this is one of the things that usually gets people excited about |
I describe the reasoning behind the decision in the commit message, have you seen that? |
Thanks for pointing me to the commit message—and, as a side note, if that message had been included in the PR description, I wouldn't have needed to ask the questions! If I'm interpreting it correctly, then your answers to my two questions are, effectively:
Yes.
Yes, but we're not going to. Understood, thank you—the reason I ask is that I anticipate many people (readers of my tutorials and book, for example) will write programs using The advice I should give them, I understand, is to explicitly import an older version of |
I do include "see the commit message" in the PR body, but it's relatively easy to miss. GitHub's PR UI is just bad with commit messages. I used to also copy commit messages into PR bodies when creating them from the command line, but that was worse, because any updates to commit messages weren't reflected in the PR body. Plus it doesn't really work past one commit.
Personally, I think it's fine to simply recommend the latest stable version of testscript, just like you would recommend the latest stable version of Go. You could make a note that the latest testscript will not have full code coverage on Go versions before 1.19, and suggest a slightly older testscript version for those stuck on 1.19, but I don't think that should matter to the majority of users. Go 1.20 has been out for ten days now, and we haven't done a testcript release yet. |
As a user I will second that the main reason I adopted testscript is to get coverage on CLIs Consider making the go.mod say 1.20 min for 1.10.1 and retracting 1.10.0 because right now it does stuff like this: ps: pretty sure I'm not the only one who doesn't jump and adopt the next version of go right away, there are always a ton of issue with linters, tooling etc.. as well as bug so typically 1.20.3 would be the first version I'd switch away from 1.19.7 - in other words, it's being "stuck" with 1.19, it's being safe and not wanting to be on bleeding edge ps2: not to say this MR isn't great, red diffs and using built in support is fantastic, just that the incompatibility with go 1.19 should be highlighted and I believe avoided/clarified for dependabot style updates using 1.20 in go.mod |
I don't really agree with specifying Go 1.20 as our minimum version. testscript works OK with Go 1.19, it's just that full test coverage (which was always an extra feature, and was a bit wonky as it caused flakes like #130) now only works with 1.20 or later. I understand that some users may still be on Go 1.19 as their main version, but they can also remain on the previous release of go-internal. Note that 1.20 came out nearly two months ago at this point; we are not rushing anything in my opinion. If someone is eagerly updating dependency versions, I would hope that they are eagerly updating their Go version as well; otherwise that's not consistent :) As I described in my previous comments, I wouldn't minded keeping support with 1.19 for another release or two if it were easy, but the old code was quite complex and buggy, so I leaned against it. |
Getting bug fixes and using dependabot to get dependency isn't inconsistent with staying 1 behind bleeding edge go version Ok, maybe a compromise is editing https://github.com/rogpeppe/go-internal/releases/tag/v1.10.0 to highlight/spell out you will lose coverage if using go < 1.20 ? |
I'm curious, why do you call Go stable "bleeding edge"? Go does have pre-releases and development versions (master); 1.20 is not bleeding edge by any means. |
.0 is never "stable" a lot of the tooling doesn't work despite prereleases etc and best effort issues are always discovered during wider adoption. |
unnecessary since rogpeppe/go-internal#201
(see commit message)
Fixes #130, as the RemoveAll call is no longer present.
Fixes #161, as the API is now deprecated.
Fixes #199, as "go test -coverprofile" now works on Go 1.20.