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

x/tools/go/analysis/analysistest: output and golden file differ, but RunWithSuggestedFixes does not report failure #47118

Closed
nishanths opened this issue Jul 10, 2021 · 7 comments

Comments

@nishanths
Copy link

@nishanths nishanths commented Jul 10, 2021

What version of Go are you using (go version)?

$ go version
go version go1.16.5 darwin/amd64

Does this issue reproduce with the latest release?

Yes.

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/nishanthshanmugham/Library/Caches/go-build"
GOENV="/Users/nishanthshanmugham/Library/Application Support/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOINSECURE=""
GOMODCACHE="/Users/nishanthshanmugham/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="/Users/nishanthshanmugham/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/darwin_amd64"
GOVCS=""
GOVERSION="go1.16.5"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/Users/nishanthshanmugham/src/analysistestissue/go.mod"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -arch x86_64 -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/xr/2qd98lwx3l709x7jt26376qr0000gn/T/go-build2078020356=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

See a minimal example at https://github.com/nishanths/analysistest-fmt-issue, along with an explanation in the README.

I originally discovered the issue doing the following:

  • Wrote an analysis.Analyzer that (inadvertently) produces broken suggested fixes (produces invalid Go code).
  • Ran a test using analysistest.RunWithSuggestedFixes.
  • The output—after suggested fixes were applied—did not match the golden file. But the test reported PASS.

What did you expect to see?

I would have expected analysistest.RunWithSuggestedFixes to report a failure.

What did you see instead?

The test passed.

@gopherbot gopherbot added this to the Unreleased milestone Jul 10, 2021
@nishanths
Copy link
Author

@nishanths nishanths commented Jul 10, 2021

Currently in two locations in RunWithSuggestedFixes() in analysistest.go, when format.Source() returns a non-nil error the code quietly continues on to the next file. So, though the output and the golden file don't match in this scenario, a failure isn't reported.

	formatted, err := format.Source([]byte(out))
	if err != nil {
		continue
	}

Calling t.Errorf when format.Source() fails should fix the issue. A formatting error means that the Analyzer is producing broken code, which should be reported as a failure. (And I can't think of a reason it shouldn't be reported as a test failure.) Edit: see below for better possible fixes.

I can submit a change if there's agreement on the fix.

Loading

@seankhliao
Copy link
Contributor

@seankhliao seankhliao commented Jul 12, 2021

Loading

@timothy-king
Copy link
Contributor

@timothy-king timothy-king commented Jul 14, 2021

Skipping on err != nil goes back to when RunWithSuggestedFixes() was introduced: https://go-review.googlesource.com/c/tools/+/224959 . @matloob or @stamblerre reviewed this and may recall the history of why this behavior was chosen.

An alternative fix is to return an error whenever want != string(formatted). I don't have a compelling reason to prefer this vs returning the error. Maybe we are okay with producing a suggestion that is not valid Go code?

Loading

@nishanths
Copy link
Author

@nishanths nishanths commented Jul 16, 2021

Thank you for looking into this issue.

An alternative fix is to return an error whenever ...

To clarify, by "return an error", did you mean "call t.Errorf"?

Maybe we are okay with producing a suggestion that is not valid Go code?

I think it's worth keeping this option open for the future. In this case, I don't think we should fail the test if format.Source fails, as I had somewhat precipitously suggested in my earlier comment.

Instead we should call format.Source, and either ignore the error or t.Logf() the error. But definitely not continue upon error as the code does currently. If we do this, the subsequent want != string(formatted) check and its associated t.Errorf() would naturally report the mismatch between the program's output and the golden file, I think.

Loading

@timothy-king
Copy link
Contributor

@timothy-king timothy-king commented Jul 16, 2021

To clarify, by "return an error", did you mean "call t.Errorf"?

Not quite what I had in mind. But your suggestion makes more sense in context.

Here is roughly my understanding of this:

formatted, err := format.Source([]byte(out))
if err != nil {
	t.Errorf("failed to format source code: %s", err)
	continue
}

Instead we should call format.Source, and either ignore the error or t.Logf() the error. But definitely not continue upon error as the code does currently.

Here is roughly my understanding of this:

formatted, err := format.Source([]byte(out))
if err != nil {
	t.Logf("failed to format source code: %s", err)
	formatted = out
}

I think both make sense. I am really interested if we know why we chose to skip reporting an error historically.

Loading

@matloob
Copy link
Contributor

@matloob matloob commented Jul 20, 2021

Looking at the CL it seems like not reporting an error was an oversight. I can't think of a good reason to just ignore that case

Loading

@nishanths
Copy link
Author

@nishanths nishanths commented Nov 25, 2021

https://golang.org/cl/351552/ has fixed this issue by calling t.Errorf if formatting fails.

Loading

@nishanths nishanths closed this Nov 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants