Custom test rule without _lcov_merger attribute always fails under bazel coverage

[fmeum]

Description of the problem / feature request:

When a custom Starlark test rule without explicit coverage support in the form of the _lcov_merger attribute is invoked via bazel coverage with Bazel after 5b216b2, it fails with

FAIL: //:custom_test (see /home/fmeum/.cache/bazel/_bazel_fhenneke/767fd922858d516eb34a4dddb64ca1a0/execroot/__main__/bazel-out/k8-fastbuild/testlogs/custom_test/test.log)
ERROR: /home/fmeum/git/bazel-custom-test-coverage/BUILD:3:12: output 'custom_test/coverage.dat' was not created
ERROR: /home/fmeum/git/bazel-custom-test-coverage/BUILD:3:12: Testing //:custom_test failed: not all outputs were created or valid
Target //:custom_test up-to-date:
  bazel-bin/custom_test
INFO: Elapsed time: 5.635s, Critical Path: 0.17s
INFO: 7 processes: 1 disk cache hit, 5 internal, 1 linux-sandbox.
FAILED: Build did NOT complete successfully
//:custom_test                                                           FAILED in 0.0s
  /home/fmeum/.cache/bazel/_bazel_fhenneke/767fd922858d516eb34a4dddb64ca1a0/execroot/__main__/bazel-out/k8-fastbuild/testlogs/custom_test/test.log

The log says:

exec ${PAGER:-/usr/bin/less} "$0" || exit 1
Executing tests from //:custom_test
-----------------------------------------------------------------------------
--
Coverage runner: cannot locate file

With Bazel 4.2.1, the build succeeds:

INFO: Build completed successfully, 5 total actions
//:custom_test                                                           PASSED in 0.0s

Executed 1 out of 1 test: 1 test passes.
There were tests whose specified size is too big. Use the --test_verbose_timeout_warnings command
INFO: Build completed successfully, 5 total actions

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

#!/usr/bin/env sh
touch WORKSPACE

cat << EOF > BUILD
load("//:rules.bzl", "custom_test")

custom_test(
    name = "custom_test",
)
EOF

cat << EOF > rules.bzl
def _custom_test_impl(ctx):
    executable = ctx.actions.declare_file(ctx.attr.name)
    ctx.actions.write(executable, "", is_executable = True)
    return [
        DefaultInfo(
            executable = executable,
        )
    ]

custom_test = rule(
    implementation = _custom_test_impl,
    test = True,
)
EOF

bazel coverage //:custom_test

What operating system are you running Bazel on?

Linux

What's the output of bazel info release?

release 5.0.0-pre.20210831.2

I bisected the origin of this issue back to 5b216b2.

Have you found anything relevant by searching the web?

With 5b216b27435aeb9eb9c3bd3c552d6498e1050cc7, the custom_test rule in the reproducer now provides an InstrumentedFilesInfo, which causes TestActionBuilder to collect coverage. However, since the rule does not have the :lcov_merger attribute set, the LCOV_MERGER env variable isn't set when collect_coverage.sh is invoked, which causes the error message reproduced above.

[keith]

I'm kinda surprised this ever worked before, I think the real core issue to fix is #10642

[fmeum]

@keith Could you take a look at #13983 and verify whether it also fixes your issue?

[keith]

With that patch it does remove the need to pass --test_env=LCOV_MERGER=/usr/bin/true to coverage invocations

[brentleyjones]

A friendly 2 month ping on this.

[meteorcloudy]

Can anyone send a PR to cherry-pick aa52f2d into release-5.0.0rc2?

[brentleyjones]

#14359

[mattem]

@meteorcloudy Hasn't this change in #14359 essentially made it so that any Starlark test rule not defining _lcov_merger or users setting --test_env=LCOV_MERGER=/usr/bin/true fail open? That exit 0 is causing the tests to pass, incorrectly.

