use mockery generate mock in clusterresource pkg #6243
Conversation
Signed-off-by: Alex Wu <[email protected]>
Code Review Agent Run #5e81f2Actionable Suggestions - 4
Additional Suggestions - 2
Review Details
AI Code Review powered by |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6243 +/- ##
===========================================
- Coverage 50.51% 36.87% -13.65%
===========================================
Files 1162 1318 +156
Lines 91811 134731 +42920
===========================================
+ Hits 46375 49676 +3301
- Misses 41340 80726 +39386
- Partials 4096 4329 +233
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Changelist by BitoThis pull request implements the following key changes.
|
| panic("no return value specified for GetClusterResourceAttributes") | ||
| } | ||
|
|
There was a problem hiding this comment.
Consider keeping the OnGetClusterResourceAttributesMatch method alongside the new EXPECT() method as they serve different purposes in mock expectations. The removal might break existing tests that rely on this method.
Code suggestion
Check the AI-generated fix before applying
@@ -31,3 +22,5 @@
+func (_m *FlyteAdminDataProvider) EXPECT() *FlyteAdminDataProvider_Expecter {
+ return &FlyteAdminDataProvider_Expecter{mock: &_m.Mock}
+}
+
+func (_m *FlyteAdminDataProvider) OnGetClusterResourceAttributesMatch(matchers ...interface{}) *FlyteAdminDataProvider_GetClusterResourceAttributes {
+ c_call := _m.On("GetClusterResourceAttributes", matchers...)
+ return &FlyteAdminDataProvider_GetClusterResourceAttributes{Call: c_call}
+}
Code Review Run #5e81f2
Is this a valid issue, or was it incorrectly flagged by the Agent?
- it was incorrectly flagged
| if len(ret) == 0 { | ||
| panic("no return value specified for GetProjects") | ||
| } | ||
|
|
There was a problem hiding this comment.
Consider handling the empty return value case more gracefully instead of using panic(). A panic in mock code could disrupt test execution unnecessarily. Consider returning a meaningful error instead.
Code suggestion
Check the AI-generated fix before applying
| if len(ret) == 0 { | |
| panic("no return value specified for GetProjects") | |
| } | |
| if len(ret) == 0 { | |
| return nil, fmt.Errorf("no return value specified for GetProjects") | |
| } | |
Code Review Run #5e81f2
Is this a valid issue, or was it incorrectly flagged by the Agent?
- it was incorrectly flagged
Tracking issue
Related to #149
Why are the changes needed?
FlyteAdmin currently relies on manually crafted mocks, which are cumbersome to maintain and extend for new interfaces. Switching to Mockery v2 generated mocks is a more efficient approach, eliminating repetitive boilerplate code and streamlining the development process.
What changes were proposed in this pull request?
This PR updates the mocks in flyteadmin/pkg/clusterresource to use Mockery v2 generated mocks and includes modifications to the related test cases to ensure compatibility.
Check all the applicable boxes
Summary by Bito
This PR updates the clusterresource package by migrating from manual mocks to mockery v2 generated mocks. The implementation includes regenerating mocks with mockery v2.40.3 and updating test cases to use the new EXPECT() syntax, enhancing the maintainability of the testing infrastructure.Unit tests added: False
Estimated effort to review (1-5, lower is better): 2