Skip to content

Conversation

@RIT3shSapata
Copy link
Contributor

CBG-4915

Describe your PR here...

  • The test was flaky because the test would progress from the require.EventuallyWithT even though the condition is not completely met
  • Refactored the code to use RestTester's WaitForVersion method instead

Pre-review checklist

  • Removed debug logging (fmt.Print, log.Print, ...)
  • 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

Copilot AI review requested due to automatic review settings December 5, 2025 13:49
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR fixes flakiness in the TestMultiActorLosingConflictUpdateRemovingAttachments test by replacing require.EventuallyWithT with RestTester.WaitForVersion() to properly wait for document replication.

Key Changes:

  • Replaced manual polling loop with built-in WaitForVersion method for more reliable synchronization

Comment on lines +81 to 82
rtBVersion, _ := rtB.GetDoc(docID)

Copy link

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

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

The error return value from GetDoc is being silently discarded. Consider handling the error or using require.NoError(t, err) to ensure the document retrieval succeeds before proceeding with the test.

Suggested change
rtBVersion, _ := rtB.GetDoc(docID)
rtBVersion, err := rtB.GetDoc(docID)
require.NoError(t, err)

Copilot uses AI. Check for mistakes.
Comment on lines +80 to +81
rtB.WaitForVersion(docID, rtAVersion)
rtBVersion, _ := rtB.GetDoc(docID)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think given this assertion, that rtBVersion and rtAVersion are the same.

I would perhaps assert that the version has CV and Revtree.

It is possible that this assertion will only work on CV, which will wait for the document to be copied (CV matches) but not imported.

I haven't looked into this, but we can discuss.

Comment on lines 132 to 133
assert.EventuallyWithT(t, func(c *assert.CollectT) {
currentRtAVersion, _ := rtA.GetDoc(docID)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This needs to get changed too

Comment on lines 150 to 151
require.NoError(t, xdcrAtoB.Stop(ctx))
require.NoError(t, xdcrBtoA.Stop(ctx))
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should move these to a defer. In the case of rosmar, the xdcr replications will stop when the buckets disappear but in CBS they won't, and so this could leak into future tests.

Maybe this is actually already handled by defer at the top.

@torcolvin torcolvin assigned RIT3shSapata and unassigned torcolvin Dec 5, 2025
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