CBG-4932 improve ResyncManger test flakes #7891
Conversation
- the problem seems to be that some tests take a long time to run, mostly after churn of the bucket pool to do query operations after resync finishes - clean up to use test functions for waiting
There was a problem hiding this comment.
Pull Request Overview
This PR addresses test flakiness in ResyncManager tests by improving wait conditions and error handling. The main issue was that some tests were timing out due to slow query operations, particularly in CI environments with GSI disabled (using views instead).
- Replaced custom wait loops with standard test helper functions for consistency
- Extended wait times for non-Walrus tests to account for slower query operations
- Improved error handling in
invalidateAllPrincipalsfunction
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
db/util_testing.go |
Extended RequireBackgroundManagerState timeout and added context handling |
db/background_mgr_resync_dcp_test.go |
Refactored tests to use helper functions and removed redundant ResyncManager initialization |
db/background_mgr_resync_dcp.go |
Added error handling for principal invalidation |
db/database.go |
Changed invalidateAllPrincipals to return error instead of ignoring it |
db/database_collection.go |
Removed unused invalidateAllPrincipals wrapper method |
db/database_test.go |
Updated test to use database-level method and check error |
rest/attachmentmigrationtest/attachment_migration_test.go |
Removed unnecessary context parameter from helper call |
db/background_mgr_attachment_migration_test.go |
Removed unnecessary context parameter from helper calls |
Co-authored-by: Copilot <[email protected]>
db/util_testing.go
Outdated
| waitTime := 10 * time.Second | ||
| if !base.UnitTestUrlIsWalrus() { | ||
| // Increase wait time for CI tests against Couchbase Server with GSI disabled (views), some queries take a | ||
| // longer time to run |
There was a problem hiding this comment.
Should there be an additional check here for gsi being enabled given the comment suggests this is views specific?
There was a problem hiding this comment.
I think the comment is wrong actually. It takes longer for views, but it just sometimes also takes a long time for Couchbase Server if I've angered overloaded it. I decided not to investigate a lot because I already did a boring and unfruitful views investigation before I realised I didn't use SG_TEST_USE_GSI=true. I think the benefits of waiting longer outweigh potentially not returning early.
… heartbeat is removed
Integration Tests
GSI=true,xattrs=truehttps://jenkins.sgwdev.com/job/SyncGatewayIntegration/175/ (known independent test failure)