var-naming doesn't run on exported test functions

[ivanvc]

Hi, thanks for the great work on revive. I checked the documentation and issues from var-naming, but couldn't find anything that explained this behavior. I'm unsure if it is intentional not to check exported test functions.

Describe the bug
The var-naming run doesn't run on exported test functions. i,e,. func TestValidIpAddress(t *testing.T). However, when unexporting the function it runs as expected.

To Reproduce

Steps to reproduce the behavior:

1. Open a new file named var_naming_test.go
2. Write the test function:

   package main
   
   import "testing"
   
   func TestValidIp(t *testing.T) {
   	t.Fatal("not implemented")
   }

3. Run revive (with the default settings), i.e. revive ./... && echo $?

Expected behavior
It should have failed because of the wrong casing in the function name. i.e.,

var_naming_test.go:5:6: func TestValidIp should be TestValidIP

For example, when unexporting the test function (testValidIp) the linter fails as expected.

Logs
If applicable, add screenshots to help explain your problem.

Desktop (please complete the following information):

• OS: Linux
• Version of Go: 1.22.9 / 1.23.3
• Revive version: 1.5.0

Additional context
We first identified this issue in our CI environment (as part of golangci-lint) and I was able to reproduce it locally. Ref: etcd-io/etcd#18816 (comment)

[mfederowicz]

@ivanvc its not a bug its a feature as some one said :P

if you check source of var-naming rule you should notice:

case *ast.FuncDecl:                                                                                                                                                    
           funcName := v.Name.Name                                                                                                                                            
           if w.file.IsTest() &&                                                                                                                                              
                 (strings.HasPrefix(funcName, "Example") ||                                                                                                                     
                       strings.HasPrefix(funcName, "Test") ||                                                                                                                     
                       strings.HasPrefix(funcName, "Benchmark") ||                                                                                                                
                       strings.HasPrefix(funcName, "Fuzz")) {                                                                                                                     
                 return w                                                                                                                                                       
           }

that code check if func name is declared in file with _test.go suffix and has prefix from (Example, Test, Benchmark, Fuzz) then it should be skipped

I made file var_naming_test.go with content:

func CheckTestValidIp(t *testing.T) {                                                                                   
    t.Fatal("not implemented")                                                                                                                                             
  } 

and lint results were like this:

revive ./...
var_naming_test.go:5:6: func CheckTestValidIp should be CheckTestValidIP

but when rename test file to var_naming.go then results looks like that:

revive ./...
var_naming.go:1:1: should have a package comment
var_naming.go:5:1: exported function CheckTestValidIp should have comment or be unexported
var_naming.go:5:6: func CheckTestValidIp should be CheckTestValidIP

so i think it work as expected (func name with prefix Test have special rights in *_test.go files, and they are excluded from lint results)

i hope my answer helps you :)

[ivanvc]

Hi @mfederowicz, thanks for your answer. The problem with skipping test case names is that we can end up with functions with var-naming violations. If we don't want to check these functions (by default), I think adding an option to check them would be a good idea.

I think the idea of a linter is to avoid unnecessary discussion on style if a project adheres to it. For example, refer to the comment that initiated this issue: etcd-io/etcd#18816 (comment).

Thanks again.

[chavacava]

Hi @ivanvc thank you for reporting the issue.
As @mfederowicz said: "it's not a bug but a feature". revive is a drop-in replacement of (the defunct) golint, and var-naming is one of the original golint rules therefore revive implemented it exactly as golint did and golint didn't analyze names on test files.

[ivanvc]

Thanks for explaining the reason behind it, @chavacava. I think the minimum would be to document this behavior so users don't need to dive deep into the source code to understand that it doesn't run on the test file's Test/Benchmark/Fuzz functions.

Cheers, and thanks for the work on the review linter :)

[chavacava]

PR are welcome @ivanvc

[ccoVeille]

Thanks for the discussion. I think I also face the issues a few times.

I have never reached the point to report it.

I'm glad to read the context and the discussion led to document it.

But then, here I am, suggesting something

Maybe this "legacy golint" behavior could be disabled via a configuration parameter for this rule

[mfederowicz]

hmm @chavacava maybe it is not a bad idea to add aditional config flag for example skipCommonTestPrefixes of course by default setted to false, but if someone want to change that, can set to true in config and then all checks like:

strings.HasPrefix(funcName, "Example")                                                                                                                      
strings.HasPrefix(funcName, "Test")                                                                                                                      
strings.HasPrefix(funcName, "Benchmark")                                                                                                                 
strings.HasPrefix(funcName, "Fuzz")

should not bother them :P

but of course give me info what you think about it?

[chavacava]

If a flag is added, it must be backward compatible i.e. its default value shall make the rule behave as it currently behaves (skip tests) thus includeTests seems to me a good flag name candidate.

[mfederowicz]

so includeTests = true by default should check all standard prefixes (Example,Test,Benchmark,Fuzz)
otherwise if we set that to false, then prefixes are ignored and code like:

func TestValidIp(t *testing.T) should return warnings:
var_naming_test.go:5:6: func testValidIp should be testValidIP

thats correct @chavacava? If its ok when back from work, I prepare PR for that :)