Range statement for test ... does not use range value in t.Run linter is not always correct

[invidian]

Following piece of code currently triggers the linter, even though it is a valid code.

package main

import (
  "fmt"
  "testing"
)

func TestFunctionRangeNotUsingRangeValueInTDotRun(t *testing.T) {
  t.Parallel()

  testCases := map[string]struct {
    value string
  }{
    "foo": {value: "foo"},
  }

  for n, tc := range testCases {
    n, tc := n, tc

    t.Run(n, func(t *testing.T) {
      t.Parallel()
      fmt.Println(tc.value)
    })
  }
}

[HTechHQ]

I have a similar example, that I consider to be valid.

func TestFunctionRangeNotUsingRangeValueInTDotRun(t *testing.T) {
    t.Parallel()

    testCases := []struct {
        Name     string
        Sequence int
    }{{Name: "foo", Sequence: 1337}}

    for _, tc := range testCases {
        tc := tc
        t.Run(fmt.Sprintf("%v->%d", tc.Name, tc.Sequence), func(t *testing.T) {
            t.Parallel()
            // ...
        })
    }
}

[kulti]

I think, it false positive with nested for range like this:

func TestValidateStateSwitchInconcistency(t *testing.T) {
    t.Parallel()

    transitions := map[models.TaskState][]models.SwitchTaskStateEvent{
        unknownState: {
            models.TodoTaskEvent,
            models.DoneTaskEvent,
            models.CancelTaskEvent,
            models.ToWorkTaskEvent,
            unknownEvent,
        },
        models.TaskStateSimple: {
            models.ToWorkTaskEvent,
            unknownEvent,
        },
        // ...
    }

    for state, tr := range transitions {
        for _, ev := range tr {
            state := state
            ev := ev
            t.Run(string(state)+" -> "+ev.String(), func(t *testing.T) {
                t.Parallel()
                // ...
            })
        }
    }
}

[fubbyb]

One more false positive example:

func TestNodeType_String(t *testing.T) {
    test := func(nt NodeType, want string) func(*testing.T) {
        return func(t *testing.T) {
            t.Parallel()
            ...
        }
    }

    tests := []struct {
        name string
        nt   NodeType
        want string
    }{
        ...
    }

    t.Parallel()
    for _, tt := range tests {
        t.Run(tt.name, test(tt.nt, tt.want))
    }
}

[rkennedy]

The linter tries to determine the name of the variable used in the first Run parameter, and then it checks whether there are any statements initializing that variable from the range's value identifier. It expects the expression in the Run parameter to be of the form var.property, and then it returns the name of var. All the examples in this issue are cases where the expression doesn't match that pattern.

A different technique would be to identify the range variable first, then the variable(s) initialized from that. Then, check whether those variables are used anywhere in the Run parameter's expression. It would provide a pretty good estimate of whether the Run name depends on the test case. It can never be perfect, though.

[jackwhelpton]

I've just started to apply this linter, and the pattern in the original post (looping over a map of test cases keyed by the test name) is so common that the number of false positives is unmanageable. Here's a good piece on why this pattern makes sense:

https://dave.cheney.net/2019/05/07/prefer-table-driven-tests

Barring a means to detect this, is there a way to control which of the rules the linter runs, i.e. to include the other checks and just disable Range statement for test ...?

[invidian]

Barring a means to detect this, is there a way to control which of the rules the linter runs, i.e. to include the other checks and just disable Range statement for test ...?

golangci-lint is capable of doing that.

[jackwhelpton]

Could you give me a little more detail? I'll check the documentation myself, too. I know I can disable the entire paralleltest linter (in fact, that's been my current strategy), but ideally I'd like to keep the other checks, which have proved helpful.

[invidian]

@jackwhelpton put the following snippet in your .golangci.yml configuration:

issues:
  exclude-rules:
    # False positive: https://github.com/kunwardeep/paralleltest/issues/8.
    - linters:
      - paralleltest
      text: "does not use range value in test Run"

[emil14]

Better to use slices - this is how auto generated tests works and slices are also faster to iterate

[invidian]

Better to use slices - this is how auto generated tests works and slices are also faster to iterate

I don't think iteration performance is crucial here, but I'm curious what kind of auto generated tests you talk about. Do you have some examples?

[emil14]

I don't think iteration performance is crucial here

Ofcource it's not :) Just another disadvantage of working with maps in table-driven go tests.

What kind of auto generated tests you talk about

I'm talking about tests generated by VSCode or Goland. I think VSCode uses https://github.com/cweill/gotests but I'm not sure

[jackwhelpton]

I'm not sure I see any advantage in slices over maps: iteration performance for a handful of test cases screams premature optimization to me.

By contrast, a map provides you with a structure that can be keyed by test name, which improves readability (the test name and data can be fetched into separate variables as part of the loop), and also offers a slight shuffling of execution order that could be beneficial: Dave's article I linked to above provides a pretty good explanation.

I've not tended to use "auto-generated" code as a guideline (Go's own source code, sometimes - although even that varies quite a lot in style based on the author). In many situations, what you get from looking at auto-generated code is something that's "good enough" in all cases, not ideal in the one you're specifically trying to address.

Until there's a workaround for this, the suppression @invidian suggested works a charm.

[emil14]

iteration performance for a handful of test cases screams premature optimization to me.

To me too

By contrast, a map provides you with a structure that can be keyed by test name, which improves readability

I agree :) That's why I've used to use them until I came into trouble we discuss here

I've not tended to use "auto-generated" code as a guideline

That's not a guideline (yet) but it seems reasonable to try to improve automation instead of rejecting it

The main problem here is parallel testing - other disadvantages like code-generation and performance are not so important but they are still there

[AlekSi]

Following piece of code currently triggers the linter, even though it is a valid code.

@invidian, FWIW, that's not a valid Go. Here I modified it to make it compile (testCases was defined incorrectly) and to highlight a problem:

func TestFunctionRangeNotUsingRangeValueInTDotRun(t *testing.T) {
    t.Parallel()

    testCases := map[string]struct {
        value string
    }{
        "1": {value: "foo"},
        "2": {value: "bar"},
    }

    for n, tc := range testCases {
        n, tc := n, tc // fixes the problem

        t.Run(n, func(t *testing.T) {
            t.Parallel()
            fmt.Println(tc.value)
        })
    }
}

Without n, tc := n, tc // fixes the problem line, the output is likely to be "bar, bar" instead of "foo, bar" or "bar, foo". That's due to https://golang.org/doc/faq#closures_and_goroutines (t.Parallel + t.Run creates a goroutine).

Some other examples in this issue are similarly incorrect.

---

That being said, this linter complains about both forms of this code, with and without n, tc := n, tc // fixes the problem line. I hope that could be fixed.

[emil14]

we need linter for this stuff n, tc := n, tc

[invidian]

we need linter for this stuff n, tc := n, tc

This is checked by scopelint, but it's now deprecated in golangci-lint :(

[jackwhelpton]

Doesn't exportloopref suffice? I have this in my golangci-lint config:

  # The following linters should not be enabled:
  ...
  # - scopelint         deprecated, supplanted by exportloopref

[invidian]

See https://github.com/kyoh86/looppointer/issues/8

[jackwhelpton]

Oh boo. That's good to know... adding those lines has become second nature within my tests, but it's a pain there's now nothing that catches their absence, as I was bitten by that a fair few times in earlier days.

[kunwardeep]

This is embarrassing. My email filter was junking all these emails. The filter has since been fixed, and the issue is now resolved. Thanks for your effort @rkennedy and @rarguelloF. Once again, my apologies