From cd500e74655fcbd0932d17d8de2a5364c27b33c1 Mon Sep 17 00:00:00 2001 From: Charles Mita Date: Fri, 10 Sep 2021 00:13:30 -0700 Subject: [PATCH] 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 --- .../google/testing/coverage/BranchExp.java | 6 +- .../testing/coverage/MethodProbesMapper.java | 2 +- .../shell/bazel/bazel_coverage_java_test.sh | 207 ++++++++++++++++++ 3 files changed, 212 insertions(+), 3 deletions(-) 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 2ee04e6f58f7ce..4d7328bf549884 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 @@ public void update(int idx, CovExp exp) { } /** Make a union of the branches of two BranchExp. */ - public void merge(BranchExp other) { - branches.addAll(other.branches); + public BranchExp merge(BranchExp other) { + List 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 530a6189e41578..69fcc72b055e1b 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 @@ public void visitEnd() { 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 cb603acaf4976c..db4dddbf1a214a 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 @@ end_of_record" 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 < 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 < 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 < 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"