New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

pitest: increase mutation coverage for pitest-checks-whitespace profile to 100% #5003

Closed
Nimfadora opened this Issue Aug 25, 2017 · 7 comments

Comments

Projects
3 participants
@Nimfadora
Contributor

Nimfadora commented Aug 25, 2017

We should increase coverage for pitest-checks-whitespace profile up to 100%.
This issue is a subtask of #3708

Current threshold of pitest-checks-whitespace profile: 95

@Nimfadora

This comment has been minimized.

Show comment
Hide comment
@Nimfadora

Nimfadora Sep 2, 2017

Contributor

@romani There is a problem with mutations in ParenPadCheck. We have such mutation, if I remove this line, all tests pass, regression pass. Ok, what's next, in this mutation we should specify more concrete lower bound, as nodes of first several types like CTOR_DEF, METHOD_DEF will never reach this method. And here something interesting, if I set this number to 3, and revert first deletion, than check finds new problems. I don't know what to do with all remained mutations of this class, as they are weird and I can't find any logic in results I receive.

Contributor

Nimfadora commented Sep 2, 2017

@romani There is a problem with mutations in ParenPadCheck. We have such mutation, if I remove this line, all tests pass, regression pass. Ok, what's next, in this mutation we should specify more concrete lower bound, as nodes of first several types like CTOR_DEF, METHOD_DEF will never reach this method. And here something interesting, if I set this number to 3, and revert first deletion, than check finds new problems. I don't know what to do with all remained mutations of this class, as they are weird and I can't find any logic in results I receive.

@Nimfadora

This comment has been minimized.

Show comment
Hide comment
@Nimfadora

Nimfadora Oct 18, 2017

Contributor

@romani here you can see mutations remained. Situation with ParenPadCheck was described in the previous comment. What about NoWhitespaceAfterCheck Even if I find exact number for the second condition (minimal I found is 13) I will produce another mutation:
lastTypeNode.getColumnNo() > ident.getColumnNo() + 13 (+ can be replaced by - and you cannot do anything with it).

Contributor

Nimfadora commented Oct 18, 2017

@romani here you can see mutations remained. Situation with ParenPadCheck was described in the previous comment. What about NoWhitespaceAfterCheck Even if I find exact number for the second condition (minimal I found is 13) I will produce another mutation:
lastTypeNode.getColumnNo() > ident.getColumnNo() + 13 (+ can be replaced by - and you cannot do anything with it).

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Oct 20, 2017

Member

@Nimfadora ,

There is a problem with mutations in ParenPadCheck. We have such mutation, if I remove this line, all tests pass, regression pass.

please remove this line and prove it by regression - in separate PR.
I am ok to have regression in this Check, one day users will complain and we will have exact case.

ParenPadCheck ...... as nodes of first several types like CTOR_DEF, METHOD_DEF will never reach this method.

confirmed, please use reflection and pure-UT test method to cover mutation. It is not reasonable to make new set of tokens without this. Attention: each reflection usage should be explained by a comment - you have a reason. Please send this by separate PR.

Do not forget to change shippable.yml to remove grep .... --exclude=....

What about NoWhitespaceAfterCheck Even if I find exact number for the second condition

there is a good practical approach: if code is hard to cover - it highly likely is weird and should be rewritten. We have intension comment " //ident and lastTypeNode lay on one line", can we change code to be written differently ?

that code should be read like

            if (ident.getColumnNo() > ast.getColumnNo()
                || lastTypeNode.getColumnNo() > ident.getColumnNo()) {
                // it is modern Java style declaration like "String[] s = null;"
                previousElement = lastTypeNode;
            }
            else {
                // it is old C style declaration like "String s [] = null;"
                previousElement = ident;
            }

so code could be simplified as:

        //ident and lastTypeNode lay on one line
        else {
            if (ident.getColumnNo() > lastTypeNode.getColumnNo()) {
                // it is old C style declaration like "String s [] = null;"
                previousElement = ident;
            } else {
                // it is modern Java style declaration like "String[] s = null;"
                previousElement = lastTypeNode;
            }
        }

getTypeLastNode method should be completely rewritten to traverse all nodes below and find node with max linenumber+column number .

