Introduce BugPattern for removing duplicate StepVerifier#expectNext calls
#443
Conversation
|
Sometimes it helps to visually review the PR, so here's a little illustration of this PR's contents: |
|
Nice parting gift @SimonBaars! 😄 W.r.t. the build failures: Running Shame that the various I'll let Lvl. boss @rickie do the first review. 🤣 |
rickie
changed the title
expectNext callsStepVerifier#expectNext calls
rickie
changed the title
StepVerifier#expectNext callsStepVerifier#expectNext calls
|
Nice job @SimonBaars! It was fun to pair up on this a few times! (Still sad to see you go man 😢) Currently review backlog is a bit full, so it'll probably be next week before I can do a proper deep-dive 😉. |
You're beginning to turn into a manager! |
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
There was a problem hiding this comment.
Nice meme haha, I did an initial review where I first skimmed over the whole setup and provided feedback. Sorry for not getting back earlier to this. The Error Prone Support review backlog is quite big 😅.
Couldn't apply all feedback myself so leaving some open points for you as well 😉. Hope you don't mind :).
There are some interesting concepts in this check! Cool stuff!
EDIT: Oh and added a commit.
| linkType = CUSTOM, | ||
| severity = SUGGESTION, | ||
| tags = SIMPLIFICATION) | ||
| public final class StepVerifierDuplicateExpectNext extends BugChecker |
There was a problem hiding this comment.
Usually we don't add in the name the thing that is "wrong" or we are flagging. In this case StepVerifierExpectNext or StepVerifierExpectNextUsage would be more in-line with the rest. Based on other BugCheckers I think Usage suffix is a must haha.
...ib/src/test/java/tech/picnic/errorprone/bugpatterns/StepVerifierDuplicateExpectNextTest.java
Outdated
Show resolved
Hide resolved
| import com.google.errorprone.BugCheckerRefactoringTestHelper.TestMode; | ||
| import org.junit.jupiter.api.Test; | ||
|
|
||
| final class StepVerifierDuplicateExpectNextTest { |
There was a problem hiding this comment.
In this test I'm missing the CompilationTestHelper. In almost all test classes from BugCheckers we have two kind of tests.
CompilationTestHelper: This test contains almost all positive and negative cases (we usually refer to this as theidentificationtest).BugCheckerRefactoringTestHelper: Contains usually a subset of the examples used in theCompilationTestHelpertest to show that actually rewriting stuff works as intended. Especially with the use ofTestMode.TEXT_MATCHwe enforce that it is correct character for character :). (We usually refer to this as thereplacementtest)
Usually we list all possible cases in the identification test, see the FluxFlatMapUsageTest. For the replacement test we show a subset of the cases used in the identification test to show that the suggested fix is applied in the correct way. See the same test class as an example.
So for example the dontRefactorSingleCall and dontRefactorParent nicely fit in identfication.
| } | ||
|
|
||
| @Test | ||
| void dontRefactorParent() { |
There was a problem hiding this comment.
As mentioned, should go in the identification test. Same as above. Other way of writing this would be to drop the addOutputLines and replace it with expectUnchanged().doTest().
...ontrib/src/main/java/tech/picnic/errorprone/bugpatterns/StepVerifierDuplicateExpectNext.java
Outdated
Show resolved
Hide resolved
...ontrib/src/main/java/tech/picnic/errorprone/bugpatterns/StepVerifierDuplicateExpectNext.java
Outdated
Show resolved
Hide resolved
| private static Optional<MethodInvocationTree> getChild(VisitorState state, int skip) { | ||
| int startPos = ((JCTree) state.getPath().getLeaf()).pos; | ||
| return StreamSupport.stream(state.getPath().spliterator(), /* parallel= */ false) | ||
| .skip(skip) |
There was a problem hiding this comment.
Maybe we can improve the name for skip variable to make it clearer what is happening 👀?
| return Description.NO_MATCH; | ||
| } | ||
|
|
||
| String newArgument = newArgs.stream().map(Object::toString).collect(joining(", ")); |
There was a problem hiding this comment.
This could also be a little more descriptive. Currently no good ideas, so leaving it open.
| // If the parent matches, this node will be considered when the parent parses its children, so | ||
| // we don't match it. | ||
| if (!STEP_EXPECTNEXT.matches(tree, state) | ||
| || getParent(tree).filter(t -> STEP_EXPECTNEXT.matches(t, state)).isPresent()) { |
There was a problem hiding this comment.
getParent does more than getting the parent. If it only got the parent it would do (probably):
state.getPath().getParentPath() with optionally using getLeaf(). So a suffix about what it does / what it gets would be nice :).
I will dive into this next time; but I suspect there is a different way of doing if the "parent" / "previous" statement is also a expectNext call 🤔.
I might be rambling but maybe something like: "Get the enclosing tree and see if it matches the STEP_EXPECT_NEXT".
Other option would be to also check here that there is indeed adjacent expectNext that we want to incorporate into the current one. That would allow for quite a fast return instead of going over some of the other statements. Also would allow us to drop statements like this:
if (newArgs.isEmpty()) {
return Description.NO_MATCH;
}
|
|
||
| String newArgument = newArgs.stream().map(Object::toString).collect(joining(", ")); | ||
| List<? extends ExpressionTree> myArgs = tree.getArguments(); | ||
| SuggestedFix.Builder argumentsFix = |
There was a problem hiding this comment.
matchMethodInvocation method is maybe a bit too full. Already made a comment about the middle part. Maybe we can extract the Description related things to a method like we do in some other places. Look for the regex: try.*\( to see some methods that return either a Fix or Description for some examples.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
|
@rickie Hey, sorry, I'm not following this thread very actively. |
|
There are some open points that I haven't addressed yet. If you want, you can take a look there 😄. |
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
Woowee! That was a freaking mess.
At first I thought I could easily solve this with a little Refaster pattern. Boy was I wrong. So many edge cases, some many little thingies. So I went down the Error Prone rabbit hole, and boi was it deep. Who knew simply traversing nodes in an AST could be such a mess? Hell, even internal functions of Error Prone (yes I'm talking about you
ASTHelpers.findEnclosingNode) have infinite while loops in them. BWAAAAH!Anyway, it's 1AM in the night and I think I have a fully working version. As in, I made all the tests I could think of and they pass. Now it's up to the Lvl. 200 mafia bosses @rickie and crew to poke some holes in this dumpster fire of a bugpattern.
To explain a little bit what is going on:
I do a little for-loop where I step to all the other method invocations. The smart logic here is
getChild(), which jumpsnsteps over the next few nodes (child nodes) to find the nextexpectNextcall. By jumping 2 steps at a time, we visit all the method calls. Other types of nodes that it may visit should be eliminated either by the matcher or by the position check.Also I miss you 😢😢😢😢😢😢❤️