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

testscript: RemoveAll flakes after the code coverage refactor #130

Closed
mvdan opened this issue Feb 26, 2021 · 1 comment · Fixed by #201
Closed

testscript: RemoveAll flakes after the code coverage refactor #130

mvdan opened this issue Feb 26, 2021 · 1 comment · Fixed by #201
Assignees
@mvdan

Comments

@mvdan
Copy link
Collaborator

mvdan commented Feb 26, 2021

See https://github.com/rogpeppe/go-internal/runs/1986747862?check_suite_focus=true, for example:

non-directory found after RemoveAll: $WORK/tmp/testscript-main205091895/cover/foo-2a9d214c00ea
2021/02/26 10:31:15 cannot merge cover profiles: unlinkat $WORK/tmp/testscript-main205091895/cover: directory not empty
exit status 2
FAIL	test	0.308s

These rare flakes started happening since #119 was merged. I'm not sure yet on what the underlying cause is, but I have a couple of theories I need to investigate.
myitcv added a commit that referenced this issue Mar 1, 2021
@myitcv
gotooltest: skip flakey test on darwin
423466a 
The need for a real fix for this is captured in #130
@myitcv myitcv mentioned this issue Mar 1, 2021
Merged
myitcv added a commit that referenced this issue Mar 1, 2021
@myitcv
gotooltest: skip flakey test on darwin (#133)
d03db15 
The need for a real fix for this is captured in #130
@mvdan
Copy link
Collaborator Author

mvdan commented Jun 21, 2022

I think we might be able to fix this issue alongside golang/go#30789 by retrying RemoveAll, just like Go does. I'll look at this soon, as the Windows failures are biting us in CUE:

exe.go:74: cannot delete temporary directory: CreateFile C:\Users\RUNNER~1\AppData\Local\Temp\testscript-main2212622325\bin\cue.exe: Access is denied.

@mvdan mvdan self-assigned this Jun 21, 2022
@mvdan mvdan mentioned this issue Feb 8, 2023
Merged
mvdan added a commit to mvdan/go-internal that referenced this issue Feb 8, 2023
@mvdan
testscript: remove our code coverage mechanism thanks to Go 1.20
26086ed 
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.
mvdan added a commit to mvdan/go-internal that referenced this issue Feb 8, 2023
@mvdan
testscript: remove our code coverage mechanism thanks to Go 1.20
3e931ac 
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.
mvdan added a commit to mvdan/go-internal that referenced this issue Feb 9, 2023
@mvdan
testscript: remove our code coverage mechanism thanks to Go 1.20
b336f0b 
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.
@mvdan mvdan closed this as completed in #201 Feb 9, 2023
mvdan added a commit that referenced this issue Feb 9, 2023
@mvdan
testscript: remove our code coverage mechanism thanks to Go 1.20
f0583b8 
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 #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 #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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@mvdan mvdan

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

testscript: remove our code coverage mechanism thanks to Go 1.20 mvdan/go-internal
1 participant
@mvdan