@Nimfadora , can you proceed further ?

FYI:

$ cat -n /home/rivanov/java/github/checkstyle/checkstyle/src/test/resources/com/puppycrawl/tools/checkstyle/checks/whitespace/nowhitespaceafter/InputNoWhitespaceAfterArrayDeclarations.java | grep -E "10|12"
    10	    String s [] = {}; //Incorrect
    12	    char[] c = {}; //Correct

$ java -jar checkstyle-8.3-SNAPSHOT-all.jar -t /home/rivanov/java/github/checkstyle/checkstyle/src/test/resources/com/puppycrawl/tools/checkstyle/checks/whitespace/nowhitespaceafter/InputNoWhitespaceAfterArrayDeclarations.java | grep "\[10\:"
    |--VARIABLE_DEF -> VARIABLE_DEF [10:13]
    |   |--MODIFIERS -> MODIFIERS [10:13]
    |   |--TYPE -> TYPE [10:13]
    |   |   `--ARRAY_DECLARATOR -> [ [10:13]
    |   |       |--IDENT -> String [10:4]
    |   |       `--RBRACK -> ] [10:14]
    |   |--IDENT -> s [10:11]
    |   |--ASSIGN -> = [10:16]
    |   |   `--ARRAY_INIT -> { [10:18]
    |   |       `--RCURLY -> } [10:19]
    |   `--SEMI -> ; [10:20]

$ java -jar checkstyle-8.3-SNAPSHOT-all.jar -t /home/rivanov/java/github/checkstyle/checkstyle/src/test/resources/com/puppycrawl/tools/checkstyle/checks/whitespace/nowhitespaceafter/InputNoWhitespaceAfterArrayDeclarations.java | grep "\[12\:"
    |--VARIABLE_DEF -> VARIABLE_DEF [12:8]
    |   |--MODIFIERS -> MODIFIERS [12:8]
    |   |--TYPE -> TYPE [12:8]
    |   |   `--ARRAY_DECLARATOR -> [ [12:8]
    |   |       |--LITERAL_CHAR -> char [12:4]
    |   |       `--RBRACK -> ] [12:9]
    |   |--IDENT -> c [12:11]
    |   |--ASSIGN -> = [12:13]
    |   |   `--ARRAY_INIT -> { [12:15]
    |   |       `--RCURLY -> } [12:16]
    |   `--SEMI -> ; [12:17]
Member

romani commented Oct 20, 2017

@Nimfadora ,

There is a problem with mutations in ParenPadCheck. We have such mutation, if I remove this line, all tests pass, regression pass.

please remove this line and prove it by regression - in separate PR.
I am ok to have regression in this Check, one day users will complain and we will have exact case.

ParenPadCheck ...... as nodes of first several types like CTOR_DEF, METHOD_DEF will never reach this method.

confirmed, please use reflection and pure-UT test method to cover mutation. It is not reasonable to make new set of tokens without this. Attention: each reflection usage should be explained by a comment - you have a reason. Please send this by separate PR.

Do not forget to change shippable.yml to remove grep .... --exclude=....

What about NoWhitespaceAfterCheck Even if I find exact number for the second condition

there is a good practical approach: if code is hard to cover - it highly likely is weird and should be rewritten. We have intension comment " //ident and lastTypeNode lay on one line", can we change code to be written differently ?

that code should be read like

            if (ident.getColumnNo() > ast.getColumnNo()
                || lastTypeNode.getColumnNo() > ident.getColumnNo()) {
                // it is modern Java style declaration like "String[] s = null;"
                previousElement = lastTypeNode;
            }
            else {
                // it is old C style declaration like "String s [] = null;"
                previousElement = ident;
            }

so code could be simplified as:

        //ident and lastTypeNode lay on one line
        else {
            if (ident.getColumnNo() > lastTypeNode.getColumnNo()) {
                // it is old C style declaration like "String s [] = null;"
                previousElement = ident;
            } else {
                // it is modern Java style declaration like "String[] s = null;"
                previousElement = lastTypeNode;
            }
        }

getTypeLastNode method should be completely rewritten to traverse all nodes below and find node with max linenumber+column number .

@Nimfadora , can you proceed further ?

FYI:

$ cat -n /home/rivanov/java/github/checkstyle/checkstyle/src/test/resources/com/puppycrawl/tools/checkstyle/checks/whitespace/nowhitespaceafter/InputNoWhitespaceAfterArrayDeclarations.java | grep -E "10|12"
    10	    String s [] = {}; //Incorrect
    12	    char[] c = {}; //Correct

$ java -jar checkstyle-8.3-SNAPSHOT-all.jar -t /home/rivanov/java/github/checkstyle/checkstyle/src/test/resources/com/puppycrawl/tools/checkstyle/checks/whitespace/nowhitespaceafter/InputNoWhitespaceAfterArrayDeclarations.java | grep "\[10\:"
    |--VARIABLE_DEF -> VARIABLE_DEF [10:13]
    |   |--MODIFIERS -> MODIFIERS [10:13]
    |   |--TYPE -> TYPE [10:13]
    |   |   `--ARRAY_DECLARATOR -> [ [10:13]
    |   |       |--IDENT -> String [10:4]
    |   |       `--RBRACK -> ] [10:14]
    |   |--IDENT -> s [10:11]
    |   |--ASSIGN -> = [10:16]
    |   |   `--ARRAY_INIT -> { [10:18]
    |   |       `--RCURLY -> } [10:19]
    |   `--SEMI -> ; [10:20]

$ java -jar checkstyle-8.3-SNAPSHOT-all.jar -t /home/rivanov/java/github/checkstyle/checkstyle/src/test/resources/com/puppycrawl/tools/checkstyle/checks/whitespace/nowhitespaceafter/InputNoWhitespaceAfterArrayDeclarations.java | grep "\[12\:"
    |--VARIABLE_DEF -> VARIABLE_DEF [12:8]
    |   |--MODIFIERS -> MODIFIERS [12:8]
    |   |--TYPE -> TYPE [12:8]
    |   |   `--ARRAY_DECLARATOR -> [ [12:8]
    |   |       |--LITERAL_CHAR -> char [12:4]
    |   |       `--RBRACK -> ] [12:9]
    |   |--IDENT -> c [12:11]
    |   |--ASSIGN -> = [12:13]
    |   |   `--ARRAY_INIT -> { [12:15]
    |   |       `--RCURLY -> } [12:16]
    |   `--SEMI -> ; [12:17]
@Nimfadora

This comment has been minimized.

Show comment
Hide comment
@Nimfadora

Nimfadora Oct 29, 2017

Contributor

can we change code to be written differently ?

@romani your example is a bit controversial:

getTypeLastNode method should return max line number+column number

so in this situation String s [] = null will be decomposed as:
ident = s
lastTypeNode= ]

ident < lastTypeNode

and it will not pass the condtiton ident.getColumnNo() > lastTypeNode.getColumnNo(). So either you have confused the comments or I didn't understand what you mean.

And also we should not forget about such case:
someStuff15 instanceof Object [] //incorrect 1
someStuff15 instanceof Object[] [] //incorrect 2
someStuff15 instanceof Object [] [] //incorrect 1,2 but we notice only the first mistake

If we changegetTypeLastNode as you propose, we will not notice the 1 at all.

I'm totally confused about this situation. And in any case we could not go far from comparisons where ">" and "<" are used, so pitest will still require some strong bound like "ident.getColumnNo() > lastTypeNode.getColumnNo() - minPossibleDistance", which is hard to determine.

Contributor

Nimfadora commented Oct 29, 2017

can we change code to be written differently ?

@romani your example is a bit controversial:

getTypeLastNode method should return max line number+column number

so in this situation String s [] = null will be decomposed as:
ident = s
lastTypeNode= ]

