bazelbuild / bazel-gazelle

Gazelle is a Bazel build file generator for Bazel projects. It natively supports Go and protobuf, and it may be extended to support new languages and custom rule sets.
Apache License 2.0
1.18k stars 373 forks source link

Exclude `go_test` deps with the same import path as the test target #1078

Open irfansharif opened 3 years ago

irfansharif commented 3 years ago

I'm trying to figure out a convenient pattern to generate mocks through bazel. The full repro for this issue can be found here (look towards the TODO for the actual "bug").

We have the following test target:

go_test(
    name = "rangefeed_test",
    srcs = [
        # ...
    ],
    embed = [":with-mocks"],  # keep
    # ...
)

With the supporting definitions:

# (present elsewhere) gazelle:resolve go github.com/cockroachdb/cockroach/pkg/kv/kvclient/rangefeed //pkg/kv/kvclient/rangefeed:with-mocks

go_library(
    name = "rangefeed",
    srcs = [
        # ...
    ],
    importpath = "github.com/cockroachdb/cockroach/pkg/kv/kvclient/rangefeed",
    # ...
)

go_library(
    name = "with-mocks",
    embed = [":rangefeed"],
    # ...
)

One of the test files in this package is an external one (i.e. using package rangefeed_test), so imports rangefeed. From the definitions above, we can see that the go_test target's importpath, by virtue of the embedded :with-mocks target, should be github.com/cockroachdb/cockroach/pkg/kv/kvclient/rangefeed. So if there are external tests that import rangefeed, I don't expect them to be included in rangefeed_test target's deps. This is existing behavior for external tests in the same package, deps is pruned out of the attempted self-import. Yet with the repro above, running gazelle, we see that deps ends up picking up the rangefeed dependency.

go_test(
    name = "rangefeed_test",
    srcs = [
        # ...
    ],
    embed = [":with-mocks"],  # keep
    deps = [
        # ...
        "//pkg/kv/kvclient/rangefeed:with-mocks",  
    ]
    # ...
)

What version of gazelle are you using?

d038863ba2e096792c6bb6afca31f6514f1aeecd

What version of rules_go are you using?

91c4e2b0f233c7031b191b93587f562ffe21f86f

What version of Bazel are you using?

Bazelisk version: development
Build label: 4.1.0
Build target: bazel-out/darwin-opt/bin/src/main/java/com/google/devtools/build/lib/bazel/BazelServer_deploy.jar
Build time: Fri May 21 11:16:55 2021 (1621595815)
Build timestamp: 1621595815
Build timestamp as int: 1621595815

Does this issue reproduce with the latest releases of all the above?

Haven't tried it, but happy to if asked.

What operating system and processor architecture are you using?

MacOS, x86 64-bit.

What did you do?

See above.

What did you expect to see?

Didn't expect to have have gazelle add the "rangefeed" dependency in the go_test target. I expected it to understand that the embedded library has the same importpath as the dependency, so can be safely ignored. To work around it, I'm currently thinking of the following:

# gazelle:resolve go github.com/cockroachdb/cockroach/pkg/kv/kvclient/rangefeed //pkg/kv/kvclient/rangefeed:noop
go_library(
    name = "noop",
    importpath = "noop",
    visibility = ["//visibility:private"],
)

go_test(
    name = "rangefeed_test",
    srcs = [
        # ...
    ],
    embed = [":with-mocks"],  # keep
    deps = [
        # ...
        "//pkg/kv/kvclient/rangefeed:noop",
        # ...
    ]
    # ...
)

What did you see instead?

See above.

rickystewart commented 3 years ago

I think the root of the problem may be that Gazelle can't distinguish between internal and external tests and handle them as rules_go expects them to. I think what rules_go wants is for internal and external tests to be split up into different go_test targets that consume the library under test in different ways (embed vs. deps respectively). Since Gazelle just dumps all the test files in one package into a single go_test target, it becomes very difficult/impossible to construct that go_test "correctly". The :noop trick that @irfansharif mentioned is a workaround that appears to basically work but I don't have confidence that trick would be resilient to e.g. a new version of rules_go or some other big change to the underlying infrastructure.

If Gazelle could emit :library_internal_test and :library_external_test targets then that would make the workaround unnecessary in the first place.