Skip to content

Issue #13095: kill mutation for VariableDeclarationUsageDistanceCheck 4#13120

Merged
rdiachenko merged 1 commit intocheckstyle:masterfrom
Kevin222004:M5
Jul 1, 2023
Merged

Issue #13095: kill mutation for VariableDeclarationUsageDistanceCheck 4#13120
rdiachenko merged 1 commit intocheckstyle:masterfrom
Kevin222004:M5

Conversation

@Kevin222004
Copy link
Copy Markdown
Contributor

@Kevin222004 Kevin222004 commented Jun 1, 2023

Issue #13095: kill mutation for VariableDeclarationUsageDistanceCheck 4

Check

https://checkstyle.org/config_coding.html#VariableDeclarationUsageDistance

Mutation

<mutation unstable="false">
<sourceFile>VariableDeclarationUsageDistanceCheck.java</sourceFile>
<mutatedClass>com.puppycrawl.tools.checkstyle.checks.coding.VariableDeclarationUsageDistanceCheck</mutatedClass>
<mutatedMethod>isInitializationSequence</mutatedMethod>
<mutator>org.pitest.mutationtest.engine.gregor.mutators.RemoveConditionalMutator_EQUAL_IF</mutator>
<description>removed conditional - replaced equality check with true</description>
<lineContent>while (result</lineContent>
</mutation>

  1. Explanation:-
    My thinking is that instead of removing the condition, we can continue if the token is SEMI and set the result to true. Now this won't have any problem as other branches can alter the result in the subsequent iterations and result is by default true.

A single dangling ; would be EMPTY_STAT and not SEMI. Please run regression on this method.

  1. Regression

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

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


Diff Regression config: https://gist.githubusercontent.com/Kevin222004/12b3bfed9dcea32830f532159c5023c6/raw/1d2cf31a526f452572fc41bf922762ecc5c6b7d6/variable.xml
Diff Regression projects: https://gist.githubusercontent.com/Kevin222004/21e3934e85f802e2fbd48af06d122364/raw/604256badd733d8568064f371d55657c04b00dfd/test-projects-2.properties
Report label: New-Regression-1

@Kevin222004
Copy link
Copy Markdown
Contributor Author

@Kevin222004
Copy link
Copy Markdown
Contributor Author

@romani
Copy link
Copy Markdown
Member

romani commented Jun 1, 2023

1 similar comment
@romani
Copy link
Copy Markdown
Member

romani commented Jun 1, 2023

@Kevin222004
Copy link
Copy Markdown
Contributor Author

@romani ready for review

@Kevin222004
Copy link
Copy Markdown
Contributor Author

@rdiachenko please review this as well

@romani
Copy link
Copy Markdown
Member

romani commented Jun 2, 2023

but yes the whole loop can run for more time then it was running with result

this is stable side-effect of pitest demands. Previosly, all times we followed pitest. I am not sure when to stop this fanaticism, but I ok to keep do code change in favor of pitest.

Copy link
Copy Markdown
Member

@romani romani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok to merge.

this code looks so familiar .....
@Vyom-Yadav , did we merge your update for this method ?

@rdiachenko
Copy link
Copy Markdown
Member

rdiachenko commented Jun 2, 2023

but yes the whole loop can run for more time then it was running with result

this is stable side-effect of pitest demands. Previosly, all times we followed pitest. I am not sure when to stop this fanaticism, but I ok to keep do code change in favor of pitest.

@romani even though, it is a stable side-effect, we are lucky there are no heavy operations in the loop. I'm more inclined to not follow pitest in such cases and put this case to a list of potentially bad side effects.

@Kevin222004 could you resolve a conflict and regenerate report please?

@Kevin222004
Copy link
Copy Markdown
Contributor Author

@Kevin222004 could you resolve a conflict and regenerate report please?

started

@Kevin222004
Copy link
Copy Markdown
Contributor Author

@romani
Copy link
Copy Markdown
Member

romani commented Jun 3, 2023

@Kevin222004 , please create issue on side effects, to let you be able to modify it as we go.

@rdiachenko , for now lets keep moving with following pitest, in this particular case it is nano optimization, so better to have NOT pitest survival, it will help later on in maintenance and defending no survival from other contributors.
Unfortunately presence of any let contributors possibility to appeal to do same, and hard time for maintainers to agree or push back.

@Kevin222004
Copy link
Copy Markdown
Contributor Author

Copy link
Copy Markdown
Member

@rdiachenko rdiachenko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@Vyom-Yadav
Copy link
Copy Markdown
Member

Vyom-Yadav commented Jun 4, 2023

@Vyom-Yadav , did we merge your update for this method ?

@romani No, would you like to merge it first?

Edit:
I would suggest merging that first.

@romani
Copy link
Copy Markdown
Member

romani commented Jun 4, 2023

@Vyom-Yadav , your update/PR still not ready, there items to resolve.

@Vyom-Yadav
Copy link
Copy Markdown
Member

@Kevin222004 Instead of removing the condition try:

while (result                                                                         
        && !isUsedVariableDeclarationFound                                            
        && currentSiblingAst != null) {                                               
    switch (currentSiblingAst.getType()) {                                            
        case TokenTypes.EXPR:                                                         
            final DetailAST methodCallAst = currentSiblingAst.getFirstChild();        
                                                                                      
            if (methodCallAst.getType() == TokenTypes.METHOD_CALL) {                  
                final String instanceName =                                           
                    getInstanceName(methodCallAst);                                   
                // method is called without instance                                  
                if (instanceName.isEmpty()) {                                         
                    result = false;                                                   
                }                                                                     
                // differs from previous instance                                     
                else if (!instanceName.equals(initInstanceName)) {                    
                    if (initInstanceName.isEmpty()) {                                 
                        initInstanceName = instanceName;                              
                    }                                                                 
                    else {                                                            
                        result = false;                                               
                    }                                                                 
                }                                                                     
            }                                                                         
            else {                                                                    
                // is not method call                                                 
                result = false;                                                       
            }                                                                         
            break;                                                                    
                                                                                      
        case TokenTypes.VARIABLE_DEF:                                                 
            final String currentVariableName = currentSiblingAst                      
                .findFirstToken(TokenTypes.IDENT).getText();                          
            isUsedVariableDeclarationFound = variableName.equals(currentVariableName);
            break;                                                                    
                                                                                      
        default:                                                                      
            result = currentSiblingAst.getType() == TokenTypes.SEMI;                  
    }                                                                                 
                                                                                      
    currentSiblingAst = currentSiblingAst.getPreviousSibling();                       
}                                                                                     

My thinking is that instead of removing the condition, we can continue if the token is SEMI and set the result to true. Now this won't have any problem as other branches can alter the result in the subsequent iterations and result is by default true.

A single dangling ; would be EMPTY_STAT and not SEMI. Please run regression on this method.

@Vyom-Yadav
Copy link
Copy Markdown
Member

We would be able to kill the mutation this way and have the condition to save time too.

@Kevin222004
Copy link
Copy Markdown
Contributor Author

Github, generate report

Copy link
Copy Markdown
Member

@Vyom-Yadav Vyom-Yadav left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Jun 7, 2023

@Kevin222004
Copy link
Copy Markdown
Contributor Author

Github, generate report

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Jun 7, 2023

Report generation failed on phase "parse_body",
step "Parsing content of PR description".
Link: https://github.com/checkstyle/checkstyle/actions/runs/5204508175

@romani
Copy link
Copy Markdown
Member

romani commented Jun 13, 2023

@Kevin222004 , please resolve conflict meanwhile

@Kevin222004 Kevin222004 force-pushed the M5 branch 3 times, most recently from 58efd9b to 169809f Compare June 19, 2023 09:35
Copy link
Copy Markdown
Member

@romani romani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@romani
Copy link
Copy Markdown
Member

romani commented Jun 19, 2023

@Kevin222004 , please resolve IDEA inspection.

@Kevin222004
Copy link
Copy Markdown
Contributor Author

Github, generate report

@github-actions
Copy link
Copy Markdown
Contributor

@Kevin222004 Kevin222004 force-pushed the M5 branch 2 times, most recently from 85faf71 to cb2c2fd Compare June 23, 2023 11:59
@Kevin222004
Copy link
Copy Markdown
Contributor Author

Github, generate report

@Kevin222004
Copy link
Copy Markdown
Contributor Author

@romani Please check failure are not related to change

@github-actions
Copy link
Copy Markdown
Contributor

@Kevin222004
Copy link
Copy Markdown
Contributor Author

Github, generate report

@github-actions
Copy link
Copy Markdown
Contributor

@rdiachenko
Copy link
Copy Markdown
Member

rdiachenko commented Jun 29, 2023

@Kevin222004 please rebase on master and make sure all checks are green.

@romani
Copy link
Copy Markdown
Member

romani commented Jun 29, 2023

Rebased in GitHub

Copy link
Copy Markdown
Member

@rdiachenko rdiachenko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants