update mocks in async package to mockery v2 generated mocks #6220
Conversation
Signed-off-by: Alex Wu <[email protected]>
Code Review Agent Run #067023Actionable Suggestions - 10
Additional Suggestions - 10
Review Details
AI Code Review powered by |
Signed-off-by: Alex Wu <[email protected]>
Changelist by BitoThis pull request implements the following key changes.
|
| mockScheduler := &mocks.EventScheduler{} | ||
| mockScheduler.EXPECT().AddSchedule(mock.Anything, mock.Anything).RunAndReturn( |
There was a problem hiding this comment.
Consider using gomock.NewController(t) and mocks.NewMockEventScheduler instead of directly initializing the mock struct. This ensures proper cleanup and validation of mock expectations.
Code suggestion
Check the AI-generated fix before applying
| mockScheduler := &mocks.EventScheduler{} | |
| mockScheduler.EXPECT().AddSchedule(mock.Anything, mock.Anything).RunAndReturn( | |
| ctrl := gomock.NewController(t) | |
| mockScheduler := mocks.NewMockEventScheduler(ctrl) | |
| mockScheduler.EXPECT().AddSchedule(mock.Anything, mock.Anything).RunAndReturn( |
Code Review Run #067023
Is this a valid issue, or was it incorrectly flagged by the Agent?
- it was incorrectly flagged
| mockScheduler := &mocks.EventScheduler{} | ||
| mockScheduler.EXPECT().RemoveSchedule(mock.Anything, mock.Anything).RunAndReturn( |
There was a problem hiding this comment.
Consider using NewMockEventScheduler() instead of directly instantiating &mocks.EventScheduler{} to ensure proper mock initialization.
Code suggestion
Check the AI-generated fix before applying
| mockScheduler := &mocks.EventScheduler{} | |
| mockScheduler.EXPECT().RemoveSchedule(mock.Anything, mock.Anything).RunAndReturn( | |
| mockScheduler := mocks.NewMockEventScheduler() | |
| mockScheduler.(*mocks.MockEventScheduler).SetRemoveScheduleFunc( |
Code Review Run #067023
Is this a valid issue, or was it incorrectly flagged by the Agent?
- it was incorrectly flagged
| mockScheduler := &mocks.EventScheduler{} | ||
| mockScheduler.EXPECT().RemoveSchedule(mock.Anything, mock.Anything).RunAndReturn( |
There was a problem hiding this comment.
Consider using a more descriptive mock setup by adding expectations for the specific input parameters that RemoveSchedule should receive. This would make the test more explicit about what it's verifying.
Code suggestion
Check the AI-generated fix before applying
| mockScheduler := &mocks.EventScheduler{} | |
| mockScheduler.EXPECT().RemoveSchedule(mock.Anything, mock.Anything).RunAndReturn( | |
| mockScheduler := &mocks.EventScheduler{} | |
| mockScheduler.EXPECT().RemoveSchedule( | |
| mock.MatchedBy(func(ctx context.Context) bool { return true }), | |
| mock.MatchedBy(func(input scheduleInterfaces.RemoveScheduleInput) bool { return input.Identifier == launchPlanNamedIdentifier }), | |
| ).RunAndReturn( |
Code Review Run #067023
Is this a valid issue, or was it incorrectly flagged by the Agent?
- it was incorrectly flagged
| @@ -816,8 +908,8 @@ func TestDisableLaunchPlan(t *testing.T) { | |||
| } | |||
|
|
|||
| var removeScheduleFuncCalled bool | |||
| mockScheduler := mocks.NewMockEventScheduler() | |||
| mockScheduler.(*mocks.MockEventScheduler).SetRemoveScheduleFunc( | |||
| mockScheduler := &mocks.EventScheduler{} | |||
There was a problem hiding this comment.
Consider using NewMockEventScheduler() constructor instead of direct struct initialization for mockScheduler to ensure proper mock initialization.
Code suggestion
Check the AI-generated fix before applying
| mockScheduler := &mocks.EventScheduler{} | |
| mockScheduler := mocks.NewMockEventScheduler() |
Code Review Run #067023
Is this a valid issue, or was it incorrectly flagged by the Agent?
- it was incorrectly flagged
| @@ -65,6 +66,7 @@ func TestSandboxProcessor_StartProcessingMessageError(t *testing.T) { | |||
| } | |||
|
|
|||
| func TestSandboxProcessor_StartProcessingEmailError(t *testing.T) { | |||
| mockSandboxEmailer := mocks.Emailer{} | |||
There was a problem hiding this comment.
Consider using mockSandboxEmailer := mocks.NewEmailer() instead of empty struct initialization to ensure proper mock setup. The mock expectations are being set using testify's mock package which requires proper initialization.
Code suggestion
Check the AI-generated fix before applying
| mockSandboxEmailer := mocks.Emailer{} | |
| mockSandboxEmailer := mocks.NewEmailer() |
Code Review Run #067023
Is this a valid issue, or was it incorrectly flagged by the Agent?
- it was incorrectly flagged
| func (_m *Publisher) EXPECT() *Publisher_Expecter { | ||
| return &Publisher_Expecter{mock: &_m.Mock} |
There was a problem hiding this comment.
Consider whether the mock expectation pattern being introduced with EXPECT() aligns with the existing mock usage pattern. The change removes OnPublish and OnPublishMatch methods in favor of an expecter pattern which may impact existing tests.
Code suggestion
Check the AI-generated fix before applying
| func (_m *Publisher) EXPECT() *Publisher_Expecter { | |
| return &Publisher_Expecter{mock: &_m.Mock} | |
| func (_m *Publisher) OnPublish(ctx context.Context, notificationType string, msg protoiface.MessageV1) *Publisher_Publish { | |
| c_call := _m.On("Publish", ctx, notificationType, msg) | |
| return &Publisher_Publish{Call: c_call} | |
| } | |
| func (_m *Publisher) OnPublishMatch(matchers ...interface{}) *Publisher_Publish { | |
| c_call := _m.On("Publish", matchers...) | |
| return &Publisher_Publish{Call: c_call} |
Code Review Run #067023
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 Publish") | ||
| } | ||
|
|
There was a problem hiding this comment.
Consider adding error handling for the panic case in the mock Publish method. A panic could make tests harder to debug. Maybe return an error instead?
Code suggestion
Check the AI-generated fix before applying
| if len(ret) == 0 { | |
| panic("no return value specified for Publish") | |
| } | |
| if len(ret) == 0 { | |
| return fmt.Errorf("no return value specified for Publish") | |
| } | |
Code Review Run #067023
Is this a valid issue, or was it incorrectly flagged by the Agent?
- it was incorrectly flagged
| func (_c *Processor_StartProcessing_Call) Return() *Processor_StartProcessing_Call { | ||
| _c.Call.Return() | ||
| return _c |
There was a problem hiding this comment.
The Return() method implementation appears to be incomplete. Consider returning the mock call results to maintain consistency with other mock implementations.
Code suggestion
Check the AI-generated fix before applying
| func (_c *Processor_StartProcessing_Call) Return() *Processor_StartProcessing_Call { | |
| _c.Call.Return() | |
| return _c | |
| func (_c *Processor_StartProcessing_Call) Return(results ...interface{}) *Processor_StartProcessing_Call { | |
| _c.Call.Return(results...) | |
| return _c |
Code Review Run #067023
Is this a valid issue, or was it incorrectly flagged by the Agent?
- it was incorrectly flagged
| @@ -78,7 +78,7 @@ var executionIdentifier = core.WorkflowExecutionIdentifier{ | |||
| Domain: "domain", | |||
| Name: "name", | |||
| } | |||
| var mockPublisher notificationMocks.MockPublisher | |||
| var mockPublisher notificationMocks.Publisher | |||
There was a problem hiding this comment.
Consider if using the concrete interface notificationMocks.Publisher instead of the mock type notificationMocks.MockPublisher is intentional, as this could affect test behavior.
Code suggestion
Check the AI-generated fix before applying
| var mockPublisher notificationMocks.Publisher | |
| var mockPublisher notificationMocks.MockPublisher |
Code Review Run #067023
Is this a valid issue, or was it incorrectly flagged by the Agent?
- it was incorrectly flagged
Signed-off-by: Alex Wu <[email protected]>
Code Review Agent Run #41e83eActionable Suggestions - 1
Review Details
AI Code Review powered by |
| req = &admin.WorkflowExecutionEventRequest{ | ||
| RequestId: "1", | ||
| Event: &event.WorkflowExecutionEvent{ | ||
| ExecutionId: &executionIdentifier, | ||
| Phase: core.WorkflowExecution_QUEUED, | ||
| OccurredAt: timestamppb.New(time.Now()), | ||
| }, | ||
| } |
There was a problem hiding this comment.
Consider consolidating the repeated workflow execution event request creation into a helper function to reduce code duplication. The same request structure is created multiple times with only the phase changing.
Code suggestion
Check the AI-generated fix before applying
- req := &admin.WorkflowExecutionEventRequest{
- RequestId: "1",
- Event: &event.WorkflowExecutionEvent{
- ExecutionId: &executionIdentifier,
- Phase: core.WorkflowExecution_ABORTED,
- OccurredAt: timestamppb.New(time.Now()),
- },
- }
+ req := createTestWorkflowEvent(core.WorkflowExecution_ABORTED)
- req = &admin.WorkflowExecutionEventRequest{
- RequestId: "1",
- Event: &event.WorkflowExecutionEvent{
- ExecutionId: &executionIdentifier,
- Phase: core.WorkflowExecution_QUEUED,
- OccurredAt: timestamppb.New(time.Now()),
- },
- }
+ req = createTestWorkflowEvent(core.WorkflowExecution_QUEUED)
- req = &admin.WorkflowExecutionEventRequest{
- RequestId: "1",
- Event: &event.WorkflowExecutionEvent{
- ExecutionId: &executionIdentifier,
- Phase: core.WorkflowExecution_RUNNING,
- OccurredAt: timestamppb.New(time.Now()),
- },
- }
+ req = createTestWorkflowEvent(core.WorkflowExecution_RUNNING)
Code Review Run #41e83e
Is this a valid issue, or was it incorrectly flagged by the Agent?
- it was incorrectly flagged
|
@eapolinario I saw you review #6197. Mind also reviewing this one? |
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/async 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 modernizes FlyteAdmin's async package by migrating from manual mocks to Mockery v2 generated mocks. The update includes adding mockery-v2 generation directives, regenerating mocks using mockery v2.40.3, and refactoring test files with new mock syntax. These changes enhance type safety, improve testing infrastructure, and reduce maintenance overhead through auto-generated implementations.Unit tests added: True
Estimated effort to review (1-5, lower is better): 5