Improve `JUnitMethodDeclaration` check #406

Stephan202 · 2022-12-11T06:03:11Z

Resolves issues found while testing Error Prone Support against Checkstyle (currently testing on this branch).

Suggested commit message:

Improve `JUnitMethodDeclaration` check (#406)

Implemented changes:
- Ignore method overrides even if not annotated with `@Override`.
- Don't rename methods to `true`, `false` or `null`.
- Don't rename methods to a name declared in a super type. This
  prevents e.g. renaming `testToString` to `toString`.

Stephan202 · 2022-12-11T07:22:40Z

There's no mutation testing report here, while #407, which I filed soon after this PR, has two (identical) reports for a single commit.

I looked into the issue a bit and sent the following email to @hcoles of Arcmutate/CDG (in response to an earlier email in which Henry gave us a heads-up about this issue possibly existing):

Hi Henry,

It appears (although this has not been confirmed) that the artifacts from one PR overwrote another.

I suspect we just hit this issue. I filed the following PRs in close succession:

Improve JUnitMethodDeclaration check #406

Improve IdentityConversion check #407

What happened next was that the latter PR received two mutation test report comments (both referencing code modified only in the latter PR), while the former PR did not receive any comment.

The two builds that generated the reports do have the correct zip file attached:

https://github.com/PicnicSupermarket/error-prone-support/actions/runs/3667668540

https://github.com/PicnicSupermarket/error-prone-support/actions/runs/3667677035

So the issue should then be with the jobs that update the PR, which is these two:

https://github.com/PicnicSupermarket/error-prone-support/actions/runs/3667685810

https://github.com/PicnicSupermarket/error-prone-support/actions/runs/3667690703

Checking the logs of these two runs, we see indeed that both specify:

(found) Run ID: 3667677035

This is incorrect for one of the two. Note also the check summary difference between the two PRs:

8 successful and 1 skipped checks

9 successful and 1 skipped checks

Searching around a bit, I wonder whether these links reference the same issue:

Report check item appears under the wrong workflow name ScaCap/action-surefire-report#39

https://github.com/orgs/community/discussions/14891

I likely won't have time in the near future to dive deeper into this issue. I hope this helps!

Cheers,
Stephan

github-actions · 2022-12-19T15:20:38Z

Surviving mutants in this change: 2
Killed mutants in this change: 29

class	surviving	killed
🧟`tech.picnic.errorprone.bugpatterns.util.JavaKeywords`	2	11
🎉`tech.picnic.errorprone.bugpatterns.JUnitMethodDeclaration`	0	18

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

Stephan202 · 2022-12-19T17:53:04Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/JavaKeywords.java

+        && !isReservedKeyword(str)
+        && !BOOLEAN_AND_NULL_LITERALS.contains(str)
+        && Character.isJavaIdentifierStart(str.codePointAt(0))
+        && str.codePoints().skip(1).allMatch(Character::isUnicodeIdentifierPart);


Pitest flags that mutants that remove .skip(1) aren't killed. Without assuming that isUnicodeIdentifierPart matches a superset of isJavaIdentifierStart I can't drop this operation. But that assumption does hold in practice, IIUC, so writing a test for it is hard/impossible. There's also a (minute) performance aspect. TL;DR: I don't see how I can avoid this Pitest warning.

Implemented changes: - Ignore method overrides even if not annotated `@Override`. - Don't rename methods to `true`, `false` or `null`. - Don't rename methods to a name declared in a super type. This prevents e.g. renaming `testToString` to `toString`.

rickie

Rebased and added a commit.

Some minor comments with questions mostly :).

Changes LGTM!

Made one tweak to suggested commit message.

rickie · 2022-12-19T15:08:51Z

error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/util/JavaKeywordsTest.java

+
+final class JavaKeywordsTest {
+  private static Stream<Arguments> isValidIdentifierTestCases() {
+    return Stream.of(


Suggested change

return Stream.of(

/* { str, expected } */

return Stream.of(

rickie · 2022-12-20T16:44:45Z

...r-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitMethodDeclaration.java

-              Matchers.not(hasModifier(Modifier.FINAL)),
-              Matchers.not(hasModifier(Modifier.PRIVATE)),
-              enclosingClass(hasModifier(Modifier.ABSTRACT))));
+      Sets.immutableEnumSet(Modifier.PRIVATE, Modifier.PROTECTED, Modifier.PUBLIC);


What is the reason for this change? :)

More efficient storage and retrieval, just like what we do here.

rickie · 2022-12-20T16:47:04Z

...r-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitMethodDeclaration.java

    }

    return Optional.empty();
  }

