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

testscript: IgnoreMissedCoverage is unused #161

Closed
FiloSottile opened this issue Jun 21, 2022 · 2 comments · Fixed by #201
Closed

testscript: IgnoreMissedCoverage is unused #161

FiloSottile opened this issue Jun 21, 2022 · 2 comments · Fixed by #201

Comments

@FiloSottile
Copy link
Contributor

It doesn't look like Params.IgnoreMissedCoverage and IgnoreMissedCoverage() are wired up to anything, and indeed code paths that end with os.Exit are missing from the coverage report without an error being raised.

@mvdan
Copy link
Collaborator

mvdan commented Jun 21, 2022

Looks like I'm to blame here; ignoreMissedCoverage was used until my refactor in dc4b495.

@mvdan
Copy link
Collaborator

mvdan commented Jun 21, 2022

In the previous mechanism, each command ran as a sub-process and was expected to produce exactly one coverage profile. This is why the boolean option mattered: if the sub-process failed to produce the coverage file, by default we would error, unless the option was enabled.

The new mechanism allows command sub-processes to produce multiple coverage files, for the sake of collecting and merging all of them. This allows us to measure the code coverage of Go tools or programs which execute themselves as children, akin to go build.

However, swapping the "must produce 1 file" for "may produce any number of files", I failed to add back the requirement that at least one file should be produced - since each command spawns at least one Go sub-process. When that is fixed, then IgnoreMissedCoverage has a purpose again.

It's worth noting that, right now, TESTSCRIPT_COVER_DIR is shared across all commands and their invocations, so we can't simply check whether the directory is not empty. One option is to use a separate directory per command invocation, mimicking the old behavior with coverFilename.

mvdan added a commit to mvdan/go-internal that referenced this issue Feb 8, 2023
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
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
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 mvdan closed this as completed in f0583b8 Feb 9, 2023
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 a pull request may close this issue.

2 participants