Resolve Pitest Issues - ImportOrderCheck (6)

[rnveach]

Child issue of #7797 ,

checkstyle/.ci/pitest.sh

Line 91 in abf829f

"ImportOrderCheck.java.html:<td class='covered'><pre><span class='survived'> return !beforeFirstImport &#38;&#38; line - lastImportLine &#62; 1;</span></pre></td></tr>"

[bossever]

I am on it.

[bossever]

I ran

groovy diff.groovy --localGitRepo /home/ayushman/Documents/Development/checkstyle --baseBranch pitestIssue6 --patchBranch mutated --config /home/ayushman/Desktop/Checkstyle\ Experiments/ImportOrder/configs.xml --listOfProjects projects-to-test-on.properties

to generate diff report, but I'm getting this error -

Error: file Checkstyle does not exist!
Caught: java.lang.IllegalArgumentException: Error: invalid command line arguments!
java.lang.IllegalArgumentException: Error: invalid command line arguments!
	at java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method)
	at java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:62)
	at java.base/jdk.internal.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45)
	at launch.run(launch.groovy:13)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
Caught: groovy.lang.GroovyRuntimeException: Error: !
groovy.lang.GroovyRuntimeException: Error: !
	at java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method)
	at java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:62)
	at java.base/jdk.internal.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45)
	at diff.executeCmd(diff.groovy:471)
	at diff.executeCmd(diff.groovy:465)
	at diff$executeCmd$10.callCurrent(Unknown Source)
	at diff.generateCheckstyleReport(diff.groovy:217)
	at diff$generateCheckstyleReport$6.callCurrent(Unknown Source)
	at diff.run(diff.groovy:30)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)

I checked my command again and it seems to be correct, any help would be appreciated.
@aryaniiit002 @nmancus1

[strkkk]

@bossever you can try to debug diff.groovy file to check what is wrong with arguments.
Also, it is better to discuss such things in gitter or in PR.

[nrmancuso]

@bossever remove the space from your config file's path. It is a good practice not to have spaces in filenames and file paths.

[bossever]

@bossever remove the space from your config file's path. It is a good practice not to have spaces in filenames and file paths.

Thanks, that made it work.

[bossever]

Pitest Report:

https://bossever.github.io/issue7877/pit-reports/202102122217/com.puppycrawl.tools.checkstyle.checks.imports/ImportOrderCheck.java.html#org.pitest.mutationtest.report.html.SourceFile@476dda8b_878

Surviving mutation on line 878:

removed conditional - replaced equality check with true → SURVIVED

Hardcoded mutation is here.

Diff Report: https://bossever.github.io/issue7877/diff-report/index.html

Config used for diff report: https://gist.github.com/bossever/9af6e0f85a4fdabaedebf3863349fc9f
Let me know if I should add more permutations to the config.

[bossever]

I have mostly understood how pitest works and what it is meant for, but I am having some trouble understanding how diff regression works. From what I can gather, it runs checkstyle on various projects from two branches (the baseBranch and the patchBranch) and tries to find differences between the output of the two.
Please correct me if I'm wrong.
Thanks!

[romani]

You are correct

[bossever]

I am somehwat confused about the usage of variable beforeFirstImport in this method.

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

Lines 877 to 879 in 9c53648

	private boolean isSeparatorBeforeImport(int line) {
	return !beforeFirstImport && line - lastImportLine > 1;
	}

The Javadoc description for it is -

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

Lines 601 to 602 in 9c53648

	/** Whether there was any imports. */
	private boolean beforeFirstImport;

This would imply that it is true whenever there were any imports at all.
However, the variable name seems to mean something else - before First Import
I tried to understand the variable by looking at its usage, but I couldn't get it, any help would be appreciated.

After understanding it, I think I could come up with a test case where the mutation can be killed.

[rnveach]

Like the variable says, it defines whether we are at or before the first import. The variable is true when we begin any new AST tree and only becomes false after we complete our first visit and examine an import statement.

