Resolve Pitest Issues - CustomImportOrderCheck (3)

[rnveach]

Child issue of #7797 ,

checkstyle/.ci/pitest.sh

Line 79 in abf829f

"CustomImportOrderCheck.java.html:<td class='covered'><pre><span class='survived'> if (bestMatch.group.equals(NON_GROUP_RULE_GROUP)) {</span></pre></td></tr>"

[DXTkastb]

I'm on it.

[DXTkastb]

@rnveach I am generating pitest regression report for this check on different configurations but I receive this error:
Failed to execute goal org.apache.maven.plugins:maven-site-plugin:3.3:site (default-site) on project sample: Error during page generation: Error rendering Maven report: Failed during checkstyle configuration: Exception was thrown while processing /home/kaustubh/ck/contribution-master/checkstyle-tester/src/main/java/openjdk9/src/java.rmi/share/classes/module-info.java: NoViableAltException occurred while parsing file /home/kaustubh/ck/contribution-master/checkstyle-tester/src/main/java/openjdk9/src/java.rmi/share/classes/module-info.java.
I have tried 3-4 configurations but they all fail. Another thing I found is regression reports are generated for guava project. Rest projects are throwing the above mentioned error.
Could you please help me in eradicating this error.

[rnveach]

Please ensure you have the following lines in your config. They are required for regression.
https://github.com/checkstyle/contribution/blob/master/checkstyle-tester/my_check.xml#L9-L13

[DXTkastb]

Pitest report:
https://dxtkastb.github.io/checkstyle/reports/202003212115/com.puppycrawl.tools.checkstyle.checks.imports/CustomImportOrderCheck.java.html

