testifylint->go-require - Should we enable it? #15535
Comments
|
Yeah we should enable this as the findings show how likely it is that you misuse |
|
+1 |
|
Folks, after the recent PRs, for the following configuration: all findings have been addressed, and we could enable this checker. As you can see, this configuration disables the check for functions and methods whose signature matches http.HandlerFunc. These handlers run in a separate service goroutine that handles the HTTP connection. Terminating these goroutines can lead to undefined behavior and difficulty in debugging tests. There are currently |
|
To be honest we should return a |
|
@srebhan I would set the status to |
Description
This issue starts a discussion about enabling:
This checker is a radically improved analogue of
go vet's testinggoroutine check.The point of the check is that, according to the documentation, functions leading to
t.FailNow(essentially toruntime.GoExit) must only be used in the goroutine that runs the test. Otherwise, they will not work as declared, namely, finish the test function.You can continue to use
requireas the current goroutine finisher, but this could leadTypically, any assertions inside goroutines are a marker of poor test architecture. Try to execute them in the main goroutine and distribute the data necessary for this into it (example).
Also a bad solution would be to simply replace all
requirein goroutines withassert(like here) – this will only mask the problem.In addition, the checker warns about
requirein HTTP handlers (functions and methods whose signature matches http.HandlerFunc), because handlers run in a separate service goroutine that services the HTTP connection. Terminating these goroutines can lead to undefined behaviour and difficulty debugging tests. You can turn off the check using the--go-require.ignore-http-handlersflag.P.S. Look at testify's issue, related to assertions in the goroutines.
Example
Expected output
Decision about enabling or not enabling this checker.
Findings
For this checker, the following findings were found in the current codebase:
Additional configuration
For this checker, additional configuration can be provided:
The text was updated successfully, but these errors were encountered: