-
Notifications
You must be signed in to change notification settings - Fork 766
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
Add unit test for frontend/admin/handler - part 2 #6003
base: master
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted filessee 21 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
Pull Request Test Coverage Report for Build 018f6355-2db9-4b66-8eea-28d1e6119fb4Details
💛 - Coveralls |
"test-cluster": 1, | ||
}, nil) | ||
mock.EXPECT().GetReplicationMessages(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil, int64(2), nil) | ||
mock.EXPECT().UpdateAckLevel(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil) |
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.
I would suggest not using goock.Any()
on the actual levels, and more generally for all the fields on write operations like this.
You probably want to ensure that if you were to do a big refactor after this test was landed, that if they were to get zero'd out or were wrong or something that the test would catch the problem.
Asserting - particularly in write cases - at least the important values, at both ends is probably worthwhile
Events: &types.DataBlob{}, | ||
}, | ||
hcHandlerFunc: func(mock *history.MockClient) { | ||
mock.EXPECT().ReapplyEvents(gomock.Any(), gomock.Any()).Return(nil) |
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.
same as above, you may want to be a little more specific about what's accepted
What changed?
Add more unit test for frontend/admin/handler
Why?
How did you test it?
Potential risks
Release notes
Documentation Changes