Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Use generated mocks in flyteadmin #6197

Merged
merged 4 commits into from
Jan 31, 2025
Merged

Use generated mocks in flyteadmin #6197

merged 4 commits into from
Jan 31, 2025

Conversation

popojk
Copy link
Contributor

@popojk popojk commented Jan 28, 2025 •
edited by flyte-bot
Loading

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/manager to use Mockery v2 generated mocks and includes modifications to the related test cases to ensure compatibility.

Check all the applicable boxes

  • All new and existing tests passed.
  • All commits are signed-off.

Summary by Bito

This PR implements a comprehensive migration from manually written mocks to Mockery v2 generated mocks across the FlyteAdmin codebase, particularly in the manager package. The changes update mock implementations to use testify/mock package with generated interfaces, implementing EXPECT().Method() pattern. This modernization improves type safety, standardizes mock implementations, and reduces manual mock maintenance overhead.

Unit tests added: False

Estimated effort to review (1-5, lower is better): 5

@flyte-bot
Copy link
Collaborator

flyte-bot commented Jan 28, 2025 •
edited
Loading

Code Review Agent Run #36a7e2

Actionable Suggestions - 14
  • flyteadmin/dataproxy/service_test.go - 1
  • flyteadmin/pkg/rpc/adminservice/tests/launch_plan_test.go - 2
    • Consider pointer initialization for mock interface · Line 47-48
    • Improper mock interface initialization detected · Line 100-101
  • flyteadmin/pkg/manager/mocks/metrics_interface.go - 1
    • Mock RunAndReturn error handling improvement · Line 81-83
  • flyteadmin/pkg/manager/mocks/resource_interface.go - 3
    • Consider adding type assertion error handling · Line 188-192
    • Consider revising RunAndReturn implementation approach · Line 673-674
    • Mock RunAndReturn implementation may be incorrect · Line 673-673
  • flyteadmin/pkg/rpc/adminservice/tests/project_domain_test.go - 1
    • Consider using proper mock initialization method · Line 93-93
  • flyteadmin/pkg/manager/mocks/node_execution_interface.go - 1
    • Consider adding error handling for RunAndReturn · Line 376-378
  • flyteadmin/pkg/manager/mocks/version_interface.go - 1
    • Mock RunAndReturn implementation type conversion · Line 81-83
  • flyteadmin/pkg/rpc/adminservice/tests/node_execution_test.go - 1
    • Consider proper mock interface initialization · Line 225-225
  • flyteadmin/pkg/manager/impl/execution_manager_test.go - 2
  • flyteadmin/pkg/manager/impl/executions/quality_of_service_test.go - 1
Additional Suggestions - 10
  • flyteadmin/pkg/manager/mocks/resource_interface.go - 3
  • flyteadmin/pkg/manager/mocks/metrics_interface.go - 1
  • flyteadmin/pkg/rpc/adminservice/tests/project_test.go - 2
    • Consider using mock constructor method · Line 18-19
    • Consider proper mock initialization pattern · Line 64-64
  • flyteadmin/pkg/rpc/adminservice/tests/task_test.go - 2
    • Consider using mock constructor method · Line 77-78
    • Consider using mock constructor for initialization · Line 49-50
  • flyteadmin/dataproxy/service_test.go - 2
