Do not merge BranchExp objects in place BranchExp objects form a tree structure that is used in report branch evaluations for a source line. Merging of these objects in place when associating them to line numbers will alter the structures rooted at other line numbers, potentially resulting in false positives in the branch coverage report. Fixes #13962 Closes #13967. PiperOrigin-RevId: 395867534
diff --git a/src/java_tools/junitrunner/java/com/google/testing/coverage/BranchExp.java b/src/java_tools/junitrunner/java/com/google/testing/coverage/BranchExp.java index 2ee04e6..4d7328b 100644 --- a/src/java_tools/junitrunner/java/com/google/testing/coverage/BranchExp.java +++ b/src/java_tools/junitrunner/java/com/google/testing/coverage/BranchExp.java
@@ -58,8 +58,10 @@ } /** Make a union of the branches of two BranchExp. */ - public void merge(BranchExp other) { - branches.addAll(other.branches); + public BranchExp merge(BranchExp other) { + List<CovExp> mergedBranches = new ArrayList<>(branches); + mergedBranches.addAll(other.branches); + return new BranchExp(mergedBranches); } @Override
diff --git a/src/java_tools/junitrunner/java/com/google/testing/coverage/MethodProbesMapper.java b/src/java_tools/junitrunner/java/com/google/testing/coverage/MethodProbesMapper.java index 530a618..69fcc72 100644 --- a/src/java_tools/junitrunner/java/com/google/testing/coverage/MethodProbesMapper.java +++ b/src/java_tools/junitrunner/java/com/google/testing/coverage/MethodProbesMapper.java
@@ -424,7 +424,7 @@ if (lineExp == null) { lineToBranchExp.put(insn.getLine(), exp); } else { - lineExp.merge(exp); + lineToBranchExp.put(insn.getLine(), lineExp.merge(exp)); } } else { // If we reach here, the internal data of the mapping is inconsistent, either
diff --git a/src/test/shell/bazel/bazel_coverage_java_test.sh b/src/test/shell/bazel/bazel_coverage_java_test.sh index cb603ac..db4dddb 100755 --- a/src/test/shell/bazel/bazel_coverage_java_test.sh +++ b/src/test/shell/bazel/bazel_coverage_java_test.sh
@@ -784,4 +784,211 @@ assert_coverage_result "$expected_result" "$coverage_file_path" } + +function test_finally_block_branch_coverage() { + # Verify branches in finally blocks are handled correctly. + # The java compiler duplicates finally blocks for the various code paths that + # may enter them (e.g. via an exception handler or when no exception is + # thrown). + cat <<EOF > BUILD +java_test( + name = "test", + srcs = glob(["src/test/**/*.java"]), + test_class = "com.example.TestFinally", + deps = [":finally-lib"], +) + +java_library( + name = "finally-lib", + srcs = glob(["src/main/**/*.java"]), +) +EOF + + mkdir -p src/main/com/example + cat <<EOF > src/main/com/example/Finally.java +package com.example; + +public class Finally { + + private static int secret = 0; + + public static int runFinally(int x) { + try { + if (x == 1 || x == -1) { + throw new RuntimeException(); + } + if (x % 2 == 0) { + return x * 2; + } else { + return x * 2 - 1; + } + } finally { + if (x >= 0) { + secret++; + } else { + secret--; + } + } + } +} +EOF + + mkdir -p src/test/com/example + cat <<EOF > src/test/com/example/TestFinally.java +package com.example; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertThrows; +import org.junit.Test; + +public class TestFinally { + + @Test + public void testEven() { + assertEquals(4, Finally.runFinally(2)); + } + @Test + public void testOdd() { + assertEquals(5, Finally.runFinally(3)); + } + @Test + public void testNegativeEven() { + assertEquals(-4, Finally.runFinally(-2)); + } + @Test + public void testNegativeOdd() { + assertEquals(-7, Finally.runFinally(-3)); + } + @Test + public void testException() { + assertThrows(RuntimeException.class, () -> Finally.runFinally(1)); + } + @Test + public void testNegativeException() { + assertThrows(RuntimeException.class, () -> Finally.runFinally(-1)); + } + +} +EOF + + # For the sake of brevity, only check the output for the branches + # corresponding to the finally block in the method under test rather than the + # entire coverage output. + bazel coverage --test_output=all //:test \ + --coverage_report_generator=@bazel_tools//tools/test:coverage_report_generator \ + --combined_report=lcov &>$TEST_log \ + --test_filter=TestFinally.testNegativeException \ + || echo "Coverage for //:test failed" + + #--test_filter=".*(testNegativeException)" \ + local coverage_file_path="$( get_coverage_file_path_from_test_log )" + local expected_result="BRDA:9,0,0,0 +BRDA:9,0,1,1 +BRDA:9,0,2,1 +BRDA:9,0,3,0 +BRDA:12,0,0,- +BRDA:12,0,1,- +BRDA:18,0,0,0 +BRDA:18,0,1,0 +BRDA:18,0,2,0 +BRDA:18,0,3,0 +BRDA:18,0,4,0 +BRDA:18,0,5,1 +BRF:12 +BRH:3" + assert_coverage_result "$expected_result" "$coverage_file_path" + + bazel coverage --test_output=all //:test \ + --coverage_report_generator=@bazel_tools//tools/test:coverage_report_generator \ + --combined_report=lcov &>$TEST_log \ + --test_filter=".*(testOdd|testNegativeOdd)" \ + || echo "Coverage for //:test failed" + + local coverage_file_path="$( get_coverage_file_path_from_test_log )" + local expected_result="BRDA:9,0,0,0 +BRDA:9,0,1,1 +BRDA:9,0,2,0 +BRDA:9,0,3,1 +BRDA:12,0,0,1 +BRDA:12,0,1,0 +BRDA:18,0,0,0 +BRDA:18,0,1,0 +BRDA:18,0,2,1 +BRDA:18,0,3,1 +BRDA:18,0,4,0 +BRDA:18,0,5,0 +BRF:12 +BRH:5" + assert_coverage_result "$expected_result" "$coverage_file_path" + + bazel coverage --test_output=all //:test \ + --coverage_report_generator=@bazel_tools//tools/test:coverage_report_generator \ + --combined_report=lcov &>$TEST_log \ + --test_filter=".*(testEven|testNegativeOdd)" \ + || echo "Coverage for //:test failed" + + local coverage_file_path="$( get_coverage_file_path_from_test_log )" + local expected_result="BRDA:9,0,0,0 +BRDA:9,0,1,1 +BRDA:9,0,2,0 +BRDA:9,0,3,1 +BRDA:12,0,0,1 +BRDA:12,0,1,1 +BRDA:18,0,0,1 +BRDA:18,0,1,0 +BRDA:18,0,2,0 +BRDA:18,0,3,1 +BRDA:18,0,4,0 +BRDA:18,0,5,0 +BRF:12 +BRH:6" + assert_coverage_result "$expected_result" "$coverage_file_path" + + bazel coverage --test_output=all //:test \ + --coverage_report_generator=@bazel_tools//tools/test:coverage_report_generator \ + --combined_report=lcov &>$TEST_log \ + --test_filter=".*(testNegativeEven|testException)" \ + || echo "Coverage for //:test failed" + + local coverage_file_path="$( get_coverage_file_path_from_test_log )" + local expected_result="BRDA:9,0,0,1 +BRDA:9,0,1,1 +BRDA:9,0,2,0 +BRDA:9,0,3,1 +BRDA:12,0,0,0 +BRDA:12,0,1,1 +BRDA:18,0,0,0 +BRDA:18,0,1,1 +BRDA:18,0,2,0 +BRDA:18,0,3,0 +BRDA:18,0,4,1 +BRDA:18,0,5,0 +BRF:12 +BRH:6" + assert_coverage_result "$expected_result" "$coverage_file_path" + + bazel coverage --test_output=all //:test \ + --coverage_report_generator=@bazel_tools//tools/test:coverage_report_generator \ + --combined_report=lcov &>$TEST_log \ + --test_filter=".*(testNegativeEven|testNegativeOdd)" \ + || echo "Coverage for //:test failed" + + local coverage_file_path="$( get_coverage_file_path_from_test_log )" + local expected_result="BRDA:9,0,0,0 +BRDA:9,0,1,1 +BRDA:9,0,2,0 +BRDA:9,0,3,1 +BRDA:12,0,0,1 +BRDA:12,0,1,1 +BRDA:18,0,0,0 +BRDA:18,0,1,1 +BRDA:18,0,2,0 +BRDA:18,0,3,1 +BRDA:18,0,4,0 +BRDA:18,0,5,0 +BRF:12 +BRH:6" + assert_coverage_result "$expected_result" "$coverage_file_path" +} + run_suite "test tests"