Skip to content

Commit

Permalink
Support the Jacoco filter merge operation in Java branch coverage.
Browse files Browse the repository at this point in the history
Instructions that are indicated to be merged must have their corresponding
coverage expressions (`CovExp`) combined in a pairwise fashion.

e.g. A pair of branch expressions that should be merged:
`BranchExp(a, b)`, `BranchExp(c, d)`

Becomes:

`BranchExp(a | c, b | d)`

This is distinct from the previous merge operation on BranchExp which
simply joined the branches of the two objects (this has been renamed to
concatenate).

Part of #12696.

Closes #13972.

PiperOrigin-RevId: 396573429
  • Loading branch information
c-mita authored and copybara-github committed Sep 14, 2021
1 parent bd6a7bf commit 908a765
Show file tree
Hide file tree
Showing 3 changed files with 98 additions and 48 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
package com.google.testing.coverage;

import java.util.ArrayList;
import java.util.Arrays;
import java.util.List;

/** A branch coverage that must be evaluated as a combination of probes. */
Expand Down Expand Up @@ -57,11 +58,33 @@ public void update(int idx, CovExp exp) {
branches.set(idx, exp);
}

/** Make a union of the branches of two BranchExp. */
public BranchExp merge(BranchExp other) {
List<CovExp> mergedBranches = new ArrayList<>(branches);
mergedBranches.addAll(other.branches);
return new BranchExp(mergedBranches);
/** Make a new BranchExp representing the concatenation of branches in inputs. */
public static BranchExp concatenate(BranchExp first, BranchExp second) {
List<CovExp> branches = new ArrayList<>(first.branches);
branches.addAll(second.branches);
return new BranchExp(branches);
}

/** Make a new BranchExp representing the pairwise union of branches in inputs */
public static BranchExp zip(BranchExp left, BranchExp right) {
List<CovExp> zippedBranches = new ArrayList<>();
int leftSize = left.branches.size();
int rightSize = right.branches.size();
int i;
for (i = 0; i < leftSize && i < rightSize; i++) {
List<CovExp> branches = Arrays.asList(left.branches.get(i), right.branches.get(i));
zippedBranches.add(new BranchExp(branches));
}
List<CovExp> remainder = leftSize < rightSize ? right.branches : left.branches;
for (; i < remainder.size(); i++) {
zippedBranches.add(new BranchExp(remainder.get(i)));
}
return new BranchExp(zippedBranches);
}

/** Wraps a CovExp in a BranchExp if it isn't one already. */
public static BranchExp ensureIsBranchExp(CovExp exp) {
return exp instanceof BranchExp ? (BranchExp) exp : new BranchExp(exp);
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ public class MethodProbesMapper extends MethodProbesVisitor implements IFilterOu
private IFilter filter;
private IFilterContext filterContext;
private HashSet<AbstractInsnNode> ignored = new HashSet<>();
private Map<AbstractInsnNode, AbstractInsnNode> unioned = new HashMap<>();

// Result
private Map<Integer, BranchExp> lineToBranchExp = new TreeMap();
Expand Down Expand Up @@ -349,13 +350,6 @@ private boolean updateBranchPredecessor(Instruction predecessor, Instruction ins
/** Finishing the method */
@Override
public void visitEnd() {
HashSet<Instruction> ignoredInstructions = new HashSet<>();
for (Map.Entry<AbstractInsnNode, Instruction> entry : instructionMap.entrySet()) {
if (ignored.contains(entry.getKey())) {
ignoredInstructions.add(entry.getValue());
}
}

for (Jump jump : jumps) {
Instruction insn = labelToInsn.get(jump.target);
jump.source.addBranch(insn, jump.branch);
Expand All @@ -366,9 +360,6 @@ public void visitEnd() {
for (Map.Entry<Integer, Instruction> entry : probeToInsn.entrySet()) {
int probeId = entry.getKey();
Instruction ins = entry.getValue();
if (ignoredInstructions.contains(ins)) {
continue;
}

Instruction insn = ins;
CovExp exp = new ProbeExp(probeId);
Expand Down Expand Up @@ -411,6 +402,24 @@ public void visitEnd() {
}
}

// Handle merged instructions
for (AbstractInsnNode node : unioned.keySet()) {
AbstractInsnNode rep = findRepresentative(node);
Instruction insn = instructionMap.get(node);
Instruction repInsn = instructionMap.get(rep);
BranchExp branch = BranchExp.ensureIsBranchExp(insnToCovExp.get(insn));
BranchExp repBranch = BranchExp.ensureIsBranchExp(insnToCovExp.get(repInsn));
insnToCovExp.put(repInsn, BranchExp.zip(repBranch, branch));
ignored.add(node);
}

HashSet<Instruction> ignoredInstructions = new HashSet<>();
for (Map.Entry<AbstractInsnNode, Instruction> entry : instructionMap.entrySet()) {
if (ignored.contains(entry.getKey())) {
ignoredInstructions.add(entry.getValue());
}
}

// Merge branches in the instructions on the same line
for (Instruction insn : instructions) {
if (ignoredInstructions.contains(insn)) {
Expand All @@ -424,7 +433,7 @@ public void visitEnd() {
if (lineExp == null) {
lineToBranchExp.put(insn.getLine(), exp);
} else {
lineToBranchExp.put(insn.getLine(), lineExp.merge(exp));
lineToBranchExp.put(insn.getLine(), BranchExp.concatenate(lineExp, exp));
}
} else {
// If we reach here, the internal data of the mapping is inconsistent, either
Expand All @@ -447,14 +456,33 @@ public void ignore(AbstractInsnNode fromInclusive, AbstractInsnNode toInclusive)

@Override
public void merge(AbstractInsnNode i1, AbstractInsnNode i2) {
// TODO(cmita): Implement as part of https://github.com/bazelbuild/bazel/issues/12696
// Track nodes to be merged using a union-find algorithm.
i1 = findRepresentative(i1);
i2 = findRepresentative(i2);
if (i1 != i2) {
unioned.put(i1, i2);
}
}

@Override
public void replaceBranches(AbstractInsnNode source, Set<AbstractInsnNode> newTargets) {
// TODO(cmita): Implement as part of https://github.com/bazelbuild/bazel/issues/12696
}

private AbstractInsnNode findRepresentative(AbstractInsnNode node) {
// The "find" part of union-find. Walk the chain of nodes to find the representative node
// (at the root), flattening the tree a little as we go.
AbstractInsnNode parent;
AbstractInsnNode grandParent;
while ((parent = unioned.get(node)) != null) {
if ((grandParent = unioned.get(parent)) != null) {
unioned.put(node, grandParent);
}
node = parent;
}
return node;
}

/** Jumps between instructions and labels */
class Jump {
public final Instruction source;
Expand Down
61 changes: 30 additions & 31 deletions src/test/shell/bazel/bazel_coverage_java_test.sh
Original file line number Diff line number Diff line change
Expand Up @@ -889,12 +889,8 @@ 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
BRDA:18,0,1,1
BRF:8
BRH:3"
assert_coverage_result "$expected_result" "$coverage_file_path"

Expand All @@ -911,13 +907,9 @@ 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
BRDA:18,0,0,1
BRDA:18,0,1,1
BRF:8
BRH:5"
assert_coverage_result "$expected_result" "$coverage_file_path"

Expand All @@ -935,12 +927,8 @@ 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
BRDA:18,0,1,1
BRF:8
BRH:6"
assert_coverage_result "$expected_result" "$coverage_file_path"

Expand All @@ -957,13 +945,9 @@ 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,0,1
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
BRF:8
BRH:6"
assert_coverage_result "$expected_result" "$coverage_file_path"

Expand All @@ -982,12 +966,27 @@ 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"
BRF:8
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=".*(testOdd|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,1
BRDA:12,0,1,0
BRDA:18,0,0,1
BRDA:18,0,1,0
BRF:8
BRH:5"
assert_coverage_result "$expected_result" "$coverage_file_path"
}

Expand Down

0 comments on commit 908a765

Please sign in to comment.