Skip to content

Commit

Permalink
Fix bazel coverage false negative (bazelbuild#14836)
Browse files Browse the repository at this point in the history
Previously this short circuit meant the tests weren't actually run, and
they would always exit 0, in the case the test rule didn't set the lcov
related attributes.

More context: bazelbuild#13978

Closes bazelbuild#14807.

RELNOTES: Fixed an issue where Bazel could erroneously report a test passes in coverage mode without actually running the test.
PiperOrigin-RevId: 428756211
(cherry picked from commit 16de035)

Co-authored-by: Keith Smiley <keithbsmiley@gmail.com>
  • Loading branch information
brentleyjones and keith authored Feb 16, 2022
1 parent 59384dd commit 344e8f8
Show file tree
Hide file tree
Showing 2 changed files with 31 additions and 1 deletion.
27 changes: 27 additions & 0 deletions src/test/shell/bazel/bazel_coverage_starlark_test.sh
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,33 @@ EOF
|| fail "Coverage run failed but should have succeeded."
}

function test_starlark_rule_without_lcov_merger_failing_test() {
cat <<EOF > rules.bzl
def _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 = _impl,
test = True,
)
EOF

cat <<EOF > BUILD
load(":rules.bzl", "custom_test")
custom_test(name = "foo_test")
EOF
if bazel coverage //:foo_test > $TEST_log; then
fail "Coverage run succeeded but should have failed."
fi
}


function test_starlark_rule_with_custom_lcov_merger() {

cat <<EOF > lcov_merger.sh
Expand Down
5 changes: 4 additions & 1 deletion tools/test/collect_coverage.sh
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,10 @@ if [[ -z "$LCOV_MERGER" ]]; then
# it.
# TODO(cmita): Improve this situation so this early-exit isn't required.
touch $COVERAGE_OUTPUT_FILE
exit 0
# Execute the test.
"$@"
TEST_STATUS=$?
exit "$TEST_STATUS"
fi

function resolve_links() {
Expand Down

0 comments on commit 344e8f8

Please sign in to comment.