Review Details
  • Files reviewed - 38 · Commit Range: 1a35e79..3f86dca
    • flyteadmin/dataproxy/service_test.go
    • flyteadmin/pkg/async/schedule/aws/workflow_executor_test.go
    • flyteadmin/pkg/clusterresource/impl/db_admin_data_provider_test.go
    • flyteadmin/pkg/manager/impl/execution_manager_test.go
    • flyteadmin/pkg/manager/impl/executions/quality_of_service_test.go
    • flyteadmin/pkg/manager/impl/metrics_manager_test.go
    • flyteadmin/pkg/manager/impl/util/resources_test.go
    • flyteadmin/pkg/manager/impl/util/shared_test.go
    • flyteadmin/pkg/manager/impl/util/single_task_execution_test.go
    • flyteadmin/pkg/manager/mocks/execution.go
    • flyteadmin/pkg/manager/mocks/execution_interface.go
    • flyteadmin/pkg/manager/mocks/launch_plan.go
    • flyteadmin/pkg/manager/mocks/launch_plan_interface.go
    • flyteadmin/pkg/manager/mocks/metrics_interface.go
    • flyteadmin/pkg/manager/mocks/named_entity.go
    • flyteadmin/pkg/manager/mocks/named_entity_interface.go
    • flyteadmin/pkg/manager/mocks/node_execution.go
    • flyteadmin/pkg/manager/mocks/node_execution_interface.go
    • flyteadmin/pkg/manager/mocks/project.go
    • flyteadmin/pkg/manager/mocks/project_interface.go
    • flyteadmin/pkg/manager/mocks/resource.go
    • flyteadmin/pkg/manager/mocks/resource_interface.go
    • flyteadmin/pkg/manager/mocks/task.go
    • flyteadmin/pkg/manager/mocks/task_execution.go
    • flyteadmin/pkg/manager/mocks/task_execution_interface.go
    • flyteadmin/pkg/manager/mocks/task_interface.go
    • flyteadmin/pkg/manager/mocks/version_interface.go
    • flyteadmin/pkg/manager/mocks/workflow.go
    • flyteadmin/pkg/manager/mocks/workflow_interface.go
    • flyteadmin/pkg/rpc/adminservice/tests/execution_test.go
    • flyteadmin/pkg/rpc/adminservice/tests/launch_plan_test.go
    • flyteadmin/pkg/rpc/adminservice/tests/node_execution_test.go
    • flyteadmin/pkg/rpc/adminservice/tests/project_domain_test.go
    • flyteadmin/pkg/rpc/adminservice/tests/project_test.go
    • flyteadmin/pkg/rpc/adminservice/tests/task_execution_test.go
    • flyteadmin/pkg/rpc/adminservice/tests/task_test.go
    • flyteadmin/pkg/rpc/adminservice/tests/util.go
    • flyteadmin/pkg/rpc/adminservice/tests/workflow_test.go
  • Files skipped - 0
  • Tools
    • Golangci-lint (Linter) - ✖︎ Failed
    • Whispers (Secret Scanner) - ✔︎ Successful
    • Detect-secrets (Secret Scanner) - ✔︎ Successful

AI Code Review powered by Bito Logo

@codecov Codecov
Copy link

codecov bot commented Jan 28, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 25.04240% with 1326 lines in your changes missing coverage. Please review.

Project coverage is 36.90%. Comparing base (ab463c3) to head (db6e5cf).
Report is 11 commits behind head on master.

Files with missing lines Patch % Lines
...lyteadmin/pkg/manager/mocks/execution_interface.go 32.81% 209 Missing and 8 partials ⚠️
...teadmin/pkg/manager/mocks/launch_plan_interface.go 21.34% 195 Missing and 4 partials ⚠️
flyteadmin/pkg/manager/mocks/resource_interface.go 23.04% 162 Missing and 5 partials ⚠️
...dmin/pkg/manager/mocks/node_execution_interface.go 30.73% 146 Missing and 5 partials ⚠️
flyteadmin/pkg/manager/mocks/project_interface.go 23.56% 130 Missing and 3 partials ⚠️
flyteadmin/pkg/manager/mocks/task_interface.go 18.91% 118 Missing and 2 partials ⚠️
flyteadmin/pkg/manager/mocks/workflow_interface.go 18.91% 118 Missing and 2 partials ⚠️
...eadmin/pkg/manager/mocks/named_entity_interface.go 13.27% 97 Missing and 1 partial ⚠️
...dmin/pkg/manager/mocks/task_execution_interface.go 36.48% 90 Missing and 4 partials ⚠️
flyteadmin/pkg/manager/mocks/version_interface.go 0.00% 27 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #6197      +/-   ##
==========================================
- Coverage   37.06%   36.90%   -0.16%     
==========================================
  Files        1318     1317       -1     
  Lines      132644   134058    +1414     
