CBG-4434 enable conflict tests #7688
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR enables previously skipped conflict tests in the topology test suite by addressing issues with Couchbase Lite (CBL) actors in multi-actor conflict scenarios. The main purpose is to allow conflict tests to run with CBL peers by relaxing HLV (Hybrid Logical Vector) assertions and handling differences in conflict resolution behavior between CBL and other peer types.
Key changes include:
- Removing skip conditions for CBL-related conflict tests
- Adding context parameter support to BlipTester for improved readability
- Implementing relaxed assertions that accommodate different conflict resolution behaviors between CBL and non-CBL peers
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| topologytest/multi_actor_conflict_test.go | Removes CBL test skips and updates conflict test assertions to use CV-based matching |
| topologytest/hlv_test.go | Adds new assertion functions for handling convergent tombstones and mixed peer type scenarios |
| topologytest/sync_gateway_peer_test.go | Fixes document retrieval to properly handle deleted documents |
| topologytest/peer_test.go | Adds KeySGTest to logging configuration |
| topologytest/couchbase_lite_mock_peer_test.go | Updates BlipTester client creation to use context parameter |
| rest/utilities_testing_blip_client.go | Adds context parameter support throughout BlipTester infrastructure |
| @@ -74,7 +74,11 @@ func (p *SyncGatewayPeer) GetDocumentIfExists(dsName sgbucket.DataStoreName, doc | |||
| return DocMetadata{}, nil, false | |||
| } | |||
| require.NoError(p.TB(), err) | |||
| return DocMetadataFromDocument(doc), base.Ptr(doc.Body(ctx)), true | |||
| meta = DocMetadataFromDocument(doc) | |||
There was a problem hiding this comment.
The variable meta is declared but not used in the original return statement that was removed. This creates a variable assignment without proper initialization in the conditional flow.
Copilot uses AI. Check for mistakes.
| @@ -960,7 +962,8 @@ func (btcc *BlipTesterCollectionClient) updateLastReplicatedRev(docID string, ve | |||
| rev.message = msg | |||
| } | |||
|
|
|||
| func newBlipTesterReplication(tb testing.TB, id string, btc *BlipTesterClient, skipCollectionsInitialization bool) *BlipTesterReplicator { | |||
| // newBlipTesterReplication creates a new BlipTesterReplicator with the given id and BlipTesterClient. Used to instantiate a push or pull replication for the client. | |||
There was a problem hiding this comment.
The comment should mention that the function now requires a context parameter, which is a breaking change from the previous signature.
| // newBlipTesterReplication creates a new BlipTesterReplicator with the given id and BlipTesterClient. Used to instantiate a push or pull replication for the client. | |
| // newBlipTesterReplication creates a new BlipTesterReplicator with the given id and BlipTesterClient. | |
| // Used to instantiate a push or pull replication for the client. | |
| // Note: This function now requires a context.Context parameter, which is a breaking change from the previous signature. |
Copilot uses AI. Check for mistakes.
| } | ||
| assertHLVEqual(c, dsName, docID, peer, version, nil, *nonCBLVersion, topology) | ||
| } | ||
| // Is there a way to do any assertion on the CBL tombstone versions? |
There was a problem hiding this comment.
[nitpick] This TODO comment should either be resolved with a concrete implementation or converted to a proper TODO/FIXME format with a tracking issue reference.
| // Is there a way to do any assertion on the CBL tombstone versions? | |
| // TODO: Investigate whether assertions can be made on the CBL tombstone versions. |
Copilot uses AI. Check for mistakes.
| // This is used when asserting on the full HLV is impossible. If XDCR is running, then asserting on the full HLV for | ||
| // non CBL peers is possible. However, conflict resolution on Couchbase Lite means that Couchbase Lite can contain | ||
| // previous versions of a document. |
There was a problem hiding this comment.
| // This is used when asserting on the full HLV is impossible. If XDCR is running, then asserting on the full HLV for | |
| // non CBL peers is possible. However, conflict resolution on Couchbase Lite means that Couchbase Lite can contain | |
| // previous versions of a document. | |
| // This is used for scenarios where it's valid for the full HLV to not converge. This includes cases where | |
| // CBL conflict resolution results in additional history in the CBL version of the HLV that may not be pushed | |
| // to CBS (e.g. remote wins) |
| requireBodyEqual(t, expectedVersion.body, body) | ||
| } | ||
| } | ||
|
|
||
| // waitForConvergingTombstones waits for all peers to have a tombstone document for a given doc ID. A matching HLV import ( |
There was a problem hiding this comment.
I'm not sure what "matching HLV import" means.
| // 1. create document on each peer with different contents | ||
| // 2. start replications | ||
| // 3. wait for documents to exist with hlv sources equal to the number of active peers and the document body is | ||
| // equivalent to the last write. The assertion is presently only on CV for Couchbase Lite peers, and full HLV |
There was a problem hiding this comment.
I think we can remove "presently", unless we expect this to change.
CBG-4434 enable conflict tests
Contextparameter for BlipTester to make it shorter to read test casesRelax some assertions:
waitForConvergingTombstonesto reflect the fact tombstone replications with XDCR will not get replicated. CBG-4788Pre-review checklist
fmt.Print,log.Print, ...)base.UD(docID),base.MD(dbName))docs/apiIntegration Tests
GSI=true,xattrs=truehttps://jenkins.sgwdev.com/job/TopologyTests/9/GSI=true,xattrs=truehttps://jenkins.sgwdev.com/job/SyncGatewayIntegration/39/