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

SA5011 false positive in tests that use gotest.tools "assert" #1022

Closed
thaJeztah opened this issue May 31, 2021 · 4 comments
Closed

SA5011 false positive in tests that use gotest.tools "assert" #1022

thaJeztah opened this issue May 31, 2021 · 4 comments

Comments

@thaJeztah
Copy link

@thaJeztah thaJeztah commented May 31, 2021

When linting the file below, static check fails to detect the assert.Assert(t, loadedConfig != nil) as a guard against nil values, causing linting to fail;

https://play.golang.org/p/ETtTE7jResx

package main

import (
	"testing"

	"gotest.tools/v3/assert"
)

func TestFoo(t *testing.T) {
	loadedConfig, err := foo()
	assert.NilError(t, err)

	assert.Assert(t, loadedConfig != nil)
	assert.Check(t, loadedConfig.SomeProp)

	assert.Check(t, loadedConfig != nil)
	assert.Check(t, loadedConfig.SomeProp)
}

type Config struct {
	SomeProp bool
}

func foo() (*Config, error) {
	return &Config{SomeProp: true}, nil
}

When running with "plain" staticcheck:

foo_test.go:14:31: possible nil pointer dereference (SA5011)
	foo_test.go:16:18: this check suggests that the pointer can be nil
foo_test.go:17:31: possible nil pointer dereference (SA5011)
	foo_test.go:16:18: this check suggests that the pointer can be nil

When running as part of golangci-lint:

foo/foo_test.go:14:31: SA5011: possible nil pointer dereference (staticcheck)
	assert.Check(t, loadedConfig.SomeProp)
	                             ^
foo/foo_test.go:16:18: SA5011(related information): this check suggests that the pointer can be nil (staticcheck)
	assert.Check(t, loadedConfig != nil)
	                ^
foo/foo_test.go:17:31: SA5011: possible nil pointer dereference (staticcheck)
	assert.Check(t, loadedConfig.SomeProp)
	                             ^

Staticcheck should detect that the assert.Assert (or assert.Check) is gating this situation.

@dominikh
Copy link
Owner

@dominikh dominikh commented May 31, 2021

Funny, I've just closed #832 and now you're telling me there's an entire testing framework built around this pattern…

I guess this can be the new issue for this.

@dominikh
Copy link
Owner

@dominikh dominikh commented May 31, 2021

The easiest workaround is probably to only consider those x != nil comparisons that appear in the context of an if statement. That way, we won't observe comparisons used as arguments to assert-style functions, and have no reason to flag the subsequent dereference.

@thaJeztah
Copy link
Author

@thaJeztah thaJeztah commented May 31, 2021

Funny, I've just closed #832 and now you're telling me there's an entire testing framework built around this pattern…

Ah, I think I saw that one when searching, and now reading the linked issue.

Yes, I can see it being somewhat complicated to take all "possible" testing frameworks into account. Thought I'd at least open a ticket for it, in case that's something you'd normally do.

I think in our case either excluding this check for tests would probably be ok (worst case: we run into a panic in a test), or just removing the nil check (there's already a check if no error occurred, and fields are validated further down, so the nil check looks like it's mostly to "be explicit".

@cbandy
Copy link

@cbandy cbandy commented Jun 1, 2021

assert.Assert calls t.FailNow (stops execution) while assert.Check calls t.Fail (continues execution).

I find that this staticcheck rule/check doesn't bite me too often. When it does, however, I find this pattern quiets it:

if assert.Check(t, thing != nil) {
  assert.Assert(t, thing.Field)
}

@dominikh dominikh closed this in e317c24 Jun 2, 2021
dominikh added a commit that referenced this issue Aug 16, 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
3 participants