ident < lastTypeNode

and it will not pass the condtiton ident.getColumnNo() > lastTypeNode.getColumnNo(). So either you have confused the comments or I didn't understand what you mean.

And also we should not forget about such case:
someStuff15 instanceof Object [] //incorrect 1
someStuff15 instanceof Object[] [] //incorrect 2
someStuff15 instanceof Object [] [] //incorrect 1,2 but we notice only the first mistake

If we changegetTypeLastNode as you propose, we will not notice the 1 at all.

I'm totally confused about this situation. And in any case we could not go far from comparisons where ">" and "<" are used, so pitest will still require some strong bound like "ident.getColumnNo() > lastTypeNode.getColumnNo() - minPossibleDistance", which is hard to determine.

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Dec 10, 2017

Member

http://rveach.no-ip.org/checkstyle/regression/pitest-reports/8/com.puppycrawl.tools.checkstyle.checks.whitespace/NoWhitespaceAfterCheck.java.html#org.pitest.mutationtest.report.html.SourceFile@1fd577ca_393
http://rveach.no-ip.org/checkstyle/regression/pitest-reports/8/com.puppycrawl.tools.checkstyle.checks.whitespace/NoWhitespaceAfterCheck.java.html#org.pitest.mutationtest.report.html.SourceFile@1fd577ca_394

