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.

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

Ensure generated CI mocks are up to date #5839

Merged

Conversation

pocaeus
Copy link
Contributor

@pocaeus pocaeus commented May 31, 2023

Description

Adds make targets to generate mocks, and adds configuration to run checks in CI to make sure generated mocks are included in a pull request.

Tests

No tests for make and CI stuff 🤷

Invariants

N/A

Additional context

N/A

Metadata

  • Fixes #[54]

TODOs

@pocaeus pocaeus requested review from a team as code owners May 31, 2023 14:47
@pocaeus pocaeus requested a review from mslipper May 31, 2023 14:47
@changeset-bot
Copy link

changeset-bot bot commented May 31, 2023

⚠️ No Changeset found

Latest commit: 5fe5f53

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@netlify
Copy link

netlify bot commented May 31, 2023

Deploy Preview for opstack-docs canceled.

Name Link
🔨 Latest commit 5fe5f53
🔍 Latest deploy log https://app.netlify.com/sites/opstack-docs/deploys/648127aecd0704000875b1cb

@pocaeus
Copy link
Contributor Author

pocaeus commented May 31, 2023

The two checks I just added are currently failing because mockery is not available in the ci-builder container. I can add it to the Dockerfile, but I'm unsure how to push up those changes.

Alternatively I can install mockery in the CI job itself which is probably the easiest/fastest route but feels a little wasteful of CI time 🙂. Let me know what's preferable!

@smartcontracts
Copy link
Contributor

Ok just waiting on a build/publish of the ci-builder image! Will keep you updated.

@smartcontracts
Copy link
Contributor

@Lemma199 can you rebase this on the latest develop? Mockery should now be included in the image.

@pocaeus
Copy link
Contributor Author

pocaeus commented Jun 1, 2023

Thanks @smartcontracts, done!

@pocaeus
Copy link
Contributor Author

pocaeus commented Jun 1, 2023

Failing due to some diffs (basically comments):

-// Code generated by mockery v2.23.1. DO NOT EDIT.
+// Code generated by mockery v2.28.1. DO NOT EDIT.

There's also some code being removed (presumably stale):

diff --git a/op-node/p2p/mocks/GossipMetricer.go b/op-node/p2p/mocks/GossipMetricer.go
index 38ebf8d50..d5da64382 100644
--- a/op-node/p2p/mocks/GossipMetricer.go
+++ b/op-node/p2p/mocks/GossipMetricer.go
@@ -1,4 +1,4 @@
-// Code generated by mockery v2.22.1. DO NOT EDIT.
+// Code generated by mockery v2.28.1. DO NOT EDIT.
 
 package mocks
 
@@ -14,11 +14,6 @@ func (_m *GossipMetricer) RecordGossipEvent(evType int32) {
 	_m.Called(evType)
 }
 
-// SetPeerScores provides a mock function with given fields: _a0
-func (_m *GossipMetricer) SetPeerScores(_a0 map[string]float64) {
-	_m.Called(_a0)
-}
-

I can apply these changes and push up a new commit that includes them — I couldn't find what version (if any) documentation for this repository suggests to use for mockery, but moving forward CI would block a PR generating mocks if the user is not using the same version as the ci-builder container. Is that alright?

Alternatively I can modify the CI task to run with the disable-version-string configuration which would remove these version strings entirely but users would have to use the same configuration locally as well 🤷

@trianglesphere
Copy link
Contributor

couldn't find what version (if any) documentation for this repository suggests to use for mockery

We don't have one. It was pretty recently added & hasn't been standardized. Individual devs have been responsible for keeping everything up to date. It's probably fine to requires a specific version if we let people know.

@ajsutton
Copy link
Contributor

ajsutton commented Jun 4, 2023

The GossipMetricer.go updates will cause some knock-on problems in peer_scorer_test.go which should actually be using a ScoreMetrics mock rather than GossipMetricer - I suspect some refactoring happened and because the mock wasn't updated, the test continued working. I ran into it with #5870 so it has the updates to peer_scorer_test required to make it work. I think it's probably better to pull the required changes in here since that's what they're really related to. I have some more testing to do on that other PR before it's ready to merge anyway and it won't be hard for me to sort out conflicts.

Good sign we need this check - thanks for picking it up. :)

@pocaeus
Copy link
Contributor Author

pocaeus commented Jun 5, 2023

Thanks for the heads up @ajsutton. I'm happy to wait for #5870 to be merged and then can rebase this PR, I don't mind leaving this open.

@mergify
Copy link
Contributor

mergify bot commented Jun 6, 2023

Hey @Lemma199! This PR has merge conflicts. Please fix them before continuing review.

@mergify mergify bot added the conflict label Jun 6, 2023
@ajsutton
Copy link
Contributor

ajsutton commented Jun 6, 2023

That merge conflict is because #5870 just merged which should resolve the GossipMetricer mock issues. There may be other mocks out of date as well but hopefully just regenerating them with the right version will be all that's required to get it all green.

@mergify mergify bot removed the conflict label Jun 6, 2023
@pocaeus
Copy link
Contributor Author

pocaeus commented Jun 6, 2023

Fixed the conflicts and updated the mocks using 2.28.1. Should be good to go now! Thanks @ajsutton

Copy link
Contributor

@ajsutton ajsutton left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for this.

@mergify
Copy link
Contributor

mergify bot commented Jun 8, 2023

This PR has been added to the merge queue, and will be merged soon.

@mergify
Copy link
Contributor

mergify bot commented Jun 8, 2023

This PR is next in line to be merged, and will be merged as soon as checks pass.

1 similar comment
@mergify
Copy link
Contributor

mergify bot commented Jun 8, 2023

This PR is next in line to be merged, and will be merged as soon as checks pass.

@OptimismBot OptimismBot merged commit 5e3219c into ethereum-optimism:develop Jun 8, 2023
80 of 83 checks passed
@mergify mergify bot removed the on-merge-train label Jun 8, 2023
@pocaeus pocaeus deleted the ci-generated-mock-check branch June 8, 2023 13:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants