search

kunwardeep / paralleltest

Linter to check if your tests have been marked as parallel correctly
MIT License
48 stars 12 forks source link

dont flag tests which call Setenv directly or whose subtests do #33

Closed spencerschrock closed 9 months ago

spencerschrock commented 10 months ago

Setenv cannot be used in parallel tests or tests with parallel ancestors.

This attempts to fix the issue raised in https://github.com/kunwardeep/paralleltest/issues/13#issuecomment-1749707624

spencerschrock commented 10 months ago

@mitar would you mind taking a first look at the added tests to see what's now being flagged/not-flagged, and if it addresses your comment?

https://github.com/kunwardeep/paralleltest/blob/7bc464af1ec3880618fa0256d62e234efcd90640/pkg/paralleltest/testdata/src/t/t_test.go#L166-L219

mitar commented 10 months ago

Thanks. This looks great.

Maybe add some test cases which use a test helper (function which calls t.Helper()) and in it it calls t.Setenv. Then test case should still not be flagged for missing t.Parallel.

spencerschrock commented 10 months ago

Maybe add some test cases which use a test helper (function which calls t.Helper()) and in it it calls t.Setenv. Then test case should still not be flagged for missing t.Parallel.

Ah that's a great catch, though it's a harder case to handle with how the rest of the analyzer is written. I don't think I'll try to implement it in this PR, it would involve larger changes to the analyzer. My initial thoughts involve some sort of pre-pass to identify test helpers which call Setenv, and then a call analysis for reachability.

PhilThurston commented 10 months ago

I like this simple approach implemented here. We are having the same issues where if we use t.Setenv() along with t.Parallel() we get a panic yet the linter is telling us we are wong.

kunwardeep commented 9 months ago

Sorry folks, I was away on holiday. I will take a look at this today