Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Filter unreachable branches in Kotlin open functions with default arguments #887

Merged
merged 6 commits into from Jun 3, 2019
Merged

Filter unreachable branches in Kotlin open functions with default arguments #887

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
b57ad1f
(WIP) add validation test
Godin May 17, 2019
b83a5ea
(WIP) add assertion about sum of branch counters of all lines in a file
Godin May 17, 2019
515e7b8
(WIP) add unit test
Godin May 17, 2019
141d681
(WIP) update filter
Godin May 31, 2019
309e7a4
(WIP) update changelog
Godin May 31, 2019
2c51c86
(WIP) add assertion about sum of missed instructions of all lines in …
Godin Jun 3, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 5 additions & 0 deletions ...e.test.validation.kotlin/src/org/jacoco/core/test/validation/kotlin/KotlinInlineTest.java
View file
Open in desktop
Expand Up @@ -23,4 +23,9 @@ public KotlinInlineTest() {
super(KotlinInlineTargetKt.class);
}

@Override
public void all_missed_instructions_should_have_line_number() {
// missed instructions without line number in inline function
}

}
7 changes: 7 additions & 0 deletions ...kotlin/src/org/jacoco/core/test/validation/kotlin/targets/KotlinDefaultArgumentsTarget.kt
View file
Open in desktop
Expand Up @@ -22,6 +22,11 @@ object KotlinDefaultArgumentsTarget {
private fun branch(a: Boolean, b: String = if (a) "a" else "b") { // assertFullyCovered(0, 2)
}

open class Open {
open fun f(a: String = "a") { // assertFullyCovered()
}
}

@JvmStatic
fun main(args: Array<String>) {
f(a = "a")
Expand All @@ -31,6 +36,8 @@ object KotlinDefaultArgumentsTarget {

branch(false)
branch(true)

Open().f()
}

}
55 changes: 55 additions & 0 deletions ...e.test/src/org/jacoco/core/internal/analysis/filter/KotlinDefaultArgumentsFilterTest.java
View file
Open in desktop
Expand Up @@ -95,4 +95,59 @@ public void should_not_filter_when_not_synthetic() {
assertIgnored();
}

/**
* <pre>
* open class Open {
* open fun foo(a: Int = 42) {
* }
* }
* </pre>
*/
@Test
public void should_filter_open_functions() {
final MethodNode m = new MethodNode(InstrSupport.ASM_API_VERSION,
Opcodes.ACC_SYNTHETIC, "foo$default",
"(LOpen;IILjava/lang/Object;)V", null, null);
context.classAnnotations
.add(KotlinGeneratedFilter.KOTLIN_METADATA_DESC);
{
m.visitVarInsn(Opcodes.ALOAD, 3);
final Label label = new Label();
m.visitJumpInsn(Opcodes.IFNULL, label);
m.visitTypeInsn(Opcodes.NEW,
"java/lang/UnsupportedOperationException");
m.visitInsn(Opcodes.DUP);
m.visitLdcInsn(
"Super calls with default arguments not supported in this target, function: foo");
m.visitMethodInsn(Opcodes.INVOKESPECIAL,
"java/lang/UnsupportedOperationException", "<init>",
"(Ljava/lang/String;)V", false);
m.visitInsn(Opcodes.ATHROW);
m.visitLabel(label);
}
{
m.visitVarInsn(Opcodes.ILOAD, 2);
m.visitInsn(Opcodes.ICONST_1);
m.visitInsn(Opcodes.IAND);
final Label label = new Label();
m.visitJumpInsn(Opcodes.IFEQ, label);
// default argument
m.visitLdcInsn(Integer.valueOf(42));
m.visitVarInsn(Opcodes.ISTORE, 1);
m.visitLabel(label);

m.visitVarInsn(Opcodes.ALOAD, 0);
m.visitVarInsn(Opcodes.ILOAD, 1);
m.visitMethodInsn(Opcodes.INVOKESPECIAL, "Open", "foo", "(I)V",
false);
m.visitInsn(Opcodes.RETURN);
}

filter.filter(m, context, output);

assertIgnored(
new Range(m.instructions.getFirst(), m.instructions.get(6)),
new Range(m.instructions.get(11), m.instructions.get(11)));
}

}
22 changes: 22 additions & 0 deletions org.jacoco.core.test/src/org/jacoco/core/test/validation/ValidationTestBase.java
View file
Open in desktop
Expand Up @@ -123,6 +123,28 @@ public void last_line_in_coverage_data_should_be_less_or_equal_to_number_of_line
source.getCoverage().getLastLine() <= source.getLines().size());
}

@Test
public void all_missed_instructions_should_have_line_number() {
CounterImpl c = CounterImpl.COUNTER_0_0;
for (Line line : source.getLines()) {
c = c.increment(line.getCoverage().getInstructionCounter());
}
assertEquals(
"sum of missed instructions of all lines should be equal to missed instructions of file",
source.getCoverage().getInstructionCounter().getMissedCount(), c.getMissedCount());
}

