DO NOT MERGE INTO 4.0: CBG-4862 force SendRev to check for errors #7762
+224
−466
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
There was a problem hiding this comment.
Pull Request Overview
This PR refactors BLIP testing utilities to improve error handling and consistency. It creates explicit functions for testing expected conflicts and simplifies the SendRev family of functions by removing error returns in favor of internal testify assertions.
- Introduces
SendRevExpectConflictfor explicit conflict testing scenarios - Refactors
SendRevand related functions to handle errors internally using testify assertions - Adds comprehensive docstrings and moves error checking outside of return values
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| rest/utilities_testing.go | Major refactoring of BLIP testing functions with new error handling approach |
| rest/blip_legacy_revid_test.go | Updated test calls to use new SendRev API and SendRevExpectConflict |
| rest/blip_api_crud_test.go | Simplified test code by removing error handling for SendRev calls |
| rest/blip_api_attachment_test.go | Updated attachment tests to use new SendRevWithAttachment API |
| rest/user_api_test.go | Simplified ReadContinuousChanges usage |
| rest/sync_fn_test.go | Updated WaitWithTimeout calls to new signature |
| rest/importtest/import_test.go | Updated WaitWithTimeout calls to new signature |
| rest/attachment_test.go | Updated SendRev calls to new API |
| rest/api_test.go | Updated WaitWithTimeout calls to new signature |
| rest/adminapitest/admin_api_test.go | Updated WaitWithTimeout calls to new signature |
| rest/access_test.go | Updated SequenceForDoc calls to new API |
Comments suppressed due to low confidence (1)
rest/blip_api_crud_test.go:1
- The error variable used in the error message should be 'getAttachmentErr' instead of 'err' to match the actual error variable being checked.
/*
rest/utilities_testing.go
Outdated
| } | ||
|
|
||
| // NewRestTester returns a rest tester backed by a single database and a single default collection. | ||
| // NewRestTesterDefaultCollection creates a rest tester backed by a single database and a single _default._deafult collection. |
There was a problem hiding this comment.
There's a typo in the comment: '_deafult' should be '_default'.
Suggested change
| // NewRestTesterDefaultCollection creates a rest tester backed by a single database and a single _default._deafult collection. | |
| // NewRestTesterDefaultCollection creates a rest tester backed by a single database and a single _default._default collection. |
Copilot uses AI. Check for mistakes.
| } | ||
|
|
||
| // NewRestTester multiple collections a rest tester backed by a single database and any number of collections and the names of the keyspaces of collections created. | ||
| // NewRestTesterMultipleCollections creates rest tester backed by a single database and any number of collections and the names of the keyspaces of collections created. |
There was a problem hiding this comment.
The comment is missing an article 'a' before 'rest tester' for grammatical correctness.
Suggested change
| // NewRestTesterMultipleCollections creates rest tester backed by a single database and any number of collections and the names of the keyspaces of collections created. | |
| // NewRestTesterMultipleCollections creates a rest tester backed by a single database and any number of collections and the names of the keyspaces of collections created. |
Copilot uses AI. Check for mistakes.
| return sent, revRequest, revResponse, fmt.Errorf("Unexpected error sending rev: %v\n%s", errorCode, body) | ||
| // SendRevWithHistory sends an unsolicited rev message and waits for the response. The docHistory should be in the same format as expected by db.PutExistingRevWithBody(), or empty if this is the first revision | ||
| func (bt *BlipTester) SendRevWithHistory(docID, docRev string, revHistory []string, body []byte, properties blip.Properties) (res *blip.Message) { | ||
| require.NotContains(bt.TB(), properties, "history", "If specifying history, use BlipTester.SendRev") |
There was a problem hiding this comment.
The error message suggests using 'BlipTester.SendRev' but the actual method name is 'SendRev'. Consider clarifying this refers to the SendRev method without history parameter.
Suggested change
| require.NotContains(bt.TB(), properties, "history", "If specifying history, use BlipTester.SendRev") | |
| require.NotContains(bt.TB(), properties, "history", "If specifying history, use BlipTester.SendRevWithHistory (not SendRev)") |
Copilot uses AI. Check for mistakes.
| assert.True(t, sent) | ||
| require.ErrorContains(t, err, "Document revision conflict") | ||
| bt.SendRevExpectConflict("doc1", "3-def", []byte(`{"key": "val"}`), blip.Properties{db.RevMessageHistory: strings.Join(history, ",")}) | ||
| //require.ErrorContains(t, err, "Document revision conflict") |
There was a problem hiding this comment.
This commented-out line should be removed as it's no longer needed with the new API.
Suggested change
| //require.ErrorContains(t, err, "Document revision conflict") |
Copilot uses AI. Check for mistakes.
- Create SendRevExpectConflict to explicitly test for when a conflict is expected.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
CBG-4862 force SendRev to check for errors
SendRevto only return response, since that is the only value used.NOTE: This doesn't need to merge into 4.0 for release, so should wait until the release/4.0 branch is created.