Issue #4394: increase coverage of pitest-checkstyle-api profile to 96% #4704

Nimfadora · 2017-07-12T14:33:03Z

pitest-checkstyle-api profile before changes

increased mutation threshold for pitest-checkstyle-api-filters profile by 8%
current threshold: 96%
execution time: 9:51 min

rnveach · 2017-07-12T14:36:35Z

src/main/java/com/puppycrawl/tools/checkstyle/api/DetailAST.java

@@ -227,11 +227,12 @@ public int getLineNo() {
            // with initialize(String text)
            resultNo = findLineNo(getFirstChild());

-            if (resultNo < 0) {
+            //line number cannot be less than 1
+            if (resultNo == -1) {


Why do we add comment here but no where else in this file?

I just forgot to remove this one.

Then please remove.

rnveach · 2017-07-12T14:36:49Z

src/main/java/com/puppycrawl/tools/checkstyle/api/AbstractLoader.java

@@ -49,12 +49,6 @@
 */
 public abstract class AbstractLoader
    extends DefaultHandler {
-    /** Feature that enables loading external DTD when loading XML files. */
-    private static final String LOAD_EXTERNAL_DTD =
-        "http://apache.org/xml/features/nonvalidating/load-external-dtd";


@romani Please review these removals.

Those features are enabled by default. I moved them to the tests - to always be sure that they are still enabled.

@Nimfadora This is not true. Look at blame for these lines and associated issue.
https://github.com/checkstyle/checkstyle/blame/154f120d7b71c30ec64f3ef801fb080a24a21c4a/src/main/java/com/puppycrawl/tools/checkstyle/api/AbstractLoader.java#L52
#3605

This removal must be reversed.

@Nimfadora ,
Unfortunately, this is weird feature request from googlers , please see commit and related issue #3605 .
How can we suppress pitest there or any other workaround (I am ok to move that code to special method addFeauresForVerySecureJavaInstallations and suppress pitest on this method)?

@romani it's kinda troublesome. I can exclude addFeauresForVerySecureJavaInstallations from being mutated, but I cannot suppress mutation of the call to this method. avoidCallTo in PIT config didn't help, cause it allows only class names as params.
I can pull this method to the separate class, eg AbstractLoaderUtils, and this helps. Is it fine or an overkill?

please try to do this by means of inner class in AbstractLoader, if it does not help ... ok to make separate top level class FeauresForVerySecureJavaInstallations in javadoc make it clear that it is result of conflict of pitest and user requirements.

rnveach · 2017-07-12T14:38:47Z

src/test/java/com/puppycrawl/tools/checkstyle/api/AbstractCheckTest.java

+        check.log(0, "key", "args");
+        final Collection<LocalizedMessage> messages =
+                (Collection<LocalizedMessage>) Whitebox.getInternalState(check, "messages");
+        check.clearMessages();


Can we add an assertion right before this that messages' size isn't 0?

rnveach · 2017-07-12T14:40:20Z

src/test/java/com/puppycrawl/tools/checkstyle/api/DetailASTTest.java

+            final BitSet branchTokenTypes = Whitebox.invokeMethod(parent, "getBranchTokenTypes");
+            method.accept(null);
+            final BitSet branchTokenTypes2 = Whitebox.invokeMethod(parent, "getBranchTokenTypes");
+            assertNotSame(branchTokenTypes, branchTokenTypes2);


We can't do more than notSame? It makes this a vague test.

I'm not really sure how. BitSets actually are equals each time. By this test, I wanted to ensure that each call to add/set sibling/child methods force bitset to be recalculated.

Can we then assert the resulting values are the same?
NotSame atleast verifies instance is different.

rnveach · 2017-07-12T14:43:26Z

src/test/java/com/puppycrawl/tools/checkstyle/api/FullIdentTest.java

+        testNonValidCoordinates(0, 0, "MyTest.MyTestik[15x14]");
+    }
+
+    private static void testNonValidCoordinates(int columnNo, int lineNo, String expected) {


Non-test methods shouldn't start with test. Rename test to validate.
Also romani doesn't like assertions outside of test method.

romani

items to improve:

romani · 2017-07-12T16:10:59Z

src/main/java/com/puppycrawl/tools/checkstyle/api/AbstractLoader.java

@@ -49,12 +49,6 @@
 */
 public abstract class AbstractLoader
    extends DefaultHandler {
-    /** Feature that enables loading external DTD when loading XML files. */
-    private static final String LOAD_EXTERNAL_DTD =
-        "http://apache.org/xml/features/nonvalidating/load-external-dtd";


@Nimfadora ,
Unfortunately, this is weird feature request from googlers , please see commit and related issue #3605 .
How can we suppress pitest there or any other workaround (I am ok to move that code to special method addFeauresForVerySecureJavaInstallations and suppress pitest on this method)?

Nimfadora · 2017-07-12T18:40:31Z

Build failed with
[testClass=com.puppycrawl.tools.checkstyle.api.FileTextTest, name=testLineColumnAfterCopyConstructor(com.puppycrawl.tools.checkstyle.api.FileTextTest)] did not pass without mutation.
This test definitly passes localy. What have I done wrong?

rnveach · 2017-07-12T21:13:28Z

This test definitly passes localy. What have I done wrong?

First thing that comes to mind is make sure you are up to date with master.
I cannot look into it right now.

romani · 2017-07-12T21:26:04Z

@Nimfadora , on my local for your branch the same on Travis:

$ mvn test
......
[ERROR] Failures: 
[ERROR]   FileTextTest.testLineColumnAfterCopyConstructor:92 expected:<44> but was:<46>

please recheck that tests are passing before pitest execution as it heavily affect pitest.

…file to 96%

codecov-io · 2017-07-14T21:24:21Z

Codecov Report

Merging #4704 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff           @@
##           master   #4704   +/-   ##
======================================
  Coverage     100%    100%           
======================================
  Files         287     287           
  Lines       15386   15388    +2     
  Branches     3501    3501           
======================================
+ Hits        15386   15388    +2

Impacted Files	Coverage Δ
...com/puppycrawl/tools/checkstyle/api/DetailAST.java	`100% <100%> (ø)`	⬆️
.../com/puppycrawl/tools/checkstyle/api/FileText.java	`100% <100%> (ø)`	⬆️
...ools/checkstyle/api/AbstractViolationReporter.java	`100% <100%> (ø)`	⬆️
...uppycrawl/tools/checkstyle/api/AbstractLoader.java	`100% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 66d866e...89c779c. Read the comment docs.

romani

as Shippable pas , we could merge

rnveach reviewed Jul 12, 2017

View reviewed changes

romani requested changes Jul 12, 2017

View reviewed changes

Issue checkstyle#4394: increase coverage of pitest-checkstyle-api pro…

89c779c

…file to 96%

romani approved these changes Jul 14, 2017

View reviewed changes

romani merged commit 94df4ad into checkstyle:master Jul 14, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue #4394: increase coverage of pitest-checkstyle-api profile to 96% #4704

Issue #4394: increase coverage of pitest-checkstyle-api profile to 96% #4704

Nimfadora commented Jul 12, 2017

rnveach Jul 12, 2017

Nimfadora Jul 12, 2017

rnveach Jul 12, 2017

Nimfadora Jul 14, 2017

rnveach Jul 12, 2017

Nimfadora Jul 12, 2017

rnveach Jul 12, 2017

romani Jul 12, 2017

Nimfadora Jul 12, 2017 •

edited

romani Jul 12, 2017

Nimfadora Jul 14, 2017

rnveach Jul 12, 2017

Nimfadora Jul 14, 2017

rnveach Jul 12, 2017

Nimfadora Jul 12, 2017

rnveach Jul 12, 2017

Nimfadora Jul 14, 2017

rnveach Jul 12, 2017

Nimfadora Jul 14, 2017

romani left a comment

romani Jul 12, 2017

Nimfadora commented Jul 12, 2017

rnveach commented Jul 12, 2017

romani commented Jul 12, 2017

codecov-io commented Jul 14, 2017 •

edited

romani left a comment

Issue #4394: increase coverage of pitest-checkstyle-api profile to 96% #4704

Issue #4394: increase coverage of pitest-checkstyle-api profile to 96% #4704

Conversation

Nimfadora commented Jul 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nimfadora Jul 12, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

romani left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nimfadora commented Jul 12, 2017

rnveach commented Jul 12, 2017

romani commented Jul 12, 2017

codecov-io commented Jul 14, 2017 • edited

Codecov Report

romani left a comment

Choose a reason for hiding this comment

Nimfadora Jul 12, 2017 •

edited

codecov-io commented Jul 14, 2017 •

edited