test_suite visibility not checked by test or aquery commands

[dws]

Description of the problem / feature request:

bazel cquery of a test_suite target fails when the constituent tests have not explicitly declared their visibility to that target.
This is unexpected because bazel test of that same test_suite target will run the tests without any complaints about visibility.

Bugs: what's the simplest, easiest way to reproduce this bug? Please provide a minimal example if possible.

I created a simple bazel project with the following contents:

$ cat WORKSPACE
$ cat d1/BUILD
sh_test(
    name = "d1_test",
    srcs = ["d1_test.sh"],
)
$ cat d1/d1_test.sh
true
$ cat d2/BUILD
sh_test(
    name = "d2_test",
    srcs = ["d2_test.sh"],
)
$ cat d2/d2_test.sh
true
$ cat d3/BUILD
test_suite(
    name = "d3",
    tests = [
        "//d1:d1_test",
        "//d2:d2_test",
    ],
)

Here, bazel test //d3:d3 works just fine, but bazel cquery //d3:d3 bails out:

$ bazel cquery //d3:d3
ERROR: /home/dsanderson/bug2/d3/BUILD:1:11: in test_suite rule //d3:d3: target '//d1:d1_test' is not visible from target '//d3:d3'. Check the visibility declaration of the former target if you think the dependency is legitimate
ERROR: /home/dsanderson/bug2/d3/BUILD:1:11: in test_suite rule //d3:d3: target '//d2:d2_test' is not visible from target '//d3:d3'. Check the visibility declaration of the former target if you think the dependency is legitimate
ERROR: Analysis of target '//d3:d3' failed; build aborted: Analysis of target '//d3:d3' failed

What operating system are you running Bazel on?

Ubuntu 20.04

What's the output of bazel info release?

release 4.2.1
release 5.0.0-pre.20210921.1

Have you found anything relevant by searching the web?

no

Any other information, logs, or outputs that you want to share?

I confirmed the unexpected behavior with both 4.2.1 and 5.0.0-pre.20210921.1.
Neither query nor aquery were similarly strict.
It is possible to work around this by declaring visibilities on the tests
participating in the test_suite. But since this was not needed for
bazel test, I did not expect to need to do this for bazel cquery.

[dws]

@juliexxia for vis

[brandjon]

I'm not surprised that cquery would fail for visibility reasons while query works -- cquery does analysis (where visibility is enforced) while query does not. Not sure why aquery wouldn't be affected.

I'm more surprised that test_suite doesn't check visibility. Perhaps there's magic in the test command to unpack test_suite contents and that magic doesn't go through the visibility check.

I don't think this is a bug so much as an inconsistency in the test command and perhaps aquery.

[ajwerner]

I don't think it's just magic in the test command; I can invoke bazel build on such a target as well.

[ajwerner]

It feels to me like changing this behavior to enforce visibility is likely to be a rather disruptive breaking change. Might I suggest we change the behavior of cquery and document our way out of this by explaining the behavior as it stands in the visibility docs?

Maybe it'd make sense to remedy the situation in a major version with a proper deprecation cycle.

[gregestren]

I'm more surprised that test_suite doesn't check visibility. Perhaps there's magic in the test command to unpack test_suite contents and that magic doesn't go through the visibility check

I think this is exactly what happens.

I don't think it's just magic in the test command; I can invoke bazel build on such a target as well.

build and test share a lot of common logic so I'd expect it to apply to both: a test is basically a variant of a build.

What about adding --nocheck_visibility to your cquery?

I agree enforcing new visibility is a huge disruptive pain (ahem: #12933). But I'd also want to avoid reducing current instances of visibility enforcement. Especially if --nocheck_visibility suffices when really needed.

I'l add a Documentation tag to reflect your docs request in the last comment.

[github-actions]

Thank you for contributing to the Bazel repository! This issue has been marked as stale since it has not had any activity in the last 1+ years. It will be closed in the next 90 days unless any other activity occurs. If you think this issue is still relevant and should stay open, please post any comment here and the issue will no longer be marked as stale.