-
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
Addition of test for TestHandler_UpdateAsyncWorkflowConfiguration #5892
base: master
Are you sure you want to change the base?
Conversation
Pull Request Test Coverage Report for Build 018ec380-10d8-45b7-aa82-c2cd1d358cd6Details
💛 - Coveralls |
Codecov Report
Additional details and impacted filessee 8 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
expectedErr error | ||
}{ | ||
{ | ||
name: "fail to get metadata", |
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.
not a real problem, but just a tip: Genuinely starting with the success case is a bit easier because you typically have to construct the most mocks and return results. And thereafter all the error cases are typically just subsets of the first entry. It just makes copying + pasting easier
LastUpdatedTime: time.Now().Add(-24 * time.Hour).UnixNano(), | ||
}, nil) | ||
|
||
mockDomainManager.EXPECT().UpdateDomain(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.
Sorry to be annoying, but if the test is to really have any serious value at all the updateDomain request should be specified here. Just doing gomock.Any() doesn't really tell you much if anything's broken
What changed?
Test for UpdateAsyncWorkflowConfiguraton
Why?
As part of code coverage week
How did you test it?
Unit test
Potential risks
No risk