Issue #12326: Resolve Pitest suppression for AccessModifierOption #12340

ThatSneakyCoder · 2022-10-23T13:44:23Z

Referencing from #12326
also related to #12341

ThatSneakyCoder · 2022-10-23T13:46:34Z

Github, generate report

romani · 2022-10-23T13:50:07Z

We do not allow pure Junit test methods anymore.
Please make coverage by Input files and real Checks.

ThatSneakyCoder · 2022-10-23T13:50:24Z

ok @romani

nrmancuso · 2022-10-23T13:53:55Z

@shubh220922 please hardcode mutation first and try to understand why this mutation survives our tests.

ThatSneakyCoder · 2022-10-23T13:54:24Z

@nrmancuso okay going through this.

nrmancuso · 2022-10-23T13:58:27Z

@shubh220922 also, please keep hack like 4f9c2a4 in mind to kill mutation (using config input to kill mutation)

ThatSneakyCoder · 2022-10-24T08:49:58Z

@nrmancuso Should I write input files for the tests previously present in the file as well or only for mine.

nrmancuso · 2022-10-24T14:40:27Z

@shubh220922 please do not modify existing test inputs, also make sure to read through #12341

ThatSneakyCoder · 2022-11-10T08:00:56Z

I'll work on this pr within the next 3 days. My exams are going on.

ThatSneakyCoder · 2022-11-15T13:01:37Z

@nrmancuso I've been trying to understand the meaning of replaced call to ..... with receiver. What is this receiver? Also I found this term naked receiver somewhere and wanted to know what it was. I surfed all of the internet for this but couldn't understand what it means. Could you please help me figure this out.

nrmancuso · 2022-11-15T13:05:32Z

@shubh220922 please read pitest documentation for this mutation operator; share specific points you need clarification on https://pitest.org/quickstart/mutators/#EXPERIMENTAL_NAKED_RECEIVER

Edit: I see you edited your comment, see our wiki at https://github.com/checkstyle/checkstyle/wiki/How-to-Generate-and-Analyze-Pitest-Reports#experimental-naked-receiver for code example

ThatSneakyCoder · 2022-11-15T13:08:51Z

I have hardcoded the mutation in the AccessModifierOption.java class by removing trim() from the line where mutation was occurring. Will study the CI.

romani

item:

romani · 2022-11-15T13:11:27Z

src/main/java/com/puppycrawl/tools/checkstyle/checks/naming/AccessModifierOption.java

