range{feed,client},roachpb: standardize mock generation through bazel #67226
Conversation
|
This change is |
|
Actually, hold off on looking. Clearly broke something and only just figured out how to run the bazel docker image locally. |
irfansharif
force-pushed
the
210705.bazel-gomocks
branch
from
0cf62b7 to
29a7bc4
Compare
irfansharif
requested review from
a team and
pbardea
and removed request for
a team and
pbardea
irfansharif
force-pushed
the
210705.bazel-gomocks
branch
from
29a7bc4 to
cd06eba
Compare
|
I'm two ways about this PR. The benefit it offers over the status quo (currently checked into master) is that gazelle will now automatically keep the "base" go_library targets up-to-date. For e.g., right now any file/dep changes to |
pkg/kv/kvclient/kvcoord/BUILD.bazel
Outdated
| # gazelle:resolve go github.com/cockroachdb/cockroach/pkg/kv/kvclient/kvcoord //pkg/kv/kvclient/kvcoord:noop | ||
| go_library( | ||
| name = "noop", | ||
| importpath = "noop", |
There was a problem hiding this comment.
This is pretty ugly. I think you're having to do this because pkg/kv/kvclient/kvcoord is a package that contains both internal and external tests, right?
The blessed way to do that would be to have two separate test targets, one for the external tests and one for the internal tests. Unfortunately Gazelle doesn't do that for us. (Not sure why not.) I'm hesitant to do this ugly thing because now we're trying to get rules_go to do something it doesn't want to do so our tests don't fail.
Bare minimum, you can add a new go_library(name = "noop", importpath = "noop", visibility = ["//pkg:__subpackages__"]) in build/bazelutil/BUILD.bazel and reference that everywhere we want an empty go_library.
There was a problem hiding this comment.
Done.
The blessed way to do that would be to have two separate test targets, one for the external tests and one for the internal tests. Unfortunately Gazelle doesn't do that for us. (Not sure why not.)
Having separate test targets used to be a thing before, and was changed at some point: bazel-contrib/bazel-gazelle#102. Unsure why, brevity maybe? I assume go_test is internally capturing the fact that there are both internal and external tests specified in the same target, I think what I'm asking in bazel-contrib/bazel-gazelle#1078 then is for gazelle to construct the right dependencies by looking at the import path, rather than just the label. Curious to see what they think of this upstream.
|
left additional commentary at bazel-contrib/bazel-gazelle#1078 |
|
|
||
| gomock( | ||
| name = "mock_rangefeed", | ||
| out = "mocks_generated.go", |
There was a problem hiding this comment.
I think we could end up needing multiple mock files -- in particular when using source mode, since I think mockgen then generates one output file per input file. With Mockery, it places mocks in a mocks/ subdirectory with package mocks. Perhaps a scheme like that could help with dependency handling as well?
There was a problem hiding this comment.
The scheme here is generalizable to multiple mock files, we'd create a gomock target for each one and make sure they're listed as deps in the package's :with-mocks target. And yea, a separate mocks directory would help, and (test) packages that want to import mocks for another package, could import it directly. I think I'll avoid doing that for now and stick to the standard way of placing golang/mocks. We can always re-visit if this proves to be too cumbersome/decide to go with mockery.
There was a problem hiding this comment.
Yeah, I just find it a bit unfortunate that whenever someone wants to add a mock to a new package they'll not only have to muck around with the package build dependencies (which is fine enough), but also have to change the package target name, update dozens of unrelated build files, and fiddle with resolve overrides and such in the root build manifest.
It'd be nice if this was a bit less involved, and I'm thinking with separate mocks packages we might be able to get away with adding a new mock target that Gazelle could automatically depend on for any tests that need the mocks. I'm not entirely sure we can avoid circular dependencies in that case though, with internal tests that depend on the mocks.
There was a problem hiding this comment.
The two gazelle:resolve (in the top-level BUILD file + package's BUILD file for this annoying noop target) + make bazel-generate should do all of it for us. I'm also hoping neither will be necessary if the upstream issues are resolved/have easier work-arounds. Do you feel it's still too cumbersome? Or could be documented better?
And yes, even with a separate mocks package, we'd need some bending over to work with internal tests. Curious to see if you have other ideas we could try.
There was a problem hiding this comment.
It's workable, and definitely an improvement over what we're currently doing, but a bit hacky. Don't have any better ideas off the bat though, and it's quite possible that this is as good as it's going to get right now. Might be worth looking around for some prior art.
irfansharif
force-pushed
the
210705.bazel-gomocks
branch
from
cd06eba to
15ee3c1
Compare
irfansharif
force-pushed
the
210705.bazel-gomocks
branch
from
15ee3c1 to
25082a5
Compare
|
Given the BUILD.bazel surface area of this PR, it keeps picking up merge conflicts against master. With a cursory browse around I couldn't find much in the way of alternatives, and I think it's a decent stopping point for now wrt mockgen + bazel (esp after filing the linked issues upstream). If y'all agree, can I get an LGTM? |
|
(bump, I'm also happy to abandon this PR if we don't think it's sufficient.) |
Fixes cockroachdb#67010. Also clean up some rotted gomock generation in sqlproxyccl (mockgen usage was removed in cockroachdb#66369). Release note: None
irfansharif
force-pushed
the
210705.bazel-gomocks
branch
from
25082a5 to
37d82fb
Compare
|
TFTR! bors r+ |
|
Build succeeded: |
Fixes #67010. Also clean up some rotted gomock generation in sqlproxyccl
(mockgen usage was removed in #66369).
Release note: None