+  private static boolean isExistingMethodName(Type clazz, String name, VisitorState state) {
+    return ASTHelpers.matchingMethods(state.getName(name), x -> true, clazz, state.getTypes())


We could signal that it's unused by naming x unused. Not sure if we gain a lot there though.

Hmm, the scope is very small, so it's clear that the parameter is unused. Perhaps better would be s/x/method/, since it's a predicate over methods.

github-actions · 2022-12-21T10:16:46Z

Surviving mutants in this change: 2
Killed mutants in this change: 29

class	surviving	killed
🧟`tech.picnic.errorprone.bugpatterns.util.JavaKeywords`	2	11
🎉`tech.picnic.errorprone.bugpatterns.JUnitMethodDeclaration`	0	18

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

Stephan202 · 2022-12-21T18:38:10Z

Made one tweak to suggested commit message.

Hmm, I think it reads better without the word "with". Just like something "is annotated @Nullable". There should IMHO not be a "with" in that sentence.

(Need to leave train; will check rest later.)

Stephan202

Added a commit. Tnx for the reviews!

W.r.t. the commit message: can live with it, so merge as you see fit.

Stephan202 · 2022-12-21T19:12:30Z

...r-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitMethodDeclaration.java

-              Matchers.not(hasModifier(Modifier.FINAL)),
-              Matchers.not(hasModifier(Modifier.PRIVATE)),
-              enclosingClass(hasModifier(Modifier.ABSTRACT))));
+      Sets.immutableEnumSet(Modifier.PRIVATE, Modifier.PROTECTED, Modifier.PUBLIC);


More efficient storage and retrieval, just like what we do here.

Stephan202 · 2022-12-21T19:14:02Z

...r-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitMethodDeclaration.java

    }

    return Optional.empty();
  }

+  private static boolean isExistingMethodName(Type clazz, String name, VisitorState state) {
+    return ASTHelpers.matchingMethods(state.getName(name), x -> true, clazz, state.getTypes())


Hmm, the scope is very small, so it's clear that the parameter is unused. Perhaps better would be s/x/method/, since it's a predicate over methods.

github-actions · 2022-12-21T19:20:26Z

Surviving mutants in this change: 2
Killed mutants in this change: 29

class	surviving	killed
🧟`tech.picnic.errorprone.bugpatterns.util.JavaKeywords`	2	11
🎉`tech.picnic.errorprone.bugpatterns.JUnitMethodDeclaration`	0	18

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

Stephan202 added this to the 0.7.0 milestone Dec 11, 2022

Stephan202 requested review from oxkitsune and rickie December 11, 2022 06:03

rickie force-pushed the sschroevers/JUnitMethodDeclaration-improvements branch from 33ac331 to 6f4a212 Compare December 19, 2022 15:18

Stephan202 commented Dec 19, 2022

View reviewed changes

rickie added the improvement label Dec 20, 2022

Stephan202 and others added 2 commits December 21, 2022 11:13

Suggestions

7fa268f

rickie force-pushed the sschroevers/JUnitMethodDeclaration-improvements branch from 6f4a212 to 7fa268f Compare December 21, 2022 10:14

rickie approved these changes Dec 21, 2022

View reviewed changes

rickie added bug fix and removed improvement labels Dec 21, 2022

eric-picnic approved these changes Dec 21, 2022

View reviewed changes

Rename parameter

b353a53

Stephan202 commented Dec 21, 2022

View reviewed changes

rickie merged commit 6313bd5 into master Dec 22, 2022

rickie deleted the sschroevers/JUnitMethodDeclaration-improvements branch December 22, 2022 07:34

Stephan202 mentioned this pull request Jan 5, 2023

Upgrade pitest-maven-plugin 1.10.3 -> 1.10.4 #445

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve `JUnitMethodDeclaration` check #406

Improve `JUnitMethodDeclaration` check #406

Stephan202 commented Dec 11, 2022 •

edited by rickie

Loading

Stephan202 commented Dec 11, 2022

github-actions bot commented Dec 19, 2022

Stephan202 Dec 19, 2022

rickie left a comment

rickie Dec 19, 2022

rickie Dec 20, 2022

Stephan202 Dec 21, 2022

rickie Dec 20, 2022

Stephan202 Dec 21, 2022

github-actions bot commented Dec 21, 2022

Stephan202 commented Dec 21, 2022

Stephan202 left a comment

Stephan202 Dec 21, 2022

Stephan202 Dec 21, 2022

github-actions bot commented Dec 21, 2022

Improve JUnitMethodDeclaration check #406

Improve JUnitMethodDeclaration check #406

Conversation

Stephan202 commented Dec 11, 2022 • edited by rickie Loading

Stephan202 commented Dec 11, 2022

github-actions bot commented Dec 19, 2022

Choose a reason for hiding this comment

rickie left a 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

github-actions bot commented Dec 21, 2022

Stephan202 commented Dec 21, 2022

Stephan202 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Dec 21, 2022

Improve `JUnitMethodDeclaration` check #406

Improve `JUnitMethodDeclaration` check #406

Stephan202 commented Dec 11, 2022 •

edited by rickie

Loading