@@ -63,7 +63,7 @@ private String getName() {
     * @return the AccessModifier associated with given access modifier name.
     */
    public static AccessModifierOption getInstance(String modifierName) {
-        return valueOf(AccessModifierOption.class, modifierName.trim().toUpperCase(Locale.ENGLISH));
+        return valueOf(AccessModifierOption.class, modifierName.toUpperCase(Locale.ENGLISH));


https://github.com/checkstyle/checkstyle/pull/12263/files#diff-81239df2f2f1b6a10b954c1086a273293352aa3dde3554cc04ce0005661f673cR3

ThatSneakyCoder · 2022-11-17T06:38:08Z

Now in order to find what violation I will get at respective lines, I will have to check through running the config but the following is being displayed:

PS C:\Users\shubh\OneDrive\Desktop\checkstyle project> cat config.xml                                                                      
<?xml version="1.0"?>
<!DOCTYPE module PUBLIC "-//Puppy Crawl//DTD Check Configuration 1.3//EN"
        "https://checkstyle.org/dtds/configuration_1_3.dtd">

<module name="Checker">
    <property name="charset" value="UTF-8" />
    <module name="TreeWalker">
        <module name="AccessModifierOption">
        </module>
    </module>
</module>
PS C:\Users\shubh\OneDrive\Desktop\checkstyle project> cat test.java                                                                       
/*
AccessModifierOption
option = \tSPACE


*/

package com.puppycrawl.tools.checkstyle.checks.naming.accessmodifieroption;

public class InputAccessModifierOption {

    class classOne {

    }

    class classTwo {

    }

    class classThree {

    }

    public void method1() {

    }

    public void method2() {

    }

}
PS C:\Users\shubh\OneDrive\Desktop\checkstyle project> java -jar "C:\Users\shubh\Downloads\checkstyle-10.4-all.jar" -c config.xml test.java
com.puppycrawl.tools.checkstyle.api.CheckstyleException: cannot initialize module TreeWalker - cannot initialize module AccessModifierOption - Unable to instantiate 'AccessModifierOption' class, it is also not possible to instantiate
        ... 5 more
Caused by: com.puppycrawl.tools.checkstyle.api.CheckstyleException: Unable to instantiate 'AccessModifierOption' class, it is also not possible to instantiate it as .AccessModifierOption, AccessModifierOptionCheck, .AccessModifierOptionCheck. Please recheck that class name is specified as canonical name or read how to configure short name usage https://checkstyle.org/config.html#Packages. Please also recheck that provided ClassLoader to Checker is configured correctly.
        at com.puppycrawl.tools.checkstyle.PackageObjectFactory.createModule(PackageObjectFactory.java:214)
        at com.puppycrawl.tools.checkstyle.TreeWalker.setupChild(TreeWalker.java:120)
        ... 7 more
Checkstyle ends with 1 errors.

It is giving me this error @nrmancuso.
At this point how do I find out what error will come at what line.

nrmancuso · 2022-11-17T11:12:56Z

@shubh220922 you must use a check that uses AccessModifierOption

Example: https://checkstyle.sourceforge.io/config_javadoc.html#JavadocMethod

ThatSneakyCoder · 2022-11-20T12:05:46Z

@nrmancuso i have made some changes.. Thanks to your link given at #12340 (comment).

But I am getting this error when I am running the test that I have made:

EDIT1: It is not able to find the messages bundle for some reason. What have I missed @nrmancuso.

nrmancuso · 2022-11-20T13:19:45Z

@shubh220922 please only create a new test and input in JavadocMethodCheckTest, with tab character added to access modifier property value. You only need a very simple input, the idea is that we need check to read the property in only.

Make sure that mvn clean verify passes before next push.

ThatSneakyCoder · 2022-11-20T17:33:34Z

Mutation didn't get killed. What am I missing @nrmancuso. Could you review my changes please @nrmancuso .

Kevin222004 · 2022-11-21T12:59:43Z

@nrmancuso this is really quite suspicious. The current test case must resolve the mutation according to the config but it is not.
I have also make test to resolve it and it won't work if we are creating the test cases for the check which don't belong to package naming in which enum AccessModiferOption is created the mutation is not killed https://github.com/checkstyle/checkstyle/tree/master/src/main/java/com/puppycrawl/tools/checkstyle/checks/naming.

I have created a test case in one of this check from naming package and mutation were killed.

i am also curious to know why this is happening :), because according to the concept of mutation testing if we don't have strong enough test case for the code which we write so it will be mutated and in this currently updated pr this test is enough to kill this mutation bcs of trim func

nrmancuso · 2022-11-21T13:57:30Z

@shubh220922 this means that the string is getting trimmed before we reach this method (AccessModifierOption#getInstance). Debug the code and follow the call stack to investigate. Provide your full analysis here.

ThatSneakyCoder · 2022-11-22T12:03:59Z

@nrmancuso the following is what I got from debugging:

I have started debugging from valueOf(AccessModifierOption.class, modifierName.trim().toUpperCase(Locale.ENGLISH)); as in the image below:

I specially tagged this modifierName="\tpublic" to understand whether it is getting trimmed before reaching AccessModifierOption.getInstance() or not.

The value of token below is \tpublic which means that AccessModifierOption.getInstance(token) is receiving the non-trimmed version of \tpublic as in the image below:

Image1>

But in the Image2 result seems to contains public.

Following the stack of execution the value of value as in the image stays \tpublic until it reaches the line value=convertForCopy(value, type); as in the image below:

After crossing this line, the value of value starts to be shown asAccessModifierOption[1]@2407

I am thinking where exactly is the problem coming from?!

nrmancuso · 2022-11-22T13:24:46Z

@shubh220922 ok, we have missed a nuance here. AccessModifierOption lives in the naming package, so due to pitest config in pom.xml we only target tests in the naming package. Please do:

Remove new test + input
Add test + input (with same \t trick in config) using https://checkstyle.sourceforge.io/config_naming.html#ParameterName or some other naming check
Create issue to move AccessModifierOption ~~to api package~~, since we use this enum outside of naming and plan to use it more extensively.

romani · 2022-11-23T03:54:39Z

Create issue to move AccessModifierOption to api package,

We do all possible to avoid API package.
We want to really treat API as contract. So if we can avoid api we should avoid.

nrmancuso · 2022-11-23T04:04:12Z

Create issue to move AccessModifierOption to api package,

We do all possible to avoid API package. We want to really treat API as contract. So if we can avoid api we should avoid.

@romani let's create issue to move AccessModifierOption, we can discuss final location in issue. It doesn't make sense to keep AccessModifierOption in naming if we use it elsewhere.

ThatSneakyCoder · 2022-11-23T05:22:39Z

@nrmancuso mvn passed successfully. Mutation killed.

ThatSneakyCoder · 2022-11-23T05:27:07Z

Q.1)
@nrmancuso as you mentioned in #12340 (comment) about the config of pitest in pom.xml preventing us from killing mutation of accessModifierOption in other packages, does this rule apply to all the packages? We necessarily need to make input and test in the same package as the surviving mutation always?

Q.2) How did you figure this out in #12340 (comment) as I wasn't able to reach any conclusion through debugging.

