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.

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

Patch: update instrumentation order to fix coverage #711

Merged
merged 3 commits into from
Apr 26, 2023

Conversation

kmnls
Copy link
Contributor

@kmnls kmnls commented Apr 14, 2023

Reason:
JaCoCo produces wrong coverage because the coverage instrumentation happens after the hooks are set.
This instrumentation should follow the JaCoCo algorithm but in fact they produce different results.
Both algorithms inject control points into the end of each BB where the INVOKE happens.
BBs where there are no INVOKEs are not marked.
Let's check the case: BBs originally did not have any INVOKE but hooks are injected traceCmp callback.
CoverageRecorder happily adds a control point into such a BB but JaCoCo will never know about this.
As the result, all points after the mentioned will be misplaced in JaCoCo coverage.

Sample for test:

package com.example;

import java.util.Random;

public class Junit5Example1 {

public static boolean earlyReturn = false;

public static boolean logic(String input) {
    long random = new Random(42).nextLong();
    boolean cmp = input.startsWith("magicstring" + random);
    if (earlyReturn) {
        return true;
    }
    if (cmp
            && input.length() > 35
            && input.charAt(35) == 'C') {
        mustNeverBeCalled();
    }
    return true;
}

private static void mustNeverBeCalled() {
    throw new Error("mustNeverBeCalled has been called");
}

}

All the lines after if (earlyReturn) will be colored wrongly because this BB will have an additional coverage point after traceCmp.
wrong.Junit5Example1.java.zip
right.Junit5Example1.java.zip

Copy link
Contributor

@fmeum fmeum left a comment

Choose a reason for hiding this comment

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

Looks good, thanks!

@kmnls
Copy link
Contributor Author

kmnls commented Apr 20, 2023

@fmeum Could you please merge?

@fmeum fmeum enabled auto-merge (rebase) April 20, 2023 14:42
@kmnls
Copy link
Contributor Author

kmnls commented Apr 20, 2023

'Build all targets and run all tests / build_and_test (windows-2019, 8) (pull_request) '
is constantly failing with
'ERROR: C:/users/runneradmin/_bazel_runneradmin/2gbstkcc/external/io_bazel_rules_kotlin/src/main/kotlin/BUILD.bazel:37:12: Building external/io_bazel_rules_kotlin/src/main/kotlin/build.jar () [for tool] failed: failed to delete output files before executing action'
Probably it may be ignored?

@fmeum
Copy link
Contributor

fmeum commented Apr 20, 2023

@kmnls Yes, that's not due to this PR. I will keep trying.

@fmeum
Copy link
Contributor

fmeum commented Apr 21, 2023

@kmnls The CI failure should be fixed, but there is a test that seems to fail for reasons related to this PR. Could you take a look?

@fmeum
Copy link
Contributor

fmeum commented Apr 21, 2023

The test times out with:

== Java Exception: java.lang.IllegalStateException: Expected "11", got "10"
	at com.example.CoverageFuzzer.assertEquals(CoverageFuzzer.java:189)
	at com.example.CoverageFuzzer.assertCoverageDump(CoverageFuzzer.java:174)
	at com.example.CoverageFuzzer.fuzzerTearDown(CoverageFuzzer.java:92)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:77)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.base/java.lang.reflect.Method.invoke(Method.java:568)
	at com.code_intelligence.jazzer.driver.FuzzTargetRunner.shutdown(FuzzTargetRunner.java:371)
	at java.base/java.lang.Thread.run(Thread.java:833)

It looks like you might just have to update the count in

assertEquals(11, countHits(classToCoverCoverage.getProbes()));
.

auto-merge was automatically disabled April 21, 2023 18:59

Head branch was pushed to by a user without write access

@fmeum fmeum enabled auto-merge (squash) April 26, 2023 08:51
@fmeum fmeum merged commit c2e8901 into CodeIntelligenceTesting:main Apr 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants