build(core/testing): migrate to go.uber.org/mock
#22314
Conversation
📝 WalkthroughWalkthroughThe changes involve updates to the Changes
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
core/testing/go.mod (1)
Go Version Mismatch and Incomplete Mock Package Migration
- The Go version in
core/testing/go.modis set to1.23, which exceeds the latest stable release (1.21) as of October 2023. Please verify if this is intentional or update it to a supported version.- The migration to
go.uber.org/mockis incomplete. There are remaining imports of the old mock package (github.com/golang/mock/gomock) in multiple files. Please ensure all references to the old mock package are removed.🔗 Analysis chain
Line range hint
3-3: Verify the Go versionThe Go version is set to 1.23, which seems unusually high. As of October 2023, the latest stable version of Go is 1.21. Please verify if this is intentional or if it should be adjusted to a current stable version.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check the Go version used in the project and compare with the one in go.mod # Get the Go version from go.mod go_mod_version=$(grep "^go " core/testing/go.mod | awk '{print $2}') echo "Go version in go.mod: $go_mod_version" # Get the current Go version current_go_version=$(go version | awk '{print $3}' | sed 's/go//') echo "Current Go version: $current_go_version" # Compare versions if [ "$go_mod_version" != "$current_go_version" ]; then echo "Warning: Go version mismatch between go.mod and the current Go installation" fiLength of output: 28593
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
core/testing/go.sumis excluded by!**/*.sum
📒 Files selected for processing (2)
- core/testing/gas/service_mocks.go (7 hunks)
- core/testing/go.mod (1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
core/testing/gas/service_mocks.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments (7)
core/testing/go.mod (1)
8-8: LGTM: Mock package migrationThe change from
github.com/golang/mocktogo.uber.org/mockaligns with the PR objective. This migration is a positive step, asgo.uber.org/mockis actively maintained and provides improved features.However, please ensure that all usages of the mock package in the codebase have been updated to use the new import path and any changed APIs.
#!/bin/bash # Description: Verify that all usages of the mock package have been updated # Check for any remaining imports of the old mock package old_imports=$(rg --type go "github.com/golang/mock") if [ -n "$old_imports" ]; then echo "Warning: Found remaining imports of the old mock package:" echo "$old_imports" else echo "No remaining imports of the old mock package found." fi # Check for usages of the new mock package new_imports=$(rg --type go "go.uber.org/mock") if [ -n "$new_imports" ]; then echo "Found usages of the new mock package:" echo "$new_imports" else echo "Warning: No usages of the new mock package found. Ensure the migration is complete." ficore/testing/gas/service_mocks.go (6)
17-17: LGTM: Import statement updated correctly.The import statement has been successfully updated to use
go.uber.org/mock/gomock, which aligns with the PR objective of migrating togo.uber.org/mock.
24-24: LGTM: Newisgomockfield added.The addition of the
isgomock struct{}field to theMockServicestruct is consistent with the new mock implementation. This field serves as a marker for the mock type and doesn't affect the existing functionality.
53-55: LGTM: Method signature updated to useany.The
GasConfigmethod signature has been updated to useanyinstead ofinterface{}. This change aligns with modern Go practices and improves code readability without altering functionality.
67-69: LGTM: Method signature updated consistently.The
GasMetermethod signature has been updated to useany, consistent with the previous changes. This maintains consistency throughout the mock implementation.
76-76: LGTM: Consistent updates toMockMeterstruct and method signatures.The changes to the
MockMeterstruct and its method signatures are consistent with the previous updates:
- The
isgomock struct{}field has been added to theMockMeterstruct.- The
ConsumeandRefundmethod signatures inMockMeterMockRecorderhave been updated to useany.These changes maintain consistency across the mock implementation and align with modern Go practices.
Also applies to: 105-107, 147-149
Line range hint
1-165: Summary: Successful migration togo.uber.org/mockThe changes in this file successfully implement the migration to
go.uber.org/mock:
- The import statement has been updated to use the new package.
- New
isgomockfields have been added to mock structs for type marking.- Method signatures have been consistently updated to use
anyinstead ofinterface{}.These changes improve code readability and align with modern Go practices without altering the functionality of the mocks. The migration appears to be complete and correct for this file.
See #22315 since the other packages depend on this |
|
Right, but you still need to run make mocks |
the |
|
I'm not sure how to solve the ci issue. if I update the but |
|
I'm going to close this. I will retain the golang/mock for the x/auth module. |
Description
#22313
Many internal packages depends on core/testing, we should migrate it first and then bump the its version for the packages.
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!in the type prefix if API or client breaking changeCHANGELOG.mdReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
Please see Pull Request Reviewer section in the contributing guide for more information on how to review a pull request.
I have...
Summary by CodeRabbit
github.com/golang/mockand addedgo.uber.org/mock.