==========================================
+ Hits        49164    49476     +312     
- Misses      79230    80289    +1059     
- Partials     4250     4293      +43
Flag Coverage Δ
unittests-datacatalog 51.58% <ø> (ø)
unittests-flyteadmin 52.48% <25.04%> (-1.86%) ⬇️
unittests-flytecopilot 30.99% <ø> (ø)
unittests-flytectl 62.33% <ø> (+0.04%) ⬆️
unittests-flyteidl 7.23% <ø> (ø)
unittests-flyteplugins 53.87% <ø> (+0.01%) ⬆️
unittests-flytepropeller 42.73% <ø> (+0.02%) ⬆️
unittests-flytestdlib 55.35% <ø> (+0.05%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@flyte-bot
Copy link
Collaborator

flyte-bot commented Jan 28, 2025 •
edited
Loading

Changelist by Bito

This pull request implements the following key changes.

Key Change Files Impacted
Testing - Test Framework Modernization

- service_test.go - Updated mock implementations to use testify/mock interfaces

- workflow_executor_test.go - Migrated to testify/mock based workflow execution tests

- db_admin_data_provider_test.go - Converted resource manager mocks to use testify/mock

- execution_manager_test.go - Refactored execution manager tests to use generated mocks

- quality_of_service_test.go - Updated QoS tests to use new mock interfaces

- metrics_manager_test.go - Migrated metrics manager tests to use generated mocks

- resources_test.go - Updated resource utility tests with new mock patterns
Testing - Mock Framework Migration

- shared_test.go - Updated resource manager mock implementation to use testify/mock

- single_task_execution_test.go - Migrated workflow and named entity manager mocks to use testify/mock

- execution.go - Added mockery-v2 generation directive for ExecutionInterface

- launch_plan.go - Added mockery-v2 generation directive for LaunchPlanInterface

- named_entity.go - Added mockery-v2 generation directive for NamedEntityInterface

- node_execution.go - Added mockery-v2 generation directive for NodeExecutionInterface

- project.go - Added mockery-v2 generation directive for ProjectInterface

- resource.go - Updated mockery generation directive to use v2 with expecter

- task.go - Added mockery-v2 generation directive for TaskInterface

- task_execution.go - Added mockery-v2 generation directive for TaskExecutionInterface

- version.go - Updated mockery generation directive to use v2 with expecter

- workflow.go - Added mockery-v2 generation directive for WorkflowInterface

- execution.go - Removed manual mock implementation in favor of generated code
Testing - Mock Framework Migration

- execution_interface.go - Added generated mock implementation for ExecutionInterface using Mockery v2

- launch_plan.go - Removed manual mock implementation in favor of generated code

- launch_plan_interface.go - Added generated mock implementation for LaunchPlanInterface using Mockery v2
Testing - Mock Framework Migration - Core Components

- launch_plan_interface.go - Added generated mock implementation for LaunchPlanInterface with Mockery v2

- named_entity_interface.go - Added generated mock implementation for NamedEntityInterface with Mockery v2

- node_execution_interface.go - Added generated mock implementation for NodeExecutionInterface with Mockery v2
Testing - Legacy Mock Cleanup

- named_entity.go - Removed manual mock implementation in favor of generated code

- node_execution.go - Removed manual mock implementation in favor of generated code
Testing - Mock Framework Migration - Node Execution

- node_execution_interface.go - Added generated mock implementation for NodeExecutionInterface with Mockery v2
Testing - Mock Framework Migration - Project Management

- project.go - Removed manual mock implementation in favor of generated code

- project_interface.go - Added generated mock implementation for ProjectInterface using Mockery v2
Testing - Mock Framework Migration - Resource Management

- resource.go - Removed manual mock implementation in favor of generated code

- resource_interface.go - Updated mock implementation to use Mockery v2 with expecter pattern
Testing - Mock Framework Migration - Resource Management

- resource_interface.go - Updated mock implementation with Mockery v2 generated code using expecter pattern and type-safe interfaces

- task.go - Removed manual mock implementation in favor of generated code
Testing - Mock Framework Migration - Task Management

- task_execution.go - Removed manual mock implementation in favor of generated code

- task_execution_interface.go - Added generated mock implementation for TaskExecutionInterface using Mockery v2

- task_interface.go - Added generated mock implementation for TaskInterface using Mockery v2
Testing - Mock Framework Migration - Version Management

- version_interface.go - Updated mock implementation to use Mockery v2.40.3 with expecter pattern

- workflow.go - Removed manual mock implementation in favor of generated code
Testing - Mock Framework Migration - Workflow Interface

- workflow_interface.go - Added generated mock implementation for WorkflowInterface using Mockery v2.40.3
Testing - Test Framework Modernization - Execution Tests

- execution_test.go - Updated execution tests to use new mock interfaces with EXPECT() pattern

- node_execution_test.go - Migrated node execution tests to use generated mocks

- task_execution_test.go - Updated task execution tests to use new mock interfaces
Testing - Test Framework Modernization - Project and Launch Plan Tests

- project_domain_test.go - Updated project domain tests to use ResourceInterface mocks

- project_test.go - Migrated project tests to use ProjectInterface mocks

- launch_plan_test.go - Updated launch plan tests to use LaunchPlanInterface mocks
Testing - Test Framework Modernization - Task and Workflow Tests

- task_execution_test.go - Updated task execution tests to use TaskExecutionInterface with EXPECT() pattern

- task_test.go - Migrated task tests to use TaskInterface with mock expectations

- workflow_test.go - Updated workflow tests to use WorkflowInterface with EXPECT() pattern

- util.go - Updated mock interface types in admin server input structure

@flyte-bot
Copy link
Collaborator

flyte-bot commented Jan 28, 2025 •
edited
Loading

Code Review Agent Run #ccbc98

Actionable Suggestions - 0
Review Details
  • Files reviewed - 12 · Commit Range: 3f86dca..db6e5cf
    • flyteadmin/pkg/manager/impl/execution_manager_test.go
    • flyteadmin/pkg/manager/interfaces/execution.go
    • flyteadmin/pkg/manager/interfaces/launch_plan.go
    • flyteadmin/pkg/manager/interfaces/named_entity.go
    • flyteadmin/pkg/manager/interfaces/node_execution.go
    • flyteadmin/pkg/manager/interfaces/project.go
    • flyteadmin/pkg/manager/interfaces/resource.go
    • flyteadmin/pkg/manager/interfaces/task.go
    • flyteadmin/pkg/manager/interfaces/task_execution.go
    • flyteadmin/pkg/manager/interfaces/version.go
    • flyteadmin/pkg/manager/interfaces/workflow.go
    • flyteadmin/pkg/manager/mocks/metrics_interface.go
  • Files skipped - 0
  • Tools
    • Golangci-lint (Linter) - ✖︎ Failed
    • Whispers (Secret Scanner) - ✔︎ Successful
    • Detect-secrets (Secret Scanner) - ✔︎ Successful

AI Code Review powered by Bito Logo

eapolinario
eapolinario approved these changes Jan 31, 2025
View reviewed changes
Copy link
Contributor

@eapolinario eapolinario left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@eapolinario eapolinario merged commit 2d457df into flyteorg:master Jan 31, 2025
51 of 53 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@flyte-bot flyte-bot flyte-bot left review comments

@eapolinario eapolinario eapolinario approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@popojk @flyte-bot @eapolinario