-
Notifications
You must be signed in to change notification settings - Fork 138
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
make blip tests fail with stack traces using testify #6748
Conversation
- Create TB() helper functions to make it easier to pick up the right TB function since it can get reset or hard to pick up - use require.EventuallyT instead of testing.T.Fatalf to get tracebacks when they fail. Because these functions fail anyway, removed the return of boolean found. - switch panic to require statements while the code was here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, just some nitpicks. I have approved as its good as is to be merged given its test code and there is no real functional impacts of my comments. Feel free to merge as is.
@@ -2257,8 +2257,7 @@ func TestRevocationMessage(t *testing.T) { | |||
assert.NoError(t, err) | |||
|
|||
// Wait for doc revision to come over | |||
_, ok := btcRunner.WaitForBlipRevMessage(btc.id, "doc", version) | |||
require.True(t, ok) | |||
_ = btcRunner.WaitForBlipRevMessage(btc.id, "doc", version) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: Do we need to have the empty return values on these in this file? Could keep it in line with what you have done in most other places and not have the empty return values.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WaitForBlipRevMessage
does use return value in blip_api_delta_sync_test.go
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I meant was you could have btcRunner.WaitForBlipRevMessage(btc.id, "doc", version)
as opposed to _ = btcRunner.WaitForBlipRevMessage(btc.id, "doc", version)
It doesn't have any impact on tests so i'm good merging, was just a nitpick.
@@ -2040,8 +2035,7 @@ func TestRemovedMessageWithAlternateAccess(t *testing.T) { | |||
|
|||
err = btcRunner.StartOneshotPull(btc.id) | |||
assert.NoError(t, err) | |||
_, ok := btcRunner.WaitForVersion(btc.id, docID, version) | |||
assert.True(t, ok) | |||
_ = btcRunner.WaitForVersion(btc.id, docID, version) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: Same as above in this file too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WaitForMessage return values are used in blip_api_delta_sync_test.go
* Cleanup tests - Create TB() helper functions to make it easier to pick up the right TB function since it can get reset or hard to pick up - use require.EventuallyT instead of testing.T.Fatalf to get tracebacks when they fail. Because these functions fail anyway, removed the return of boolean found. - switch panic to require statements
This is really only to help debug the CBG-3818 ticket part 1, I have a second round of refactoring just for this test to see what documents aren't coming through.
To review:
Pre-review checklist
fmt.Print
,log.Print
, ...)base.UD(docID)
,base.MD(dbName)
)docs/api
Integration Tests
GSI=true,xattrs=true
https://jenkins.sgwdev.com/job/SyncGateway-Integration/2369/