[bossever]

Thanks, that made it clear!

[bossever]

After a lot of thought, I believe that this particular mutation cannot be killed. I have explained why below.
The method at which mutation occurred returns whether there is a separator before current import. It calculates this using current line number and the line number of the last import. It is only called once, inside isSeparatorInGroup to check whether there is an unnecessary separator in an import group.

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

Lines 877 to 879 in 9c53648

	private boolean isSeparatorBeforeImport(int line) {
	return !beforeFirstImport && line - lastImportLine > 1;
	}

When !beforeFirstImport is changed to true by pitest, the return statement is efectively reduced to

    return line - lastImportLine > 1; 

I could create a UT in two ways -

Case 1:

$ cat Test.java

import java.util.HashMap;
// Unnecessary separator
import java.util.LinkedList;

public class Test {}

Expected violation: Extra separation in import group before 'java.util.LinkedList'
Actual violation: null

For this, I would need the method isSeparatorBeforeImport to return false, which is only possible when line - lastImportLine > 1 is false, which is clearly not the case. So case 1 will not work.

Case 2:

$ cat Test.java

import java.util.HashMap;
import java.util.LinkedList;

public class Test {}

Expected violation: null
Actual violation: Extra separation in import group before 'java.util.LinkedList'

This would require line - lastImportLine > 1 to be true, which is again, not possible for this test file. So case 2 will also not work.

Conclusion:

I think !beforeFirstImport cannot be removed either because that would break the logic of the method. I have not tested this with non-default configuration of check, because in all possible configurations, extra separator in import group will always produce violation.
I request admins to have a look and let me know if I have missed anything.
@romani @strkkk @nmancus1

[nrmancuso]

Extra separation in import group before 'java.io.IOException'

I do not see this import in your example.

[bossever]

I do not see this import in your example.

Ah, looks like I forgot to change the violation message appropriately, will change now.

[nrmancuso]

@bossever it looks like a lot of refactoring around this method happened at 9e24cf1 , and checking !beforeFirstImport in this method might be unnecessary now. Please submit a PR to remove this condition. Make sure to link to all of your research in the PR description.

Edit: please generate a new diff report in your PR, and add one more config with both <property name="separated" value="true"/> and <property name="separatedStaticGroups" value="true"/> in the same config.

[bossever]

@nmancus1 If I remove !beforeFirstImport, the method's return value will be entirely dependent on line - lastImportLine > 1. But, this method is called before lastImportLine is actually assigned a line number. (It is initialized with Integer.MIN_VALUE). Wouldn't that result in unexpected behaviour?

[nrmancuso]

Wouldn't that result in unexpected behaviour?

No, this is what pitest is telling you; no existing test inputs are hitting this code such that mutating !beforeFirstImport to true is failing. We must either find a test case where this mutation fails (then the mutation is "killed"), or remove this condition, since it isn't doing us any good right now.

To convince yourself, run mvn clean verify on your mutated branch; it should pass all tests.

Edit: I misread your comment, ignore the following:

> called before lastImportLine is actually assigned a line number

lastImportLine is assigned before we begin visiting tokens in this check.

[bossever]

mvn clean verify is indeed passing, but I can't understand the behind the scenes. This in particular -

But, this method is called before lastImportLine is actually assigned a line number. (It is initialized with Integer.MIN_VALUE).

Is it so that removal of the condition is the only other option? If so, I will create the PR immediately.

[nrmancuso]

Is it so that removal of the condition is the only other option?

You have two options, as stated above:

1. Find a test case that depends on this condition.
2. Remove the condition.

In order to find test cases, we usually look to the regression report (which is clean in this case, so no help there) or think of some way to hit the code otherwise (with real input, not reflection, etc. in this case). It is good to think about it logically; is there ever a case where the result of this method does not solely depend on line - lastImportLine > 1 ?

[bossever]

Hmm okay, I don't think I can come up with a test case that can kill the mutation, so I will remove this condition now.

[strkkk]

fix is merged