Switch workflowengine compiler mock to mockery generated #6261
Conversation
Signed-off-by: Arthur <[email protected]>
…r_test the AddCompileTaskCallback is only used in old hand roll mock, nowhere else is using it, fine to remove it Signed-off-by: Arthur <[email protected]>
…rate into compiler Signed-off-by: Arthur <[email protected]>
Code Review Agent Run #285e27Actionable Suggestions - 2
Review Details
AI Code Review powered by |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6261 +/- ##
==========================================
- Coverage 36.86% 36.85% -0.01%
==========================================
Files 1318 1318
Lines 134773 134821 +48
==========================================
+ Hits 49683 49695 +12
- Misses 80758 80782 +24
- Partials 4332 4344 +12
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.
|
| mockCompiler.On("GetRequirements", mock.AnythingOfType("*core.WorkflowTemplate"), mock.AnythingOfType("[]*core.WorkflowTemplate")).Return(func(fg *core.WorkflowTemplate, subWfs []*core.WorkflowTemplate) (compiler.WorkflowExecutionRequirements, error) { | ||
| return compiler.WorkflowExecutionRequirements{}, nil | ||
| }) |
There was a problem hiding this comment.
Consider using mockCompiler.On() with explicit return values instead of using callback functions for better readability and maintainability. The current implementation using callback functions makes the test setup more complex than necessary.
Code suggestion
Check the AI-generated fix before applying
| mockCompiler.On("GetRequirements", mock.AnythingOfType("*core.WorkflowTemplate"), mock.AnythingOfType("[]*core.WorkflowTemplate")).Return(func(fg *core.WorkflowTemplate, subWfs []*core.WorkflowTemplate) (compiler.WorkflowExecutionRequirements, error) { | |
| return compiler.WorkflowExecutionRequirements{}, nil | |
| }) | |
| mockCompiler.On("GetRequirements", mock.AnythingOfType("*core.WorkflowTemplate"), mock.AnythingOfType("[]*core.WorkflowTemplate")).Return(compiler.WorkflowExecutionRequirements{}, nil) |
Code Review Run #285e27
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| mockCompiler := &workflowMocks.Compiler{} | ||
| expectedErr := errors.New("expected error") | ||
| mockCompiler.(*workflowMocks.MockCompiler).AddCompileTaskCallback( | ||
| func(task *core.TaskTemplate) (*core.CompiledTask, error) { | ||
| return nil, expectedErr | ||
| }) | ||
| mockCompiler.On("CompileTask", mock.AnythingOfType("*core.TaskTemplate")).Return(func(taskTemplate *core.TaskTemplate) (*core.CompiledTask, error) { | ||
| return nil, expectedErr | ||
| }) |
There was a problem hiding this comment.
Consider using testify/mock functionality more idiomatically. Instead of using a callback function with Return, you could use mock.MatchedBy with direct return values for better readability and maintainability.
Code suggestion
Check the AI-generated fix before applying
- mockCompiler := &workflowMocks.Compiler{}
- mockCompiler.On("CompileTask", mock.AnythingOfType("*core.TaskTemplate")).Return(func(taskTemplate *core.TaskTemplate) (*core.CompiledTask, error) {
- return nil, expectedErr
- })
+ mockCompiler := &workflowMocks.Compiler{}
+ mockCompiler.On("CompileTask", mock.MatchedBy(func(*core.TaskTemplate) bool { return true })).Return(nil, expectedErr)
Code Review Run #285e27
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
Signed-off-by: Arthur <[email protected]>
luckyarthur
force-pushed
the
switch-workflowengine-compiler-mock-to-mockery-generated
branch
from
41ca1a2 to
5db164d
Compare
Signed-off-by: Arthur <[email protected]>
Code Review Agent Run #74cc11Actionable Suggestions - 1
Review Details
AI Code Review powered by |
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, also add the go generate comment which automates update of mocks
What changes were proposed in this pull request?
this PR includes changes of mock for flyteadmin/pkg/workflowengine/interfaces/compiler.go, and update unit test usage of the new mockery generated mock
Check all the applicable boxes
Summary by Bito
Modernization of testing infrastructure by replacing manual mocks with Mockery v2 generated mocks for the workflow engine compiler interface. The update includes adding go:generate directives and implementing new Mockery v2-based mocks. The changes streamline the codebase by removing old manual mock implementations while enhancing maintainability and consistency of test files.Unit tests added: True
Estimated effort to review (1-5, lower is better): 2