Resolve Pitest Issues - AvoidStaticImportCheck

[rnveach]

Child issue of #7797 ,

This issue specifically focuses on the line

checkstyle/.ci/pitest.sh

Line 76 in abf829f

"AvoidStaticImportCheck.java.html:<td class='covered'><pre><span class='survived'> if (exclude.endsWith(&#34;.*&#34;)) {</span></pre></td></tr>"

[gaurabdg]

I am on it.

[gaurabdg]

1. Pitest Report:

https://gaurabdg.github.io/checkstyle-tester-reports/pitest/AvoidStaticImport/com.puppycrawl.tools.checkstyle.checks.imports/AvoidStaticImportCheck.java.html#org.pitest.mutationtest.report.html.SourceFile@46d29511_161
Surviving Mutation: removed conditional - replaced equality check with true → SURVIVED

2. Mutated Branch:

Hardcoded Mutation Commit: gaurabdg@cd24f3b

Since the mutation was removed conditional - replaced equality check with true → SURVIVED, the if statement was removed to set the execution of all the statements to always true.

3. Regression Report:

https://gaurabdg.github.io/checkstyle-tester-reports/regression/AvoidStaticImport/diff/index.html
Used only guava and elastic-search to add their file specific import excludes.
Module used in config file:

    <module name="AvoidStaticImport">
        <property name="excludes" value="com.google.common.base.*,com.google.common.collect.*,
                java.util.stream.Collectors.*,org.elasticsearch.xpack.ql.type.DataTypes.*"/>
    </module>

Diff found: 0

4. Regression and Code Logic Analysis

Although the results of the regression report wasn't useful, I have discovered an UT by analysing the code which tests a particular edge case of if(exclude.endsWith(".*")) . The reason the absence of the if statement didn't break the test codes:
if(exclude.endsWith(".*")) is redundant, since even if this check wasn't there and let's assume the exclude was a random member, it would yield true for the second condition but not for the first condition in if (classOrStaticMember.startsWith(excludeMinusDotStar)&& !classOrStaticMember.equals(excludeMinusDotStar)). and hence not move forward.

But if a specific exclude had a pattern like package.path.class.<member> where was a single letter ( for e.g. java.lang.Math.E) the absence of the if statement would yield that as if .E is .* and give a false positive. Hence, including this test case has forced the if statement to do an important check and the mutation was killed.
PR for this UT addition: #7828
Pitest Report after the fix: https://gaurabdg.github.io/checkstyle-tester-reports/pitest/AvoidStaticImport-fixed/index.html

[gaurabdg]

@rnveach Please review.

[gaurabdg]

@rnveach Added an UT. Please review

[rnveach]

Pitest report looks good and matches line in issue.

Mutated branch is acceptable.

the if statement was removed to set the execution of all the statements to always true.

I would personally keep the if and just change the complete condition to true. Ex: if (true) {.
This keeps the difference looking cleaner and straight forward on what is being changed. You don't want to accidentally delete more code than necessary as that could cause all types of issues. Checks and CI should not be a concern for mutation branches that is just for testing.

Since you have gone on to do more analysis it is not an issue but you should find some way to include all projects in regression. You can have multiple modules in 1 regression run so it isn't an issue. See https://github.com/checkstyle/contribution/tree/master/checkstyle-tester#checkstyle-pitest-regression

It is only with running regression on as many projects as possible do we go through all the weird code and such out there to help us solve this issue.

[rnveach]

Since you have found a UT and started a PR, I feel you understand pitest well enough for what needs to be done. This is usually the hardest part when regression fails us. Without a regression example, it is hard to create cases and even scarier to remove code when we are not sure how well it is used or not. We have removed code before thinking it was not needed for pitest and we were later reported with issues from our community that it was needed.

If you had not found a test case, we would be walking through the code for a negative case trying to determine if a case could be created or the code could be changed to satisfy the original condition in a new way.

In this case the condition was changed to if (true) so we know the code supports the true condition and doesn't have an issue there with pitest. When I say a negative case, we would be stepping through the code with the condition as if (false) with a case that would only be getting hit with if (true). This is the method that I feels helps understand why the test continues to work and provides greater insight on what more needs to be done.

I hope this makes sense for future contributors, but I feel you did this even if my words fail. This is specifically the process I take to work on pitest issues.

[romani]

fix is merged.

[gaurabdg]

I would personally keep the if and just change the complete condition to true. Ex: if (true) {.
This keeps the difference looking cleaner and straight forward on what is being changed. You don't want to accidentally delete more code than necessary as that could cause all types of issues. Checks and CI should not be a concern for mutation branches that is just for testing.

Okay, I removed it because of the checks. I'll keep in mind from now on.

Since you have gone on to do more analysis it is not an issue but you should find some way to include all projects in regression. You can have multiple modules in 1 regression run so it isn't an issue. See https://github.com/checkstyle/contribution/tree/master/checkstyle-tester#checkstyle-pitest-regression

It is only with running regression on as many projects as possible do we go through all the weird code and such out there to help us solve this issue.

I thought that supplying random excludes to each project separately wouldn't help me find anything, so went on to analyse the edge cases instead. But I'll keep this in mind from now on.

If you had not found a test case, we would be walking through the code for a negative case trying to determine if a case could be created or the code could be changed to satisfy the original condition in a new way.

In this case the condition was changed to if (true) so we know the code supports the true condition and doesn't have an issue there with pitest. When I say a negative case, we would be stepping through the code with the condition as if (false) with a case that would only be getting hit with if (true). This is the method that I feels helps understand why the test continues to work and provides greater insight on what more needs to be done.

A great approach and much more methodic, I'll definitely take this approach from now on, thanks for the guidance.