Skip to content

CBG-5829: have better isgr test assertions - #8838

Merged
torcolvin merged 5 commits into
mainfrom
CBG-5829
Oct 5, 2026
Merged

torcolvin merged 5 commits into
mainfrom
CBG-5829

Conversation

@gregns1

@gregns1 gregns1 commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor

CBG-5829

Helper function to assert on documnet details rather than just waiting for a specific version to arrive at a given peer.

Pre-review checklist

  • Logging sensitive data? Make sure it's tagged (e.g. base.UD(docID), base.MD(dbName))
  • Updated relevant information in the API specifications (such as endpoint descriptions, schemas, ...) in docs/api

Dependencies (if applicable)

  • Link upstream PRs
  • Update Go module dependencies when merged

Integration Tests

@gregns1
gregns1 requested review from a team and a balanced review from Copilot September 29, 2026 09:48
@factory-droid

factory-droid Bot commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor

Droid finished @gregns1's task —— View job


Review summary (validator pass): Most of the new ISGR test helpers look solid and should reduce flakiness. One edge case remains: the snapshot API currently can’t represent or validate legacy documents that truly have no HLV, which can make “peer unchanged” assertions misleading or fail unexpectedly.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

The new HLV comparator lacks negative coverage for current and merge-version mismatches.

Review effort: Balanced
Findings: 1 Medium severity · 1 Low severity

Open (2)
What changed in this PR

Adds stronger ISGR test assertions by comparing replicated document state across peers.

Changes:

  • Adds reusable document replication assertion helpers.
  • Validates HLVs, revision ancestry, channels, bodies, tombstones, and attachments.
  • Migrates replication tests to the stronger assertions.
File Description
rest/​utilities_testing_isgr.go Implements ISGR document comparison helpers.
rest/​utilities_testing_isgr_test.go Tests encoded-revision HLV comparisons.
rest/​revocation_test.go Strengthens revocation replication checks.
rest/​replicatortest/​replicator_test.go Migrates general replication assertions.
rest/​replicatortest/​replicator_test_legacy_rev_test.go Migrates legacy-revision assertions.
rest/​replicatortest/​replicator_conflict_test.go Strengthens conflict-resolution checks.
rest/​blip_api_crud_test.go Strengthens process-revision replication validation.

💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread rest/utilities_testing_isgr_test.go
Comment thread rest/replicatortest/replicator_test_legacy_rev_test.go Outdated
Comment thread rest/utilities_testing_isgr.go Outdated
Comment thread rest/replicatortest/replicator_conflict_test.go Outdated
Comment thread rest/replicatortest/replicator_conflict_test.go Outdated
Comment thread rest/replicatortest/replicator_conflict_test.go Outdated
Comment thread rest/replicatortest/replicator_test.go Outdated
Comment thread rest/replicatortest/replicator_test.go Outdated
Comment thread rest/replicatortest/replicator_test.go Outdated
Comment on lines +5880 to +5881
source.WaitForTombstoneRevIDOnly(doc2ID, deleteVersion)
sgrRunner.RequireDocReplicated(doc2ID, source, dest, deleteVersion)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Drop the duplicate wait. WaitForDocReplicated already waits on source, tombstones included.

Suggested change
source.WaitForTombstoneRevIDOnly(doc2ID, deleteVersion)
sgrRunner.RequireDocReplicated(doc2ID, source, dest, deleteVersion)
sgrRunner.WaitForDocReplicated(doc2ID, source, dest, deleteVersion)

Comment thread rest/replicatortest/replicator_test.go Outdated
Comment on lines +6566 to +6569
sgrRunner.RequireDoc(docID, activeRT, rest.ExpectedISGRDoc{Version: expVersion})
sgrRunner.RequireDoc(docID, passiveRT, passiveDocBeforeReplication)
} else {
sgrRunner.RequireDocReplicated(docID, activeRT, passiveRT, expVersion)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Race in the v4 local-wins case: expVersion.CV is read from activeRT at lines 6554-6562, right after the replication starts and often before the conflict is resolved. Local wins mints a new CV, so the wait can time out on a stale CV. Pass only the rev tree ID and check the CV source after the wait. The remote-wins branch also gets a barrier through RequireDocUnchanged.

Lines 6550-6563 are outside the diff: once this is applied, you can delete that block and use *remoteWinsVersion in the remote-wins branch.

Suggested change
sgrRunner.RequireDoc(docID, activeRT, rest.ExpectedISGRDoc{Version: expVersion})
sgrRunner.RequireDoc(docID, passiveRT, passiveDocBeforeReplication)
} else {
sgrRunner.RequireDocReplicated(docID, activeRT, passiveRT, expVersion)
sgrRunner.RequireDoc(docID, activeRT, rest.ExpectedISGRDoc{Version: expVersion})
sgrRunner.RequireDocUnchanged(docID, activeRT, passiveRT, passiveDocBeforeReplication)
} else {
doc := sgrRunner.WaitForDocReplicated(docID, activeRT, passiveRT, test.expectedFinalVersion)
if sgrRunner.IsV4Protocol() {
// local wins mints a new CV on activeRT during resolution, so only its source is known up front
assert.Equal(t, activeRT.GetDatabase().EncodedSourceID, doc.HLV.SourceID)
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm made changes to make the code better on this test but we don't mint a new CV on local wins for HLV. only the rev tree ID changes.

Comment thread rest/replicatortest/replicator_test_legacy_rev_test.go Outdated
Comment thread rest/replicatortest/replicator_test_legacy_rev_test.go Outdated
Comment thread rest/utilities_testing_isgr.go Outdated
@torcolvin torcolvin assigned gregns1 and unassigned torcolvin Sep 30, 2026
@gregns1 gregns1 assigned torcolvin and unassigned gregns1 Oct 2, 2026
Comment thread rest/utilities_testing_isgr.go Outdated
@torcolvin
torcolvin merged commit b4bc5b6 into main Oct 5, 2026
48 checks passed
@torcolvin
torcolvin deleted the CBG-5829 branch October 5, 2026 13:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants