Skip to content

Commit

Permalink
Fix coverage runfiles directory issue (#15047)
Browse files Browse the repository at this point in the history
As discovered in #15030 there
was another bug with the short circuiting coverage logic where tests
were not run from the correct directory. In that PR this issue is fixed
directly, with this change we instead make missing LCOV_MERGER be
deferred until after all test setup and run, which I think is more
future proof to other changes here, and also allows users to know their
tests are being run with coverage, and output the correct files, even in
the case they don't setup the coverage merger infrastructure.

Closes #15031.

PiperOrigin-RevId: 434715347

Co-authored-by: Keith Smiley <keithbsmiley@gmail.com>
  • Loading branch information
fmeum and keith authored Mar 15, 2022
1 parent 91a580a commit 1345938
Show file tree
Hide file tree
Showing 2 changed files with 34 additions and 26 deletions.
18 changes: 16 additions & 2 deletions src/test/shell/bazel/bazel_coverage_starlark_test.sh
Original file line number Diff line number Diff line change
Expand Up @@ -40,8 +40,22 @@ function test_starlark_rule_without_lcov_merger() {
cat <<EOF > rules.bzl
def _impl(ctx):
output = ctx.actions.declare_file(ctx.attr.name)
ctx.actions.write(output, "", is_executable = True)
return [DefaultInfo(executable=output)]
ctx.actions.write(output, """\
#!/bin/bash
if [[ ! -r extra ]]; then
echo "extra file not found" >&2
exit 1
fi
if [[ -z \$COVERAGE ]]; then
echo "COVERAGE environment variable not set, coverage not run."
exit 1
fi
""", is_executable = True)
extra_file = ctx.actions.declare_file("extra")
ctx.actions.write(extra_file, "extra")
return [DefaultInfo(executable=output, runfiles=ctx.runfiles(files=[extra_file]))]
custom_test = rule(
implementation = _impl,
Expand Down
42 changes: 18 additions & 24 deletions tools/test/collect_coverage.sh
Original file line number Diff line number Diff line change
Expand Up @@ -30,21 +30,6 @@ if [[ -n "$VERBOSE_COVERAGE" ]]; then
set -x
fi

if [[ -z "$LCOV_MERGER" ]]; then
# this can happen if a rule returns an InstrumentedFilesInfo (which all do
# following 5b216b2) but does not define an _lcov_merger attribute.
# Unfortunately, we cannot simply stop this script being called in this case
# due to conflicts with how things work within Google.
# The file creation is required because TestActionBuilder has already declared
# it.
# TODO(cmita): Improve this situation so this early-exit isn't required.
touch $COVERAGE_OUTPUT_FILE
# Execute the test.
"$@"
TEST_STATUS=$?
exit "$TEST_STATUS"
fi

function resolve_links() {
local name="$1"

Expand Down Expand Up @@ -101,15 +86,6 @@ export JAVA_COVERAGE_FILE=$COVERAGE_DIR/jvcov.dat
export COVERAGE=1
export BULK_COVERAGE_RUN=1


for name in "$LCOV_MERGER"; do
if [[ ! -e $name ]]; then
echo --
echo Coverage runner: cannot locate file $name
exit 1
fi
done

# Setting up the environment for executing the C++ tests.
if [[ -z "$GCOV_PREFIX_STRIP" ]]; then
# TODO: GCOV_PREFIX_STRIP=3 is incorrect on MacOS in the default setup
Expand Down Expand Up @@ -202,6 +178,24 @@ if [[ "$CC_CODE_COVERAGE_SCRIPT" ]]; then
eval "${CC_CODE_COVERAGE_SCRIPT}"
fi

if [[ -z "$LCOV_MERGER" ]]; then
# this can happen if a rule returns an InstrumentedFilesInfo (which all do
# following 5b216b2) but does not define an _lcov_merger attribute.
# Unfortunately, we cannot simply stop this script being called in this case
# due to conflicts with how things work within Google.
# The file creation is required because TestActionBuilder has already declared
# it.
exit 0
fi

for name in "$LCOV_MERGER"; do
if [[ ! -e $name ]]; then
echo --
echo Coverage runner: cannot locate file $name
exit 1
fi
done

# Export the command line that invokes LcovMerger with the flags:
# --coverage_dir The absolute path of the directory where the
# intermediate coverage reports are located.
Expand Down

0 comments on commit 1345938

Please sign in to comment.