nrmancuso · 2022-11-23T05:34:09Z

@shubh220922 test was right, but mutation was not killed. This means that we did not use this test to cover the mutation we are working on. We cannot put all tests on all packages, as pitest would take a very long time to run. I initially missed this since mutation class was suppressed in naming package, but used in other packages. This is what prompted me to mention that we should create an issue to move AccessModifiers to a more general location.

ThatSneakyCoder · 2022-11-23T06:15:52Z

@nrmancuso

This means that we did not use this test to cover the mutation we are working on.

Please see if my interpretation of this is correct or not:

Statement 1

Due to certain config of pitest present in pom.xml that we have set, it only allows for killing those mutations for which the tests and inputs are written within the same package as the main java file containing mutation

It is thereby not possible to write tests and inputs in a different package than the package that contains the file containing the mutation due to the config of pit in pom.xml

Question 1.1

Did we set this config of PIT in pom.xml? Or it is given by default by PIT?

Question 2

I initially missed this since mutation class was suppressed in naming package
Could you please help me understand this.

Question 2.1

Where is the suppression exactly?

Question 2.2

Which mutation class is being suppressed here?

Question 2.3

The suppression of the mentioned mutation class is suppressing what function here?

ThatSneakyCoder · 2022-11-23T13:29:21Z

@nrmancuso should i create issue to move AccesssModifierOption to api package?

nrmancuso · 2022-11-23T13:45:01Z

@shubh220922 #12445

nrmancuso · 2022-11-23T13:53:59Z

@shubh220922

Did we set this config of PIT in pom.xml? Or it is given by default by PIT?

See for yourself in the pom file.

Where is the suppression exactly?

This is what you have removed in this PR, read the suppression you've removed for all details. To be clear: most pitest mutation suppression files are named by package, like pitest-<package name>-suppressions.xml, so when I say that it was suppressed in naming package, this is what I was referring to.

nrmancuso

Items:

src/test/java/com/puppycrawl/tools/checkstyle/checks/naming/ParameterNameCheckTest.java

...wl/tools/checkstyle/checks/naming/parametername/InputParameterNameAccessModifierChecker.java

…Option

ThatSneakyCoder · 2022-11-23T15:17:20Z

Travis CI failing! Fixing it.

nrmancuso · 2022-11-23T15:32:13Z

Travis CI failing! Fixing it.

Disregard this failure, this is in Travis side

nrmancuso · 2022-11-24T14:13:26Z

...kstyle/checks/naming/parametername/InputParameterNameWhitespaceInAccessModifierProperty.java

+ParameterName
+format = ^h$
+ignoreOverridden = (default)false
+accessModifiers = \tpublic


config like this: public\t,\t,

In other PR, we had public, \t, case taken from this PR is with tab character "on" the access modifier. The first trim in both methods took care of the single \t, the second trim takes care of the \t "on" the access modifier.

rnveach added the awaiting changes label Nov 9, 2022

rnveach assigned ThatSneakyCoder Nov 9, 2022

ThatSneakyCoder force-pushed the shubh/mutant branch from 5d1fea9 to 973fc77 Compare November 15, 2022 13:06

romani requested changes Nov 15, 2022

View reviewed changes

checkstyle deleted a comment from ThatSneakyCoder Nov 16, 2022

ThatSneakyCoder force-pushed the shubh/mutant branch from 973fc77 to 4b685bb Compare November 17, 2022 06:34

ThatSneakyCoder force-pushed the shubh/mutant branch from 4b685bb to 856dca1 Compare November 20, 2022 11:58

ThatSneakyCoder force-pushed the shubh/mutant branch 2 times, most recently from c7b0c4c to 9ebf025 Compare November 20, 2022 17:32

nrmancuso removed the awaiting changes label Nov 21, 2022

ThatSneakyCoder force-pushed the shubh/mutant branch from 9ebf025 to af40a7a Compare November 23, 2022 05:21

nrmancuso mentioned this pull request Nov 23, 2022

Relocate AccessModifierOption #12445

