Giter VIP home page Giter VIP logo

thelper's Introduction

thelper

CI Go Report Card Coverage MIT License

thelper detects golang test helpers without t.Helper() call. Also, it checks the consistency of test helpers and has similar checks for benchmarks and TB interface. Read further to learn more.

Why?

Why I need to add t.Helper() into my helpers functions?

With this call go test prints correct lines of code for failed tests. Otherwise, printed lines will be inside helpers functions.

Compare (https://goplay.space/#I_pAJ-vWcRq):

func TestBad(t *testing.T) {
	testBad(t)
}

func testBad(t *testing.T) {
	t.Fatal("unhelpful line") // <--- output point this line
}

func TestGood(t *testing.T) {
	testGood(t) // <--- output point this line
}

func testGood(t *testing.T) {
	t.Helper()
	t.Fatal("clear output")
}

Why should I call t.Helper() as the first statement in my test helper functions?

Because t.Helper() only affects asserts that are called after the t.Helper() call, so requiring it to be the first statement helps ensure all assertions in the helper function are affected.

Why do I need to name *testing.T as t? Why it should be the first parameter?

It adds more consistency into your code. When common variables have the same name and are placed into the same position, it makes it easier to understand and read.

Note that you can also have it as the second parameter - this is primarily intended to allow compatibility with context.Context.

Installation

go install github.com/kulti/thelper/cmd/thelper@latest

Usage

golangci-lint

golangci-lint supports thelper, so you can enable this linter and use it.

Shell

Check everything:

thelper ./...

Enable only required checks

If you run via golangci-lint look at .golangci.example.yml for an example of the configuration.

Otherwise you can run thelper with --checks command line argument. E.g. the following command checks that *testing.T is the first param and *testing.B is named b:

thelper --checks=t_first,b_name ./...

Available checks

  • t_begin - t.Helper() should begin helper function.
  • t_name - *testing.T should be named t.
  • t_first - *testing.T should be the first param of helper function.

The same for fuzzing, benchmarks and TB interface:

  • f_begin - f.Helper() should begin helper function.
  • f_name - *testing.F should be named f.
  • f_first - *testing.F should be the first param of helper function.
  • b_begin - b.Helper() should begin helper function.
  • b_name - *testing.B should be named b.
  • b_first - *testing.B should be the first param of helper function.
  • tb_begin - tb.Helper() should begin helper function.
  • tb_name - *testing.TB should be named tb.
  • tb_first - *testing.TB should be the first param of helper function.

Exceptions

  • t_name allows using _ name.
  • t_begin allows subtests and fuzz tests not begin from t.Helper().
  • t_first allows to be the second after context.Context param.

Examples

Without t.Helper()

func TestSmth(t *testing.T) {
	checkSmth(t)
}

// Bad
func checkSmth(t *testing.T) {
	t.Fatal("check failed")
}

// Good
func checkSmth(t *testing.T) {
	t.Helper()
	t.Fatal("check failed")
}

With invalid name

// Bad
func checkSmth(tt *testing.T) {
    // ...
}

// Good
func checkSmth(t *testing.T) {
    // ...
}

With t as not the first param

// Bad
func checkSmth(doneCh <-chan struct{}, t *testing.T) {
    // ...
}

// Good
func checkSmth(t *testing.T, doneCh <-chan struct{}) {
    // ...
}

thelper's People

Contributors

estensen avatar g-rath avatar kulti avatar ldez avatar

Stargazers

 avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar

Watchers

 avatar  avatar  avatar  avatar

thelper's Issues

Cannot disable tb name

I would like to disable the check that suggests testing.TB be named tb (related #27) but I cannot figure out how.

The docs link to .golangci.example.yml which says that all checks are enabled by default. I tried setting tb: { name: false }, tb: { name: true }, and others without any change in behavior. Is it possible to disable a check through golangci-lint?

Files:

// ==> go.mod <==
module tmp

go 1.17

// ==> main_test.go <==
package main

import "testing"

func helper(t testing.TB) { t.Helper() }

func TestHelper(t *testing.T) { helper(t) }
# ==> .golangci.yaml <==
linters:
  disable-all: true
  enable: [thelper]

linters-settings:
  thelper:
    tb_name: false
    tb:
      name: false
$ go test .
ok      tmp     0.002s

$ golangci-lint version
golangci-lint has version 1.43.0 built from 861262b7 on 2021-11-03T11:57:46Z

$ golangci-lint run
main_test.go:5:6: parameter testing.TB should have name tb (thelper)
func helper(t testing.TB) { t.Helper() }
     ^

check usage count for sub-test functions when do t_begin check

If a helper function is a root of the sub-test it will be skipped from checks (#15). So, the following code is ok:

func Test(t *testing.T) {
	t.Run("sub", check)
}

func check(t *testing.T) {
	// ...
}

But if the check function used as a root of different sub-tests or as a non-sub-test root it should be marked with t.Helper.

panic when passing function to t.Run()

Hey folks,

I just upgraded to golangci-lint v1.44.0 which contains thelper v0.5.0. This version of thelper
panics when running it against our repository (ClusterAPI). The previous version (v0.4.0) didn't.

The code block in ClusterAPI that triggers the panic seems to be:

t.Run("for Cluster", customDefaultValidateTest(ctx, c, webhook))

https://github.com/kubernetes-sigs/cluster-api/blob/82ab810646f11ec7c96a1543697264deaa3884ec/internal/webhooks/cluster_test.go#L352

thelper panics because f.Obj is nil here:

		funObjDecl, ok := f.Obj.Decl.(*ast.FuncDecl)

https://github.com/kulti/thelper/blob/master/pkg/analyzer/analyzer.go#L415

How to reproduce

Clone cluster-api (https://github.com/kubernetes-sigs/cluster-api). Checkout commit 4c4b47d

cd into the repo and run:

/thelper ./internal/webhooks/...
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x20 pc=0x129572d]

goroutine 6016 [running]:
github.com/kulti/thelper/pkg/analyzer.unwrapTestingFunctionBuilding(0x12c0700, {0x1373088, 0xc0014aa7c0}, {0x13729a0, 0xc002ddf9e0})
        /Users/buringerst/code/src/github.com/kulti/thelper/pkg/analyzer/analyzer.go:415 +0x6d
github.com/kulti/thelper/pkg/analyzer.extractSubtestExp(0xc002f03520, 0xc0014aa800, {0x137ddf8, 0xc00067dbd0}, {0x13729a0, 0xc002ddf9e0})
        /Users/buringerst/code/src/github.com/kulti/thelper/pkg/analyzer/analyzer.go:394 +0xe5
github.com/kulti/thelper/pkg/analyzer.thelper.run.func1({0x1372158, 0xc0014aa800})
        /Users/buringerst/code/src/github.com/kulti/thelper/pkg/analyzer/analyzer.go:147 +0xb5
golang.org/x/tools/go/ast/inspector.(*Inspector).Preorder(0xc001593878, {0xc00261b650, 0x1520700, 0x1}, 0xc00261b950)
        /Users/buringerst/code/pkg/mod/golang.org/x/[email protected]/go/ast/inspector/inspector.go:77 +0x86
github.com/kulti/thelper/pkg/analyzer.thelper.run({0x12cf480}, 0xc002f03520)
        /Users/buringerst/code/src/github.com/kulti/thelper/pkg/analyzer/analyzer.go:133 +0x40b
golang.org/x/tools/go/analysis/internal/checker.(*action).execOnce(0xc001252000)
        /Users/buringerst/code/pkg/mod/golang.org/x/[email protected]/go/analysis/internal/checker/checker.go:691 +0xa1e
sync.(*Once).doSlow(0x125ec29, 0xc00014c1c0)
        /usr/local/Cellar/go/1.17.2/libexec/src/sync/once.go:68 +0xd2
sync.(*Once).Do(...)
        /usr/local/Cellar/go/1.17.2/libexec/src/sync/once.go:59
golang.org/x/tools/go/analysis/internal/checker.(*action).exec(0xc001592fa8)
        /Users/buringerst/code/pkg/mod/golang.org/x/[email protected]/go/analysis/internal/checker/checker.go:579 +0x3d
golang.org/x/tools/go/analysis/internal/checker.execAll.func1(0x0)
        /Users/buringerst/code/pkg/mod/golang.org/x/[email protected]/go/analysis/internal/checker/checker.go:567 +0x25
created by golang.org/x/tools/go/analysis/internal/checker.execAll
        /Users/buringerst/code/pkg/mod/golang.org/x/[email protected]/go/analysis/internal/checker/checker.go:573 +0x168

Workaround

When I split:

t.Run("for Cluster", customDefaultValidateTest(ctx, c, webhook))

into two statements:

testFunc := customDefaultValidateTest(ctx, c, webhook)
t.Run("for Cluster", testFunc)

everything works as expected.

Related ClusterAPI PR to upgrade to golangci-lint v1.44 / thelper v0.5.0 (kubernetes-sigs/cluster-api#6014)

type method also can be testing helper functions

The following case works well:

type helperType struct{}

func (h helperType) check(t *testing.T) {} // want "test helper function should start from t.Helper()"

But if the helper function used as a subtest, warnings should be suppressed.

func TestSubtest(t *testing.T) {
	h := subhelper{}
	t.Run("sub sel", h.check)
}

type subhelper struct{}

func (sh subhelper) check(t *testing.T) {}

token.Pos is not unique between packages

I'm developing new linter now and I created a type similar to yours:

type reports struct {
	reports  []report
	filter   map[token.Pos]struct{}
	nofilter map[token.Pos]struct{}
}

But I noticed that token.Pos cannot be unique ID for the entire linter, because linter is working with different passes from different packages (not with single one only) and reports is global singleton.

My code:

type positionID struct {
	pkg string
	pos token.Pos
}

type reporter struct {
	cache map[positionID]struct{}
}

Maybe it will be useful for you.

t.Helper() should not be mandatory when already called from an helper.

In this following situation:

func TestSmth(t *testing.T) {
	checkSmth(t)
}


func checkSmth(t *testing.T) {
	t.Helper()
        checkSmthElse(t)
}

func checkSmthElse(t *testing.T) {
}

checkSmthElse, as only be called by another function being marked with t.Helper() and not directly by any tests does not need t.Helper(), as the test will always fail (in that case), on the checkSmth(t) call.

Right now, the helper requires checkSmthElse to be annoted too.

`_ *testing.T` should be okay

Currently a parameter called _ of type _ *testing.T is an error (probably same for testing.B). This should be okay because sometimes you want to implement a testing related interface but don't have any assertions to make, so you don't want the *testing.T parameter. If this is not allowed, this linter would conflict with unparam https://github.com/mvdan/unparam.

thelper doesn't work inside golangci-lint when the analysed project use go1.17

Step to reproduce

  • use go1.17 (IMPORTANT)
  • use golangci-lint v1.46.0 or v1.46.1 (compiled with go1.18)
  • use the following files:
`go.mod`
module github.com/example/sandbox

go 1.17
`foo_test.go`
package sandbox

import "testing"

func thelperWithHelperAfterAssignment(t *testing.T) {
	_ = 0
	t.Helper()
}

func thelperWithNotFirst(s string, t *testing.T, i int) {
	t.Helper()
}

func thelperWithIncorrectName(o *testing.T) {
	o.Helper()
}
`.golangci.yml`
linters:
  disable-all: true
  enable:
    - typecheck
    - thelper

issues:
  exclude-use-default: false
  max-same-issues: 0
  max-issues-per-linter: 0

Got

No issue reported:

$ golangci-lint run
$

Want

$ golangci-lint run
foo_test.go:5:6: test helper function should start from t.Helper() (thelper)
func thelperWithHelperAfterAssignment(t *testing.T) {
     ^
foo_test.go:10:6: parameter *testing.T should be the first or after context.Context (thelper)
func thelperWithNotFirst(s string, t *testing.T, i int) {
     ^
foo_test.go:14:6: parameter *testing.T should have name t (thelper)
func thelperWithIncorrectName(o *testing.T) {
     ^

False positive: fuzz tests

I've run across a couple of false positives relating to fuzz tests:

package test

import (
	"testing"
)

func FuzzTest(f *testing.F) {
	f.Add("something")
	f.Fuzz(func(t *testing.T, in string) {   // lint warning is here
	})
}

This emits test helper function should start from t.Helper() for the internal function; presumably since fuzz tests are relatively new, thelper doesn't understand them like it does for normal subtests?

Thanks for the tool - has found a few cases where I can improve my test's output!

Support stretchr/testify/suite.Suite helpers

Hello!

Thank u for linter.

Example of that I mean:

import "github.com/stretchr/testify/suite"

type ServiceSuite struct {
    suite.Suite
    // ...
}

func (s *ServiceSuite) TestGetUser() {
    // ...
    s.requireMakeRequest()
}

func (s *ServiceSuite) requireMakeRequest() {  // Private method with suite API calls is helper.
    s.T().Helper()
    
    // Do stuff ...
    s.Equal(http.StatusOk, resp.StatusCode)
}

Feature request: configure the symbol name to check for

Some code I work with uses a subset of testing.T's interface, and so has declared something like this:

type TestingT interface {
    Helper()
    // ... other methods of testing.T
}

The linter doesn't pick up on that and AFAICT can't be configured to consider TestingT to be something to check for Helper calls.

add note about golangci-lint

thelper should be included into new release (1.34) of golangci-lint. After that it will be noticed in readme with example of configuration.

false positive: subtest (t.Run) where test function is returned from factory

I have a case, where I do the following:

func TestIntegration(t *testing.T) {
	tt := []struct {
		name     string
		testFunc func(testFixture *integrationtest.Test) func(*testing.T)
	}{
		{
			name:     "Foo",
			testFunc: testFoo,
		},
	}

	testFixture := integrationtest.New(t)
	defer testFixture.Shutdown()

	for _, tc := range tt {
		t.Run(tc.name, tc.testFunc(testFixture))
	}
}

func testFoo(testFixture *integrationtest.Test) func(*testing.T) {
	return func(t *testing.T) {
		testFixture.Setup()

		// some test logic
	}
}

The logic is used to start a database backend as docker container (integrationtest.New(t)) and then create a DB with the necessary tables and fixtures (testFixture.Setup()) for each test case.

The actual test function, that is executed with t.Run(...) is returned from a factory function. In this case, the returned function is not a test helper but the actual test function. But thelper does falsely recognize the returned function as test helper.

Might be related to: #15

linter setting to configure exceptions

Why?

Testing frameworks can provide special syntax that can generate false linter warnings.

How?

So there are several ways to solve it:

  • Add // nolint: thelper // Not a helper. to any exception, but it is not the best idea if there are many tests...
    Naive disabling of linter for the entire file, also so-so, because it may still have helpers.
  • Once configure common pattern exceptions in linter-settings of golangci-lint config. It is my suggestion.
  • Hard-code exceptions in thelper. Too many cases to handle, so not really an option.

Relates to hedhyw/gherkingen#51

Thank you for the linter!

false positive: subtest (t.Run) detected since v0.2.0

Hi.
Thanks for the wonderful linter.
I found something like false positive at v0.2.0.

main_test.go

package main

import "testing"

func Test(t *testing.T) {
	t.Run("sub", func(t *testing.T) {})
}
$ go install github.com/kulti/thelper/cmd/[email protected]
$ thelper ./...
$ go install github.com/kulti/thelper/cmd/[email protected]
$ thelper ./...
/path/to/main_test.go:6:15: test helper function should start from t.Helper()

Allow context.Context as first argument

I have test helpers that use both contexts and testing.T. There's a bit of style ambiguity regarding which one comes first. I always put the context first, and then the testing.T second, but I see messages like:

internal/birc/fakeirc/helper.go:43:1: parameter *testing.T should be the first (thelper)
func NewHelper(ctx context.Context, t *testing.T, opts ...Option) *Helper {
^
internal/pkg/apiclient/twitch/channel_test.go:442:1: parameter *testing.T should be the first (thelper)
func tokFor(ctx context.Context, t *testing.T, tw *twitch.Twitch, ft *fakeTwitch, id int64) *oauth2.Token {
^
internal/pkg/apiclient/twitch/dead_token_test.go:59:1: parameter *testing.T should be the first (thelper)
func invalidRefreshTokFor(ctx context.Context, t *testing.T, tw *twitch.Twitch, ft *fakeTwitch, id int64, refresh string) *oauth2.Token {

If I swap them, then golint itself complains:

internal/birc/fakeirc/helper.go:43:1: context.Context should be the first parameter of a function (golint)
func NewHelper(t *testing.T, ctx context.Context, opts ...Option) *Helper {

It'd be nice if for purposes of this analyzer that the context is ignored; I don't exactly want to fight golint.

Recommend Projects

  • React photo React

    A declarative, efficient, and flexible JavaScript library for building user interfaces.

  • Vue.js photo Vue.js

    ๐Ÿ–– Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.

  • Typescript photo Typescript

    TypeScript is a superset of JavaScript that compiles to clean JavaScript output.

  • TensorFlow photo TensorFlow

    An Open Source Machine Learning Framework for Everyone

  • Django photo Django

    The Web framework for perfectionists with deadlines.

  • D3 photo D3

    Bring data to life with SVG, Canvas and HTML. ๐Ÿ“Š๐Ÿ“ˆ๐ŸŽ‰

Recommend Topics

  • javascript

    JavaScript (JS) is a lightweight interpreted programming language with first-class functions.

  • web

    Some thing interesting about web. New door for the world.

  • server

    A server is a program made to process requests and deliver data to clients.

  • Machine learning

    Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.

  • Game

    Some thing interesting about game, make everyone happy.

Recommend Org

  • Facebook photo Facebook

    We are working to build community through open source technology. NB: members must have two-factor auth.

  • Microsoft photo Microsoft

    Open source projects and samples from Microsoft.

  • Google photo Google

    Google โค๏ธ Open Source for everyone.

  • D3 photo D3

    Data-Driven Documents codes.