Surviving Mutation:
Line 771: if (bestMatch.group.equals(NON_GROUP_RULE_GROUP)) {
removed conditional - replaced equality check with true → SURVIVED

Hardcoded Branch:
DXTkastb@dece5da

Pitest regression report:
https://dxtkastb.github.io/checkstyle/reports/diff/index.html

https://dxtkastb.github.io/importordernewdiff/index.html

Code analysis:
The regression report did not provide any help that would kill the mutation.

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

Lines 757 to 789 in f9131f9

	private String getImportGroup(boolean isStatic, String importPath) {
	RuleMatchForImport bestMatch = new RuleMatchForImport(NON_GROUP_RULE_GROUP, 0, 0);
	if (isStatic && customImportOrderRules.contains(STATIC_RULE_GROUP)) {
	bestMatch.group = STATIC_RULE_GROUP;
	bestMatch.matchLength = importPath.length();
	}
	else if (customImportOrderRules.contains(SAME_PACKAGE_RULE_GROUP)) {
	final String importPathTrimmedToSamePackageDepth =
	getFirstDomainsFromIdent(samePackageMatchingDepth, importPath);
	if (samePackageDomainsRegExp.equals(importPathTrimmedToSamePackageDepth)) {
	bestMatch.group = SAME_PACKAGE_RULE_GROUP;
	bestMatch.matchLength = importPath.length();
	}
	}
	if (bestMatch.group.equals(NON_GROUP_RULE_GROUP)) {
	for (String group : customImportOrderRules) {
	if (STANDARD_JAVA_PACKAGE_RULE_GROUP.equals(group)) {
	bestMatch = findBetterPatternMatch(importPath,
	STANDARD_JAVA_PACKAGE_RULE_GROUP, standardPackageRegExp, bestMatch);
	}
	if (SPECIAL_IMPORTS_RULE_GROUP.equals(group)) {
	bestMatch = findBetterPatternMatch(importPath,
	group, specialImportsRegExp, bestMatch);
	}
	}
	}
	if (bestMatch.group.equals(NON_GROUP_RULE_GROUP)
	&& customImportOrderRules.contains(THIRD_PARTY_PACKAGE_RULE_GROUP)
	&& thirdPartyPackageRegExp.matcher(importPath).find()) {
	bestMatch.group = THIRD_PARTY_PACKAGE_RULE_GROUP;
	}
	return bestMatch.group;
	}

The method(Line:757 getImportGroup(boolean isStatic, String importPath) ) assigns an import to an appropriate group.
This if condition(Line:771) is actually useless, It checks whether bestMatch.group is assigned previously a value(STATIC_RULE_GROUP/SAME_PACKAGE_RULE_GROUP). But even if it is initialized before it doesn't matter because static and same_package rule have higher priority than standard_java_pkg rule and special_imports rule( if import matches more than one group) so it won't effect the working.
I removed this condition and the mutation was killed.
Pitest report now:
https://dxtkastb.github.io/checkstyle/reports/pit2/com.puppycrawl.tools.checkstyle.checks.imports/CustomImportOrderCheck.java.html

@rnveach Please review.

[rnveach]

Pitest report, mutated branch, and regression look good.

because static and same_package rule have higher priority than standard_java_pkg rule and special_imports rule

Please point to this code that this statement comes from. So absolutely under no other circumstance will some other rule override those 2 in this scenario? Please provide that proof too.

Is there some other way to rewrite the code to kill the mutation besides removing the if condition? It seems like wasted effort to run findBetterPatternMatch when we know its value won't matter.

Line 783 has a NON_GROUP_RULE_GROUP check too that is killed. Can we just merge these 2 conditions together?

[DXTkastb]

private String getImportGroup(boolean isStatic, String importPath) {
    RuleMatchForImport bestMatch = new RuleMatchForImport(NON_GROUP_RULE_GROUP, 0, 0);
    if (isStatic && customImportOrderRules.contains(STATIC_RULE_GROUP)) {
        bestMatch.group = STATIC_RULE_GROUP;
        bestMatch.matchLength = importPath.length();
    }
    else if (customImportOrderRules.contains(SAME_PACKAGE_RULE_GROUP)) {
        final String importPathTrimmedToSamePackageDepth =
                getFirstDomainsFromIdent(samePackageMatchingDepth, importPath);
        if (samePackageDomainsRegExp.equals(importPathTrimmedToSamePackageDepth)) {
            bestMatch.group = SAME_PACKAGE_RULE_GROUP;
            bestMatch.matchLength = importPath.length();
        }
    }
    if (bestMatch.group.equals(NON_GROUP_RULE_GROUP)) {
        for (String group : customImportOrderRules) {
            if (STANDARD_JAVA_PACKAGE_RULE_GROUP.equals(group)) {
                bestMatch = findBetterPatternMatch(importPath,
                        STANDARD_JAVA_PACKAGE_RULE_GROUP, standardPackageRegExp, bestMatch);
            }
            if (SPECIAL_IMPORTS_RULE_GROUP.equals(group)) {
                bestMatch = findBetterPatternMatch(importPath,
                        group, specialImportsRegExp, bestMatch);
            }
        }
    }
    if (bestMatch.group.equals(NON_GROUP_RULE_GROUP)
            && customImportOrderRules.contains(THIRD_PARTY_PACKAGE_RULE_GROUP)
            && thirdPartyPackageRegExp.matcher(importPath).find()) {
        bestMatch.group = THIRD_PARTY_PACKAGE_RULE_GROUP;
    }
    return bestMatch.group;
}


private static RuleMatchForImport findBetterPatternMatch(String importPath, String group,
        Pattern regExp, RuleMatchForImport currentBestMatch) {
    RuleMatchForImport betterMatchCandidate = currentBestMatch;
    final Matcher matcher = regExp.matcher(importPath);
    while (matcher.find()) {
        final int length = matcher.end() - matcher.start();
        if (length > betterMatchCandidate.matchLength
                || length == betterMatchCandidate.matchLength
                    && matcher.start() < betterMatchCandidate.matchPosition) {
            betterMatchCandidate = new RuleMatchForImport(group, length, matcher.start());
        }
    }
    return betterMatchCandidate;
}

Please point to this code that this statement comes from. So absolutely under no other circumstance will some other rule override those 2 in this scenario? Please provide that proof too.

If the conditions for Static/Same_package hold (in getImportGroup(...) method) , then also the import wouldn't change group because
matchlength of the import is initialized which will fail the condition of

        if (length > betterMatchCandidate.matchLength
                || length == betterMatchCandidate.matchLength
                    && matcher.start() < betterMatchCandidate.matchPosition) {
            betterMatchCandidate = new RuleMatchForImport(group, length, matcher.start());
        }

in findBetterPatternMatch method
Here length is the matched substring length which be in most cases is less than betterMatchCandidate.matchlength, even if I provide the same name regexp for standard_package/special_imports( so that length=betterMatchCandidate.matchLength) then betterMatchCandidate.matchPosition is already assigned to 0, which will always be less<= to mathcer.start(). so
|| length == betterMatchCandidate.matchLength && matcher.start() < betterMatchCandidate.matchPosition
this condition also fails.

findBetterPatternMatch method would come into play if standard_package and Special_imports have a dispute on the same regexp .

---

Line 783 has a NON_GROUP_RULE_GROUP check too that is killed. Can we just merge these 2 conditions together?

I tried this too but the some tests failed so I did not dig deep into this scenario.

---

One more thing,when I changed the if (bestMatch.group.equals(NON_GROUP_RULE_GROUP)) {
to ( for reducing processing time)
if (!((bestMatch.group.equals(STATIC_RULE_GROUP)) || (bestMatch.group.equals(SAME_PACKAGE_RULE_GROUP))))

Pitest comes with another mutation surviving:

removed conditional - replaced equality check with true → SURVIVED

removed conditional - replaced equality check with true → SURVIVED

So I think it is better to just remove the if condition. If everything sounds fine should I start a PR?

[DXTkastb]

@rnveach could you please review. If there's a problem, let me know. If everthing seems fine should I start a PR?

[DXTkastb]

@rnveach ping

[rnveach]

Fix was merged