@Test
public void all_branches_should_have_line_number() {
Copy link
Member Author

@Godin Godin May 31, 2019 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@marchof here is some notes about this new test:

Unfortunately in our validation tests all assertions are about code that has line numbers, so validation test was green without this - see first commit.

Similar generic assertion for instructions counter is not possible, because it does not hold for KotlinInlineTarget.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, I like this!

I actually tried with instructions and it fails with most Kotlin tests.

I wonder whether we should add this test to the base class and explicitly overwrite it for those tests where it does not hold true (with comments why this is).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@marchof

I actually tried with instructions and it fails with most Kotlin tests.

You're right - instruction counters won't be equal for most Kotlin targets, because first entry in LineNumberTable is past non-null checks of parameters:

  public static final void main(java.lang.String[]);
    descriptor: ([Ljava/lang/String;)V
    flags: (0x0019) ACC_PUBLIC, ACC_STATIC, ACC_FINAL
    Code:
      stack=8, locals=2, args_size=1
         0: aload_0
         1: ldc           #10                 // String args
         3: invokestatic  #16                 // Method kotlin/jvm/internal/Intrinsics.checkParameterIsNotNull:(Ljava/lang/Object;Ljava/lang/String;)V
         6: new           #18                 // class org/jacoco/core/test/validation/kotlin/targets/KotlinDataClassTarget$DataClass
         ...
      LineNumberTable:
        line 40: 6

And in fact I wasn't precise enough about description of "similar generic assertion": what holds for every target except KotlinInlineTarget - is equality of number of missed instructions with line number and without, i.e. that all missed instructions are visible in source, while covered might be invisible, which is ok since they are covered 😉

explicitly overwrite it for those tests where it does not hold true (with comments why this is)

Haven't thought about override. Nice idea! 👍 Added it.

CounterImpl c = CounterImpl.COUNTER_0_0;
for (Line line : source.getLines()) {
c = c.increment(line.getCoverage().getBranchCounter());
}
assertEquals(
"sum of branch counters of all lines should be equal to branch counter of file",
source.getCoverage().getBranchCounter(), c);
}

/*
* Predefined assertion methods:
*/
Expand Down
22 changes: 22 additions & 0 deletions ...acoco.core/src/org/jacoco/core/internal/analysis/filter/KotlinDefaultArgumentsFilter.java
View file
Open in desktop
Expand Up @@ -18,6 +18,7 @@
import org.objectweb.asm.Type;
import org.objectweb.asm.tree.AbstractInsnNode;
import org.objectweb.asm.tree.JumpInsnNode;
import org.objectweb.asm.tree.LdcInsnNode;
import org.objectweb.asm.tree.MethodNode;
import org.objectweb.asm.tree.VarInsnNode;

Expand Down Expand Up @@ -67,6 +68,27 @@ public void match(final MethodNode methodNode,
final IFilterOutput output) {
cursor = methodNode.instructions.getFirst();

nextIs(Opcodes.IFNULL);
nextIsType(Opcodes.NEW, "java/lang/UnsupportedOperationException");
nextIs(Opcodes.DUP);
nextIs(Opcodes.LDC);
if (cursor == null
|| !(((LdcInsnNode) cursor).cst instanceof String)
|| !(((String) ((LdcInsnNode) cursor).cst).startsWith(
"Super calls with default arguments not supported in this target"))) {
cursor = null;
}
nextIsInvoke(Opcodes.INVOKESPECIAL,
"java/lang/UnsupportedOperationException", "<init>",
"(Ljava/lang/String;)V");
nextIs(Opcodes.ATHROW);
if (cursor != null) {
output.ignore(methodNode.instructions.getFirst(), cursor);
next();
} else {
cursor = methodNode.instructions.getFirst();
}

final Set<AbstractInsnNode> ignore = new HashSet<AbstractInsnNode>();
final int maskVar = Type.getMethodType(methodNode.desc)
.getArgumentTypes().length - 2;
Expand Down
7 changes: 7 additions & 0 deletions org.jacoco.doc/docroot/doc/changes.html
View file
Open in desktop
Expand Up @@ -20,6 +20,13 @@ <h1>Change History</h1>

<h2>Snapshot Build @qualified.bundle.version@ (@build.date@)</h2>

<h3>New Features</h3>
<ul>
<li>Branches added by the Kotlin compiler for <code>open</code> functions with
default arguments are filtered out during generation of report
(GitHub <a href="https://github.com/jacoco/jacoco/issues/887">#887</a>).</li>
</ul>

<h2>Release 0.8.4 (2019/05/08)</h2>

<h3>New Features</h3>
Expand Down