Issue #13328: Kill mutation ImportOrderCheck by Kevin222004 · Pull Request #13334 · checkstyle/checkstyle

Kevin222004 · 2023-07-02T05:31:08Z

Issue #13328: Kill mutation ImportOrderCheck

Check

https://checkstyle.org/checks/imports/importorder.html#ImportOrder

Mutation

checkstyle/config/pitest-suppressions/pitest-imports-suppressions.xml

Lines 57 to 64 in b8915da

    
           <mutation unstable="false"> 
        
             <sourceFile>ImportOrderCheck.java</sourceFile> 
        
             <mutatedClass>com.puppycrawl.tools.checkstyle.checks.imports.ImportOrderCheck</mutatedClass> 
        
             <mutatedMethod>beginTree</mutatedMethod> 
        
             <mutator>org.pitest.mutationtest.engine.gregor.mutators.experimental.MemberVariableMutator</mutator> 
        
             <description>Removed assignment to member variable lastImportStatic</description> 
        
             <lineContent>lastImportStatic = false;</lineContent> 
        
           </mutation>

Explaination

begin tree always reset data. for lastImportStatic = false so we need to create two input somehow lastImportStatic become true while the sequentially second file will be come in process and that is easy we only have to set last import of first file as true.

lets start from visitToken
In visit token the parameter isStatic will check wether the current import is static or not.

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

Lines 800 to 810 in b8915da

    
           final boolean isStatic; 
        
           if (ast.getType() == TokenTypes.IMPORT) { 
        
               ident = FullIdent.createFullIdentBelow(ast); 
        
               isStatic = false; 
        
           } 
        
           else { 
        
               ident = FullIdent.createFullIdent(ast.getFirstChild() 
        
                       .getNextSibling()); 
        
               isStatic = true; 
        
           }

To move further
all the condition

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

Lines 814 to 825 in b8915da

    
           if (option == ImportOrderOption.TOP || option == ImportOrderOption.ABOVE) { 
        
               final boolean isStaticAndNotLastImport = isStatic && !lastImportStatic; 
        
               doVisitToken(ident, isStatic, isStaticAndNotLastImport, ast); 
        
           } 
        
           else if (option == ImportOrderOption.BOTTOM || option == ImportOrderOption.UNDER) { 
        
               final boolean isLastImportAndNonStatic = lastImportStatic && !isStatic; 
        
               doVisitToken(ident, isStatic, isLastImportAndNonStatic, ast); 
        
           } 
        
           else if (option == ImportOrderOption.INFLOW) { 
        
               // "previous" argument is useless here 
        
               doVisitToken(ident, isStatic, true, ast); 
        
           }

call doVisitToken know our target is to check that output get affected if remove lastimportStatic from beginTree

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

