search

stretchr / testify

A toolkit with common assertions and mocks that plays nicely with the standard library
MIT License
22.92k stars 1.58k forks source link

Detect and fail on `require.*` methods called from a wrong goroutine #1499

Open ash2k opened 10 months ago

ash2k commented 10 months ago

As already documented (via https://github.com/stretchr/testify/pull/1392), require.* functions/methods are only safe to use from the main test/benchmark goroutine and are unsafe otherwise. Go stdlib may fix this one way or another (https://github.com/golang/go/issues/15758) but even if/when this happens, testify doesn't call those methods unless there is an assertion failure. This means the bug in the test can live in users code for a very long time.

I think ideally testify would fail the test (or panic) if a function/method from the require family is used incorrectly regardless of whether there is a failure in the test itself or not.

dolmen commented 10 months ago

No idea how this could be implemented (if ever that is a good idea in the first place).

Antonboom commented 9 months ago

Covered by https://github.com/Antonboom/testifylint#go-require

ash2k commented 9 months ago

@dolmen Perhaps the check could be implemented by the assertion looking at the goroutine's stacktrace? This might be fragile as stacktrace is not an API so there should be a test for this.

@Antonboom That's cool but static analysis likely cannot catch all cases.

Antonboom commented 9 months ago

@ash2k hi

but static analysis likely cannot catch all cases

I think these are very rare cases, because the linter works exactly the same way – it analyzes the call stack. But it looks as poor workaround for the open source library.

At the moment the Go team is going the static analysis way (go vet's testinggoroutine check), and I'm not sure they'll check the stack rather than just remove the requirement for where these functions should be called.

Because when I was testing my linter, I noticed that many projects (especially the largest ones) use require in goroutines and your proposal will be breaking change for them and looks like candidate for v2 tag.

Anyway sorry for offtop 🙏

dolmen commented 9 months ago

@Antonboom Don't be sorry: this is totally on topic.