Open

nrmancuso requested changes Nov 23, 2022

View reviewed changes

src/test/java/com/puppycrawl/tools/checkstyle/checks/naming/ParameterNameCheckTest.java Outdated Show resolved Hide resolved

...wl/tools/checkstyle/checks/naming/parametername/InputParameterNameAccessModifierChecker.java Outdated Show resolved Hide resolved

nrmancuso assigned nrmancuso and unassigned ThatSneakyCoder Nov 23, 2022

Issue checkstyle#12326: Resolve Pitest suppression for AccessModifier…

cb0778e

…Option

ThatSneakyCoder force-pushed the shubh/mutant branch from af40a7a to cb0778e Compare November 23, 2022 14:45

nrmancuso approved these changes Nov 23, 2022

View reviewed changes

nrmancuso assigned romani and unassigned nrmancuso Nov 23, 2022

nrmancuso mentioned this pull request Nov 23, 2022

Issue #12443: Fix null pointer exception on config value with trailin… #12444

Merged

romani approved these changes Nov 23, 2022

View reviewed changes

romani merged commit edd2b09 into checkstyle:master Nov 23, 2022

nrmancuso reviewed Nov 24, 2022

View reviewed changes

ThatSneakyCoder deleted the shubh/mutant branch November 26, 2022 05:40

Issue #12326: Resolve Pitest suppression for AccessModifierOption #12340

Issue #12326: Resolve Pitest suppression for AccessModifierOption #12340

Conversation

ThatSneakyCoder commented Oct 23, 2022 • edited

ThatSneakyCoder commented Oct 23, 2022

romani commented Oct 23, 2022

ThatSneakyCoder commented Oct 23, 2022

nrmancuso commented Oct 23, 2022

ThatSneakyCoder commented Oct 23, 2022

nrmancuso commented Oct 23, 2022

ThatSneakyCoder commented Oct 24, 2022

nrmancuso commented Oct 24, 2022 • edited

ThatSneakyCoder commented Nov 10, 2022

ThatSneakyCoder commented Nov 15, 2022 • edited

nrmancuso commented Nov 15, 2022 • edited

ThatSneakyCoder commented Nov 15, 2022 • edited

romani left a comment

Choose a reason for hiding this comment

romani Nov 15, 2022

Choose a reason for hiding this comment

ThatSneakyCoder commented Nov 17, 2022 • edited

nrmancuso commented Nov 17, 2022 • edited

ThatSneakyCoder commented Nov 20, 2022 • edited

nrmancuso commented Nov 20, 2022 • edited

ThatSneakyCoder commented Nov 20, 2022 • edited

Kevin222004 commented Nov 21, 2022 • edited

nrmancuso commented Nov 21, 2022

ThatSneakyCoder commented Nov 22, 2022

nrmancuso commented Nov 22, 2022 • edited

romani commented Nov 23, 2022

nrmancuso commented Nov 23, 2022

ThatSneakyCoder commented Nov 23, 2022

ThatSneakyCoder commented Nov 23, 2022

nrmancuso commented Nov 23, 2022

ThatSneakyCoder commented Nov 23, 2022

Statement 1

Question 1.1

Question 2

Question 2.1

Question 2.2

Question 2.3

ThatSneakyCoder commented Nov 23, 2022

nrmancuso commented Nov 23, 2022

nrmancuso commented Nov 23, 2022 • edited

nrmancuso left a comment

Choose a reason for hiding this comment

ThatSneakyCoder commented Nov 23, 2022

nrmancuso commented Nov 23, 2022

nrmancuso Nov 24, 2022

Choose a reason for hiding this comment

rnveach Nov 24, 2022

Choose a reason for hiding this comment

nrmancuso Nov 24, 2022

Choose a reason for hiding this comment

ThatSneakyCoder commented Oct 23, 2022 •

edited

nrmancuso commented Oct 24, 2022 •

edited

ThatSneakyCoder commented Nov 15, 2022 •

edited

nrmancuso commented Nov 15, 2022 •

edited

ThatSneakyCoder commented Nov 15, 2022 •

edited

ThatSneakyCoder commented Nov 17, 2022 •

edited

nrmancuso commented Nov 17, 2022 •

edited

ThatSneakyCoder commented Nov 20, 2022 •

edited

nrmancuso commented Nov 20, 2022 •

edited

ThatSneakyCoder commented Nov 20, 2022 •

edited

Kevin222004 commented Nov 21, 2022 •

edited

nrmancuso commented Nov 22, 2022 •

edited

nrmancuso commented Nov 23, 2022 •

edited