Line 845 in b8915da

    
           private void doVisitToken(FullIdent ident, boolean isStatic, boolean previous, DetailAST ast) {

know in doVisitToken only 2 parameter is iportant for us isStatic and previous

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

Lines 815 to 816 in b8915da

    
           final boolean isStaticAndNotLastImport = isStatic && !lastImportStatic; 
        
           doVisitToken(ident, isStatic, isStaticAndNotLastImport, ast);

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

Lines 818 to 820 in b8915da

    
           else if (option == ImportOrderOption.BOTTOM || option == ImportOrderOption.UNDER) { 
        
               final boolean isLastImportAndNonStatic = lastImportStatic && !isStatic; 
        
               doVisitToken(ident, isStatic, isLastImportAndNonStatic, ast);

because they and the reason is they are only one right know which related with lastimportstatic and violation throwing
and in the whole method previous is used only at

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

Lines 856 to 858 in b8915da

    
           else if (groupIdx == lastGroup) { 
        
               doVisitTokenInSameGroup(isStatic, previous, name, ast); 
        
           }

know this condition is not big deal we can execute that and if we look closure on

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

Line 923 in b8915da

private void doVisitTokenInSameGroup(boolean isStatic,

this will execute when

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

Line 925 in b8915da

if (ordered) {

which is by default true so that is not issue in that if never execute because in from visittoken condition.

know in else either

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

Line 936 in b8915da

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

Lines 940 to 942 in b8915da

    
               lastImportStatic == isStatic 
        
           && isWrongOrder(name, isStatic);

this is need to be true

case 1
lets see the previous first we know that we need previos true and

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

Lines 814 to 817 in b8915da

    
           if (option == ImportOrderOption.TOP || option == ImportOrderOption.ABOVE) { 
        
               final boolean isStaticAndNotLastImport = isStatic && !lastImportStatic; 
        
               doVisitToken(ident, isStatic, isStaticAndNotLastImport, ast); 
        
           }

lets suppose we set option as above so previous import needs to be nonStatic and current ast need to be static so from here it is for sure that
we want previous as true previous = true know suppose option is set to be true so previous value is comming from visitToken so we need top set isStatic = false and lastimportStatic = true so in previous file lastimportstatic needs to be non static it means false then if it already same then no need to be reset so begin tree is redundant.

know lets suppose option is set to Bottom or under

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

Lines 818 to 821 in b8915da

    
           else if (option == ImportOrderOption.BOTTOM || option == ImportOrderOption.UNDER) { 
        
               final boolean isLastImportAndNonStatic = lastImportStatic && !isStatic; 
        
               doVisitToken(ident, isStatic, isLastImportAndNonStatic, ast); 
        
           }

in this case to set previous true we need to set lastimportstatic = true mean previous file import needs to be static and in current file ast needs to be non static it means isStatic=fasle and isLastImport=true in this case

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

Lines 856 to 858 in b8915da

    
           else if (groupIdx == lastGroup) { 
        
               doVisitTokenInSameGroup(isStatic, previous, name, ast); 
        
           }

this never will be equal so conditon will never execute

this is alsosimilar if both are static

so no test cases looks possible to clear the state using input

Regression

Report-1 :- https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/920122e_2023074505/reports/diff/index.html

Report-2 :- https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/920122e_2023090228/reports/diff/index.html

Diff Regression config: https://gist.githubusercontent.com/Kevin222004/e3a00111b4bb7b8c054483466b88606d/raw/4895c88b1daf686479058c972bad0c27cd0818ef/IOC.xml
Diff Regression projects: https://gist.githubusercontent.com/Kevin222004/9600f179b602d4c971bdb0a050099005/raw/360a95ed7bb60d7a0956e531199d484c4d6f6617/test-projects.properties
Report label: new-Regression-1

Kevin222004 · 2023-07-02T05:31:22Z

Github, generate report

github-actions · 2023-07-02T07:46:48Z

new-Regression-2:
https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/920122e_2023074505/reports/diff/index.html

Kevin222004 · 2023-07-02T07:48:08Z

Github, generate report

github-actions · 2023-07-02T09:03:20Z

new-Regression-1:
https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/920122e_2023090228/reports/diff/index.html

Vyom-Yadav

LGTM!

Vyom-Yadav · 2023-07-08T07:02:38Z

@Kevin222004 Logic looks correct to me, but to be on the safer side, please generate a report with both changes in this PR and in #13333. You can do it in a separate PR and link it back here.

Kevin222004 · 2023-07-09T12:07:46Z

@Vyom-Yadav new report #13378

Report-1 :- https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/43193d6_2023095732/reports/diff/index.html
Report-2 :- https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/43193d6_2023115512/reports/diff/index.html

rdiachenko

lgtm

played with this trying multiple inputs and combinations, didn't manage to find a failure case

romani · 2023-07-20T23:23:04Z

This is great example of scary update :).
Right now, yes, maybe we can remove this cleanup.
But it gives opportunity for module to come to wrong state and eventually produce hard to catch unstable violations that will drive crazy our users.

What if we stay on safe side with clean up of all fields in class to default state ? I know we fighting a lot on such pitest in beginTree in other Checks, and before this I was ok, and we able to find coverage.

Proposal: make method resetToDefaultIgnoredByPitest and put there only fields resets we afraid to remove. And we exclude this method from pitest to consider. We do javadoc over it to explain that we damage design a bit in favor of pitest. It doesn't not mean we can easily apply same pattern to other Modules. Allowance to be in such method will be only by very special allowance.

Let's vote, If all agree, we can do this.

rnveach

As I mentioned in discord, we have a special set of utils to kill mutations like this.

isStatefulFieldClearedDuringBeginTree

checkstyle/src/test/java/com/puppycrawl/tools/checkstyle/internal/utils/TestUtil.java

Line 134 in b2df719

    
           public static boolean isStatefulFieldClearedDuringBeginTree(AbstractCheck check,

It looks like visitToken always set the field at

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

Line 830 in e1fc7ce

lastImportStatic = isStatic;

.
All that is needed is to have the field change its value using the isStatefulFieldClearedDuringBeginTree.

rdiachenko · 2023-07-22T15:23:47Z

I'd go with what @rnveach suggests instead of having pitest specific methods in the check's class. It looks clenar and avoids questions like why we have such tool related methods in the mian code. If we do introduce resetToDefaultIgnoredByPitest sort of stuff, it opens opportunities to add similar things for violations from other tools if we can't cover them, which will make main code even more obscure.

Kevin222004 · 2023-07-24T15:54:46Z

Ok I have updated the pr with the test case suggested at #13334 (review)

romani

Item:

src/test/java/com/puppycrawl/tools/checkstyle/checks/imports/ImportOrderCheckTest.java

romani

ok to merge

rnveach · 2023-08-02T04:14:02Z

config/pitest-suppressions/pitest-imports-suppressions.xml

+
+
+
+


Why are we adding empty lines to this file?

To avoid merge Conflicts

Is the plan to remove them at some point if we can't remove the entire file?

Yes, If the entire file is not removed then I will update this and remove it. so at the, it looks clean

But right know in this file 4 mutations are still left which is possible to cover

Kevin222004 mentioned this pull request Jul 3, 2023

Targeted Pitest Mutation in July 2023 #13328

Closed

12 tasks

Kevin222004 force-pushed the BeginTree3 branch from 920122e to a4d1ad6 Compare July 3, 2023 08:48

Kevin222004 marked this pull request as ready for review July 3, 2023 08:51

Kevin222004 mentioned this pull request Jul 3, 2023

Issue #13328: Kill mutation ImportOrder #13333

Merged

romani requested review from Vyom-Yadav, rdiachenko and romani July 3, 2023 13:10

romani assigned Vyom-Yadav Jul 3, 2023

Vyom-Yadav approved these changes Jul 8, 2023

View reviewed changes

Vyom-Yadav assigned rdiachenko and unassigned Vyom-Yadav Jul 8, 2023

Kevin222004 force-pushed the BeginTree3 branch from a4d1ad6 to e1fc7ce Compare July 10, 2023 20:27

rdiachenko approved these changes Jul 18, 2023

View reviewed changes

rdiachenko assigned romani and unassigned rdiachenko Jul 18, 2023

rnveach suggested changes Jul 21, 2023

View reviewed changes

Kevin222004 force-pushed the BeginTree3 branch from e1fc7ce to 40fdf81 Compare July 24, 2023 15:52

romani requested changes Jul 26, 2023

View reviewed changes

src/test/java/com/puppycrawl/tools/checkstyle/checks/imports/ImportOrderCheckTest.java Show resolved Hide resolved

Issue checkstyle#13328: Kill mutation ImportOrderCheck

5504ac6

Kevin222004 force-pushed the BeginTree3 branch from 40fdf81 to 5504ac6 Compare July 27, 2023 15:58

Kevin222004 mentioned this pull request Jul 27, 2023

Bad Side effects of Pitest #13142

Closed

romani approved these changes Jul 29, 2023

View reviewed changes

romani assigned rnveach and unassigned romani Jul 29, 2023

Kevin222004 mentioned this pull request Aug 2, 2023

storage Kevin222004/GitHub-action#3

Closed

rnveach reviewed Aug 2, 2023

View reviewed changes

rnveach approved these changes Aug 2, 2023

View reviewed changes

rnveach merged commit 8c3b8fd into checkstyle:master Aug 2, 2023

	<mutation unstable="false">
	<sourceFile>ImportOrderCheck.java</sourceFile>
	<mutatedClass>com.puppycrawl.tools.checkstyle.checks.imports.ImportOrderCheck</mutatedClass>
	<mutatedMethod>beginTree</mutatedMethod>
	<mutator>org.pitest.mutationtest.engine.gregor.mutators.experimental.MemberVariableMutator</mutator>
	<description>Removed assignment to member variable lastImportStatic</description>
	<lineContent>lastImportStatic = false;</lineContent>
	</mutation>

	final boolean isStatic;

	if (ast.getType() == TokenTypes.IMPORT) {
	ident = FullIdent.createFullIdentBelow(ast);
	isStatic = false;
	}
	else {
	ident = FullIdent.createFullIdent(ast.getFirstChild()
	.getNextSibling());
	isStatic = true;
	}

	if (option == ImportOrderOption.TOP \|\| option == ImportOrderOption.ABOVE) {
	final boolean isStaticAndNotLastImport = isStatic && !lastImportStatic;
	doVisitToken(ident, isStatic, isStaticAndNotLastImport, ast);
	}
	else if (option == ImportOrderOption.BOTTOM \|\| option == ImportOrderOption.UNDER) {
	final boolean isLastImportAndNonStatic = lastImportStatic && !isStatic;
	doVisitToken(ident, isStatic, isLastImportAndNonStatic, ast);
	}
	else if (option == ImportOrderOption.INFLOW) {
	// "previous" argument is useless here
	doVisitToken(ident, isStatic, true, ast);
	}

	else if (groupIdx == lastGroup) {
	doVisitTokenInSameGroup(isStatic, previous, name, ast);
	}

	lastImportStatic == isStatic
	&& isWrongOrder(name, isStatic);

Uh oh!

Conversation

Kevin222004 commented Jul 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Check

Mutation

Explaination

Regression

Uh oh!

Kevin222004 commented Jul 2, 2023

Uh oh!

github-actions bot commented Jul 2, 2023

Uh oh!

Kevin222004 commented Jul 2, 2023

Uh oh!

github-actions bot commented Jul 2, 2023

Uh oh!

Vyom-Yadav left a comment

Choose a reason for hiding this comment

Uh oh!

Vyom-Yadav commented Jul 8, 2023

Uh oh!

Kevin222004 commented Jul 9, 2023

Uh oh!

rdiachenko left a comment

Choose a reason for hiding this comment

Uh oh!

romani commented Jul 20, 2023

Uh oh!

rnveach left a comment

Choose a reason for hiding this comment

Uh oh!

rdiachenko commented Jul 22, 2023

Uh oh!

Kevin222004 commented Jul 24, 2023

Uh oh!

romani left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

romani left a comment

Choose a reason for hiding this comment

Uh oh!

rnveach Aug 2, 2023

Choose a reason for hiding this comment

Uh oh!

Kevin222004 Aug 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rnveach Aug 2, 2023

Choose a reason for hiding this comment

Uh oh!

Kevin222004 Aug 2, 2023

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Kevin222004 commented Jul 2, 2023 •

edited

Loading

Kevin222004 Aug 2, 2023 •

edited

Loading