For example, using diff_test from sky_lib now fails open. In the original repro in this, under bazel 5, adding exit 1 into generated test script does not cause the test to fail, as it's never called.

[meteorcloudy]

For example, using diff_test from sky_lib now fails open. In the original repro in this, under bazel 5, adding exit 1 into generated test script does not cause the test to fail, as it's never called.

@fmeum @c-mita Can you respond this? I'm not familiar with this feature.

[fmeum]

@meteorcloudy I didn't look into it much after #13983 was closed, but just at a glance, @mattem seems to have caught a real regression: I also think that tests executed with bazel coverage will generally pass even if they shouldn't. But @c-mita would know the details here.

Should probably be marked as a release blocker for 5.0.1.

[mattem]

Yeah, the diff_test was just an example of a OSS rule that doesn't have the _lcov_merger attr set. Adding the exit 1 to the generated test script above will repro the issue and the test will pass incorrectly under bazel coverage.

#!/usr/bin/env sh
touch WORKSPACE

cat << EOF > BUILD
load("//:rules.bzl", "custom_test")

custom_test(
    name = "custom_test",
)
EOF

cat << EOF > rules.bzl
def _custom_test_impl(ctx):
    executable = ctx.actions.declare_file(ctx.attr.name)
    ctx.actions.write(executable, "exit 1", is_executable = True)
    return [
        DefaultInfo(
            executable = executable,
        )
    ]

custom_test = rule(
    implementation = _custom_test_impl,
    test = True,
)
EOF

bazel coverage //:custom_test

I can open a new issue if that's easier too.

[fmeum]

However, the alternative is that all these tests will now fail until the _lcov_merger attribute is added to the rule definition, which would bring back the original regression. I think we need a fundamentally better fix here.

[mattem]

True, but in the current state, bazel 5.x is passing tests when they should be failing for those running coverage over their entire build. Is a reasonable workaround here to set --test_env=LCOV_MERGER=...? If so then there's a workaround, otherwise it's a little scary to have tests fail open.

[alexeagle]

@fmeum in your original post you point out that 5b216b2 is what actually broke this. Maybe we should try to revert that flag flip in a 5.0.x release?

[fmeum]

True, but in the current state, bazel 5.x is passing tests when they should be failing for those running coverage over their entire build. Is a reasonable workaround here to set --test_env=LCOV_MERGER=...? If so then there's a workaround, otherwise it's a little scary to have tests fail open.

Yes, both fail open and fail close are bad here, but I agree that fail open is even worse. I'm just saying that there is no obvious "fix forward" approach.

@fmeum in your original post you point out that 5b216b2 is what actually broke this. Maybe we should try to revert that flag flip in a 5.0.x release?

It would fix this issue, but would technically cause a regression from 5.0.0: bazel coverage would suddenly return coverage information in much fewer cases because the flip has been disabled. I don't know whether this is acceptable.

The only way I am aware of that preserves backwards compatible with both 4.2.2 (fixing the regression) and 5.0.0 (not introducing a new regression) is to provide some default _lcov_merger, either via the approach originally proposed in #13983 or the equivalent of --test_env=LCOV_MERGER=... as suggested by @mattem. But that may cause conflicts with Google's internal setup as per #13983 (comment).

[mattem]

The --test_env=LCOV_MERGER=... works internally for us, and all tests are now correctly green. It seems like this issue should be more broadly communicated though, as for some they could have any number of failing open tests and not realize.

[keith]

Here's a fix #14807

[c-mita]

Sorry for the delay in responding.

#14807 looks to me to be an ok patch for this scenario, allowing us to proceed without regressing, but I'm not satisfied with how things are.
Bazel needs to be more intelligent in handling the case of rules not providing an _lcov_merger attribute, especially since this isn't exactly a well documented feature.

[keith]

Related #10642

[fmeum]

I think that the fix is incomplete: Without LCOV_MERGER, tests will have the exec root set as their working directory rather than the runfiles root, see #15030.