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.

cc @matloob

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?

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.

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.

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

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