leighmcculloch / gochecknoglobals

Check that no globals are present in Go code.
MIT License
104 stars 11 forks source link

Ignore variables in test packages/files #11

Closed kaskavalci closed 4 years ago

kaskavalci commented 5 years ago

It is possible to initialize test variables by TestMain. However global variables are the only way to share data with TestMain and tests. Can we ignore global variables in _test packages or _test.go files in general?

leighmcculloch commented 5 years ago

That can cause side effects to leak from one test to the next though. Could you share an example of when a global variable is required for tests instead of a function that returns a setup to be shared across tests?

GregoryVds commented 5 years ago

Facing the same issue here. My setup is expensive to prepare (loading a BPF program into the kernel). I want to perform this setup step only once and then reuse it across several tests. So I basically get a file descriptor in TestMain that I want to share with my test cases.

leonklingele commented 5 years ago

gochecknoglobals lints tests only when a -t flag is provided. Why not run it without the flag? Otherwise (*testing.T).Run might be of help.

GregoryVds commented 5 years ago

Thanks for the suggestion leon, I didn't know about this flag. I am actually using golangci-lintto run gochecknoglobals and it looks like it does not expose the -t flag. Anyway, I ended up using an exclude-rule to turn off gochecknoglobals on the single test file that was problematic, so all good now.

bombsimon commented 4 years ago

A bit late to the party but golangci-lint has it's own flag for this but defaults to true. Have a look at --tests=false or --skip-files=.*_tests

leighmcculloch commented 4 years ago

It's also worth noting golangci-lint ports this linter into its code, so it does not actually call this gochecknoglobals implementation and it's implementation is based on an older version of this tool. There's an issue discussing that here https://github.com/golangci/golangci-lint/issues/1393.

leighmcculloch commented 4 years ago

I think the two recommendations above not using the -t flag, or using golangci-lint's --tests flag, solve the issue posted, so I am closing. Please reopen if this issue is not addressed or I am missing something.

bbkane commented 1 year ago

I'm a bit late to this party, but I'm using the "golden files" pattern ( https://speakerdeck.com/mitchellh/advanced-testing-with-go?slide=19 ) for my tests (see my actual code here). I really want to add gochecknoglobals for all code (tests included) in golangci-lint, but I'm not sure how to restructure my code to "unglobal" the update flag or have gochecknoglobals ignore this one specific flag in just this one test file. Any ideas? From this discussion it looks my options are either to:

Any ideas? And thanks for this tool, I love this idea!

leighmcculloch commented 1 year ago

@bbkane There's a couple options I'm aware of. Others might chime in with additional ideas. Here's what I know:

You can use //nolint:gochecknoglobals. (I haven't used this myself, so I think I have the syntax right, but you might need to fiddle. I've seen it before here: https://golangci-lint.run/usage/false-positives/).

Another option is to put that var update = into its own file, and then stop linting that one file. You only need to put the definition into its own file, not the uses of the variable.

bbkane commented 1 year ago

That seems to work, thank you!

//nolint:gochecknoglobals  // https://github.com/leighmcculloch/gochecknoglobals/issues/11#issuecomment-1368578131
var update = stdlibflag.Bool("update", false, "update golden files")

Although if you (or anyone) hear about any techniques to write an update testing flag without using this package level variable trick, please update this issue.