-
Notifications
You must be signed in to change notification settings - Fork 366
chore: reduce memdb test flakes #2820
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
base: main
Are you sure you want to change the base?
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2820 +/- ##
==========================================
- Coverage 78.46% 78.45% -0.00%
==========================================
Files 459 459
Lines 44674 44674
==========================================
- Hits 35048 35044 -4
- Misses 6769 6772 +3
- Partials 2857 2858 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Hmm... The memdb tests are already spinning up a separate memdb instance per test, so it's not likely resource contention within a single database. |
| func TestMemdbDatastore(t *testing.T) { | ||
| t.Parallel() | ||
| test.All(t, memDBTest{}, true) | ||
| // NOTE: The individual tests are not parallelized because enough write traffic will cause |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i don't understand how this fixes things. Each individual test is creating its own memdb instance. For example:
func RegisterRelationshipCountersInParallelTest(t *testing.T, tester DatastoreTester) {
rawDS, err := tester.New(0, veryLargeGCInterval, veryLargeGCWindow, 1)could it be that the default number of retries is 10 and this test is doing 10 parallel writes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh hmmm... yeah I suppose we could reduce it to 9 and see if that helps?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah. but don't harcode to 9, use numAttempts - 1 and maybe rename numAttempts to defaultNumAttempts :P
Description
See https://github.com/authzed/spicedb/actions/runs/21007920564/job/60396619281?pr=2818. We were getting test flakes from write failures that were the result of too much load on the memdb instance. For this reason, we're allowing the set of Memdb tests to run in parallel with other tests, but the test itself will not be parallelized to ensure that we aren't hammering the instance.
Changes
Testing
Review.