http://rveach.no-ip.org/checkstyle/regression/reports/140/
http://rveach.no-ip.org/checkstyle/regression/reports/141/

Regression has shown no differences for changing these remaining lines during mutation.
UTs show these lines can't be removed.
We must hit this using reflection or mocks.

ast is always ARRAY_DECLARATOR. parent is always TYPE.
ident is either null, IDENT or RPAREN.
lastTypeNode is either IDENT, GENERIC_END, LITERAL_INT, LITERAL_BYTE, etc...

To hit these lines in getPreviousNodeWithParentOfTypeAst,
ident can't be null, and must have the same line number as ast.
line 393, ident column number has to equal ast column number + 1.
line 394, lastTypeNode column number has to equal ident column number.

ident column number has to equal ast column number + 1.

This code is in the form []ident, the ending ] is why it can never be equal when +1. It is always atleast +2 away. I think we can get away with incrementing the code by 1 here again.

line 394, lastTypeNode column number has to equal ident column number.

This code only seems to be in the form ident instanceof type[], the [ is always quite a distance away from ident which is why we can never get equal.

Member

rnveach commented Dec 10, 2017

http://rveach.no-ip.org/checkstyle/regression/pitest-reports/8/com.puppycrawl.tools.checkstyle.checks.whitespace/NoWhitespaceAfterCheck.java.html#org.pitest.mutationtest.report.html.SourceFile@1fd577ca_393
http://rveach.no-ip.org/checkstyle/regression/pitest-reports/8/com.puppycrawl.tools.checkstyle.checks.whitespace/NoWhitespaceAfterCheck.java.html#org.pitest.mutationtest.report.html.SourceFile@1fd577ca_394

http://rveach.no-ip.org/checkstyle/regression/reports/140/
http://rveach.no-ip.org/checkstyle/regression/reports/141/

Regression has shown no differences for changing these remaining lines during mutation.
UTs show these lines can't be removed.
We must hit this using reflection or mocks.

ast is always ARRAY_DECLARATOR. parent is always TYPE.
ident is either null, IDENT or RPAREN.
lastTypeNode is either IDENT, GENERIC_END, LITERAL_INT, LITERAL_BYTE, etc...

To hit these lines in getPreviousNodeWithParentOfTypeAst,
ident can't be null, and must have the same line number as ast.
line 393, ident column number has to equal ast column number + 1.
line 394, lastTypeNode column number has to equal ident column number.

ident column number has to equal ast column number + 1.

This code is in the form []ident, the ending ] is why it can never be equal when +1. It is always atleast +2 away. I think we can get away with incrementing the code by 1 here again.

line 394, lastTypeNode column number has to equal ident column number.

This code only seems to be in the form ident instanceof type[], the [ is always quite a distance away from ident which is why we can never get equal.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Dec 12, 2017

Member

last fix is merged.

Member

romani commented Dec 12, 2017

last fix is merged.

@romani romani closed this Dec 12, 2017

timurt added a commit to timurt/checkstyle that referenced this issue Dec 19, 2017

timurt added a commit to timurt/checkstyle that referenced this issue Dec 19, 2017

timurt added a commit to timurt/checkstyle that referenced this issue Dec 19, 2017

timurt added a commit to timurt/checkstyle that referenced this issue Dec 19, 2017

@romani romani moved this from To Do to Done in Practice What You Preach Mar 11, 2018

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