-
Notifications
You must be signed in to change notification settings - Fork 1.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
suite may race when its test creates goroutine #1168
Comments
cannot reproduce locally maybe paste complete code, some symbol is missing |
How about we add a Our usage is in tests, we spawn a new goroutine to do some asynchronizing job, in the main test goroutine there is barrier to make sure the job is finished.
but if we use suite to check the |
the snippet should be put in testify/suite/suite_test.go |
SGTM. To me, it's better to help users do the right thing(close all goroutines after a test) easier. |
2293: Fix race detector issues. r=zhangchiqing a=SaveTheRbtz While exploring code base I've stumbled upon a couple of race-detector problems. Most of them are harmless issues that only affect test flakiness. Nevertheless, here is a sample of interesting ones: * Unprotected `OnSucceed` callback access in badger batches: 35babdb * Potential error clobbering in ledger migration's "storage-used" calculation: c42159f * EpochMgr `epochs` access without lock on `Done`: a0c6cee * `BLS12_381` verification accesses uninitialized memory: #2293 (comment) * Unprotected global access in tests d26ca03 * Lock being copied by value by the mock framework: a89101e * `time.Sleep` references in tests: 05a87c7 * Ignore `runtime.noescape` in `checkptr`: 1288c1d I needed to disable the network topology test in `-race` since it was triggering the goroutine limit of the race detector bb24d8f. For this one we'll need to wait until the support of infinite number of goroutines is released golang/go@d6a1ffd is available (go likely 1.19). Also there is a race condition in herocache that can be solved by either: * Making Counter and Timestamp atomic: SaveTheRbtz@d1cc406 * Removing `logTelemetry` methods from the "read-only" methods protected only by `RLock`: SaveTheRbtz@6b0dcbd What is the better approach here? (cc: `@yhassanzadeh13)` Testify also has couple of nasty races in their `Suite` implementation if coupled with spawning gorouptines: stretchr/testify#1168 (comment): 4851614 71b3fdc Looking at [race detector](https://go.dev/doc/articles/race_detector)'s ability to find timing/concurrency related problems in both tests and code it looks like it should likely be a first-class citizen in the Flow's CI/CD pipeline. WDYT about its ROI? Seems like `@huitseeker` did look at this problem some time ago but since then race detector work was mostly abandoned. Maybe it is possible to make it an optional part of the CI/CD pipeline, like fuzzing and benchmarking? Co-authored-by: Alexey Ivanov <rbtz@ph34r.me>
Calling More generally (beyond this case) parallel testing with |
in testify/suite/suite_test.go
The text was updated successfully, but these errors were encountered: