Add option ignoremissingsubtests to ignore missing t.Parallel() in subtests
#32
Conversation
…n subtests In our codebase we've coded ourselves into a bit of a corner wherein although all our top-level tests are safe to run concurrently, subtests _within_ any particular test are not due to the use of a shared set of variables. It'd be nice to fix this of course, but doing so would involved changing thousands of tests and many dozens of hours of work. For any Go project, if you've managed to get to a place where all your top-level tests within a package can run in parallel, that's pretty good. You might not get the same granularity of work balancing that you would if all your subtests were parallel too, but (depending somewhat on the variance in timing between your tests) it gets like you like 80% of the way there. So here, I propose that the package adds a new `-ignoremissingsubtests` flag. It's similar to the existing `-i`, but only ignores missing calls to `t.Parallel` in subtests and ranges.
| A few options can be activated by flag: | ||
|
|
||
| * `-i`: Ignore missing calls to `t.Parallel` and only report incorrect uses of it. | ||
| * `-ignoremissingsubtests`: Require that top-level tests specify `t.Parallel`, but don't require it in subtests (`t.Run(...)`). |
There was a problem hiding this comment.
The flag naming here might be a bit of a mouthful, but I didn't want to pick another arbitrary one letter name like -i because longer term, that'll add to confusion (say more flags are added), and these flags will rarely be typed by human anyway.
In terms of -ignoremissingsubtests versus ignore-missing-subtests versus ignore_missing_subtests, I went with the first option because that seems to be the convention preferred by Go's Plan 9 style flag system (see go help build for some examples of this). With Linux flags I would've used ignore-missing-subtests. I did notice that some other linters use underscores for option names. I have no strong preference and happy to change this to whatever.
| type parallelAnalyzer struct { | ||
| analyzer *analysis.Analyzer | ||
| ignoreMissing bool | ||
| ignoreMissingSubtests bool |
There was a problem hiding this comment.
I put in a wrapper struct like this to make it much simpler and less hacky to set extra options from the test suite. The convention is inspired by Forbidigo (another Go linter) whose code I referenced.
|
@kunwardeep Hey, I saw that the project seems to be in a semi-maintained state, but it's still in golangci-lint's core, and AFAIK there isn't a better maintained alternative. Thoughts on this idea? It'd be useful for us, but I figured others might find it useful too. |
| @@ -0,0 +1,163 @@ | |||
| package t | |||
There was a problem hiding this comment.
This is just a full carbon copy of t_test.go with some of the expected warnings removed.
|
Hey @brandur i am glad this liner is helping your and your team. I have been sick for a week, hence the delay in response. I'll have a look at this PR this week. Thanks for contributing. |
|
Thanks Kunwardeep! And no worries — hope you're feeling better soon. |
|
Thanks @kunwardeep. Opened golangci/golangci-lint#3985 to see if we can get this into golangci-lint! |
In our codebase we've coded ourselves into a bit of a corner wherein
although all our top-level tests are safe to run concurrently, subtests
within any particular test are not due to the use of a shared set of
variables. It'd be nice to fix this of course, but doing so would
involved changing thousands of tests and many dozens of hours of work.
For any Go project, if you've managed to get to a place where all your
top-level tests within a package can run in parallel, that's pretty
good. You might not get the same granularity of work balancing that you
would if all your subtests were parallel too, but (depending somewhat on
the variance in timing between your tests) it gets like you like 80% of
the way there.
So here, I propose that the package adds a new
-ignoremissingsubtestsflag. It's similar to the existing
-i, but only ignores missing callsto
t.Parallelin subtests and ranges.