Resolve Pitest Issues - ImportOrderCheck (3)

[rnveach]

Child issue of #7797 ,

checkstyle/.ci/pitest.sh

Line 88 in abf829f

"ImportOrderCheck.java.html:<td class='covered'><pre><span class='survived'> if (caseSensitive) {</span></pre></td></tr>"

[romani]

@SunJiFengPlus , I removing you from assignments, as you need to start from "gsoc first issue", there is not any PR in three days, you should not block issue to resolve.

[nrmancuso]

I'm on it!

[nrmancuso]

Pitest Report: https://nmancus1.github.io/Issue-7874/com.puppycrawl.tools.checkstyle.checks.imports/ImportOrderCheck.java.html#org.pitest.mutationtest.report.html.SourceFile@7ea147e3_820
Surviving mutation: removed conditional - replaced equality check with false → SURVIVED
Hardcoded mutation: nrmancuso@561eabc
Diff report 1: https://nmancus1.github.io/Issue-7874/diff_1/index.html
Diff report 2: https://nmancus1.github.io/Issue-7874/diff_2/index.html

I'm going to try more configs to track this down and update this comment with another report.

[nrmancuso]

checkstyle/src/main/java/com/puppycrawl/tools/checkstyle/checks/imports/ImportOrderCheck.java

Lines 820 to 822 in 2625cbb

	if (caseSensitive) {
	compareContainersOrderResult = container1.compareTo(container2);
	}

This code is redundant, but I'm not sure it should be removed. This:

    private static int compareContainerOrder(String importName1, String importName2,
                                             boolean caseSensitive) {
        final int result;
        final String container1 = getImportContainer(importName1);
        final String container2 = getImportContainer(importName2);
        final int compareContainersOrderResult = 
                container1.compareToIgnoreCase(container2);
        if (compareContainersOrderResult == 0) {
            result = compare(importName1, importName2, caseSensitive);
        }
        else {
            result = compareContainersOrderResult;
        }
        return result;
    }

passes mvn -Dtest=ImportOrderCheckTest test and mvn -Dtest=ImportOrderCheckTest test, but doesn't make the intention clear that we want to make the initial comparison on a case sensitive basis based on the container name only. Thoughts?

[rnveach]

Pitest report, mutation branch, and regression look good from what I see.

This code is redundant

Please explain more. Since you are now talking about changing code or removing code, we need to have a deeper analysis of why code isn't working like it was probably intended. I suggest walking through the mutated branch where a case goes to opposite condition that the normal code would have prevented and see why the change makes no difference to violations.

The line in question is directly connected to a user property and just from a glance at the code, looks like pitest is saying to ignore this option. Why? What are the values of container1 and container2 and why can't we get a case in there that would make case sensitive matter?

[nrmancuso]

Thanks for encouraging me to dig deeper. I have found a case. When we have two static imports like this:

import static io.netty.handler.Codec.HTTP.HttpHeaders.elvis.same;
import static io.netty.handler.Codec.HTTP.HttpHeaders.TPARK.same;

When evaluated by if (caseSensitive) the two containers are correctly evaluated lexicographically, and compareTo returns a positive value (17) meaning that they are in the wrong order, per isWrongOrder. If we mutate caseSensitve to false, these two statements evalute to -15, which means, according to isWrongOrder that they are in the correct order.

The trick was that I need to figure out container names that would evaluate to a value of a different sign to change the outcome of the method.

[nrmancuso]

Here is my fix: nrmancuso@63bd1e4
Here is the pitest report: https://nmancus1.github.io/Issue-7874/pitest_2/202003281608/com.puppycrawl.tools.checkstyle.checks.imports/ImportOrderCheck.java.html#org.pitest.mutationtest.report.html.SourceFile@59127824_820

Let me know if we're good, then I'll submit a PR with the line removed from pitest.sh and my new UT.

[rnveach]

@nmancus1 If you have found a case that kills pitest mutation without rewriting code, then you are free to start a PR to close this issue.

[strkkk]

Fix is merged