Resolve Pitest Issues - IllegalImportCheck (2)

[rnveach]

Child issue of #7797 ,

checkstyle/.ci/pitest.sh

Line 83 in abf829f

"IllegalImportCheck.java.html:<td class='covered'><pre><span class='survived'> if (!result &#38;&#38; illegalClasses != null) {</span></pre></td></tr>"

[harsh-kukreja]

I'm on it

[harsh-kukreja]

Hey @rnveach
(Pitest Report)
https://harsh-kukreja.github.io/pitest-report-before/com.puppycrawl.tools.checkstyle.checks.imports/IllegalImportCheck.java.html
(Line number 422)

Hardcoded commit

harsh-kukreja@335b00f

Diff report
https://harsh-kukreja.github.io/IllegalImportCheckDiffReport3/

[rnveach]

@harsh-kukreja
https://harsh-kukreja.github.io/pitest-report/com.puppycrawl.tools.checkstyle.checks.imports/IllegalImportCheck.java.html#org.pitest.mutationtest.report.html.SourceFile@1194697_422
Pitest report is not correct. Expected line should match line in pitest.sh . It looks like pitest was run on mutated branch and not checkstyle master. Report needs to be run on master so we can see what is being reported and the mutation involved.

[harsh-kukreja]

So @rnveach basically you want me to remove the line in pitest.sh and rerun the report command?

[rnveach]

You must run pitest report on a clean checkstyle master branch. We expect no changes to be done to the branch or the report. We want to see what is being reported right now to help us understand how to fix it.

[harsh-kukreja]

Hey @rnveach
(Pitest Report)
https://harsh-kukreja.github.io/pitest-report-before/com.puppycrawl.tools.checkstyle.checks.imports/IllegalImportCheck.java.html
(Line number 422)

Hardcoded commit

harsh-kukreja@335b00f

Diff report
https://harsh-kukreja.github.io/IllegalImportCheckDiffReport3/

I have update the same comment @rnveach, please check also added the diff report

[harsh-kukreja]

@rnveach should i create a PR or i should wait for your confirmation?

[rnveach]

@harsh-kukreja You can start a PR but you have provided no guidance on what you plan to do since regression showed no cases to go by. If you can analyze the code and create a UT from it, then that is the best path. If you feel the only option is to remove the code to kill the mutation, then you need to prove to us why the code is not needed and won't cause any issues by removing it.

[harsh-kukreja]

@rnveach I removed !result because whatever the input string comes this line will get executed so there is no point adding this !result

if we want the if to not to work we can do the following:

private boolean isIllegalImportByPackagesAndClassNames(String importText) {
        boolean result = false;
        for (String element : illegalPkgs) {
            if (importText.startsWith(element + ".")) {
                // the below line ensures that if there is an illegalPackage Import then directly return
                return true;
            }
        }
        if (!result && illegalClasses != null) {
            for (String element : illegalClasses) {
                if (importText.equals(element)) {
                    result = true;
                    break;
                }
            }
        }
        return result;
    }

Or else let the mutation survive

[harsh-kukreja]

@rnveach please rply

[rnveach]

// the below line ensures that if there is an illegalPackage Import then directly return
return true;

This is not possible as it would violate our checkstyle rule that there should only be 1 return statement in a method.

else let the mutation survive

We prefer to avoid this at any circumstance if we cannot find a way to kill and we know for sure the code is needed (or atleast sure enough that it is not a good idea to remove it).

whatever the input string comes this line will get executed so there is no point adding this !result

As the code is written right now line 423 - 428 will not get executed because the !result will prevent it from being executed.

I need more details on why there is absolutely no other way than to remove this code and no impact to the check. For example, lets look at a test case line by line where !result is false but it still enters the if condition on line 422 like in the mutated branch. Why does it not cause a difference in this scenario?

If it does prove to be never a difference, we can't rewrite the code some other way other than removing the !result? Looking at the code alone, it looks like this was added to prevent executing the if statement when we already know the result is true.

[harsh-kukreja]

Yes @rnveach you are right the if line at 422 will not get executed if there is illegalPackage import.
Removing !result will just increase our compilation time which we don't want to. So we need to keep the !result in the code
There will be no difference in the output if we put both illegal(package and class) then too the result will be same because the result was set to true in the illegalPackage loop itself. So we need to have !result in our code

[rnveach]

Removing !result will just increase our compilation time which we don't want to. So we need to keep the !result in the code

And there is no way to rewrite the code to give us the same benefit as well as removing the mutation?

For example, do we need to execute both loops or can we know ahead to only execute one? In this case this looks to be a yes, but, again, it is just an example.

will just increase our compilation time which we don't want to

It is not desirable, but it may be the only solution unless you can provide us another workaround.

[harsh-kukreja]

@rnveach How can we execute only one loop as the first loop checks for illegalpackage and other checks for illegalclass the solution i thought was the block of code which i gave

[rnveach]

@harsh-kukreja As stated above, it was just an example and I already said we couldn't do that and current tests should prove that if we did change the code. I am just seeing if we can think outside the box and see if there is some other way to implement this functionality to pass pitest without removing it.

[harsh-kukreja]

Hey @rnveach

private boolean isIllegalImportByPackagesAndClassNames(String importText) {
        boolean result = false;
        String[] temObject = illegalPkgs;
        if (illegalClasses != null){
            temObject = Stream.concat(Arrays.stream(illegalPkgs), Arrays.stream(illegalClasses))
                    .toArray(String[]::new);
        }

        for (String element : temObject) {
            if (importText.startsWith(element)) {
                result = true;
                break;
            }
        }

        return result;
    }

We can write the code like this by concatenating both of them

[rnveach]

It seems to me that just creates more work than just removing the condition as we now have to concat them and then turn them back into an array again.

I have thought about this and don't have any ideas either. I can't see a way around just not removing the condition.

Feel free to start a PR to remove the code.

[rnveach]

Fix was merged