AT001 Can't use lintignore on TestCase

[armsnyder]

Community Note

• Please vote on this issue by adding a 👍 reaction to the original issue to help the community and maintainers prioritize this request
• Please do not leave "+1" or other comments that do not add relevant new information or questions, they generate extra noise for issue followers and do not help prioritize the request
• If you are interested in working on this issue or have submitted a pull request, please leave a comment

tfproviderlint and terraform-plugin-sdk Version

$ tfproviderlint -version

$ go mod graph | cut -d' ' -f 1 | grep terraform-plugin-sdk | uniq

github.com/bflad/tfproviderlint v0.18.0

github.com/hashicorp/terraform-plugin-sdk/v2@v2.0.0
github.com/hashicorp/terraform-plugin-sdk@v1.9.0
github.com/hashicorp/terraform-plugin-sdk@v1.15.0

Affected Check(s) or Function(s)

• AT001

Expected Behavior

I expect to be able to use lintignore as documented in AT001 in either of these two ways:

//lintignore:AT001
resource.Test(t, resource.TestCase{
	Steps: []resource.TestStep{},
})

resource.Test(t, resource.TestCase{ //lintignore:AT001
	Steps: []resource.TestStep{},
})

Actual Behavior

Neither of the above two samples work. The check is not ignored for the TestCase.

Workaround

I was able to ignore the check this way:

resource.Test(t, resource.TestCase{ 
	Steps: []resource.TestStep{},
}) //lintignore:AT001

This is a readability problem IMO as it behaves differently that nolint directives do in other linters. Also, there can be a lot of steps in the TestCase, and having the lintignore at the end makes it easy to miss.

[bflad]

Just to provide some insight here, my guess is that this occurs due to how Go builds the AST comment maps and how we filter for ignores in the commentignore pass. We essentially require the code comment to occur within the file positions of the given AST node. Since the resource.Test() represents a node and file positions that wrap the resource.TestCase{}, its likely that this does not play well with this design.

Another workaround is having the resource.TestCase{} as its own variable, which will appropriately be handled by the current ignore comment logic, e.g.

//lintignore:AT001
testCase := resource.TestCase{/* ... */}
resource.Test(t, testCase)

Since the wrapping resource.Test()/resource.ParallelTest() call expression is very common in Terraform Provider development, this is certainly an undesirable workaround though. However, I also worry about trying to introduce the ability to accept certain wrapper AST nodes to adjust the positions, since it could be complex and potentially cause more issues if not implemented carefully.

I won't have time to dive further into this for a bit, but happy to hear thoughts or design proposals in the meantime.