openrewrite / rewrite-testing-frameworks

OpenRewrite recipes that perform common Java testing migration tasks.
Apache License 2.0
77 stars 73 forks source link

Handle multiple blocks in JMockit Expectations #596

Closed yurii-yu closed 2 months ago

yurii-yu commented 2 months ago

What's changed?

Cope with a corner case in refactoring JMockit Expectations. Sometimes programmers will use more than one curly braces in one Expectation block. This looks bizarre and confusing, but it is syntactically correct. It will be ideal if such code can also be refactored automatically.

usually people write code like this:

new Expectations() {{
    MethodComparatorTest.this.method.getName(); result = "NoTest";
    MethodComparatorTest.this.method2.getName(); result = "Test";
}};

while some programmers prefer grouping related statements with redundant curly braces

new Expectations() {
    {
        MethodComparatorTest.this.method.getName();
        this.result = "NoTest";
    }
    {
        MethodComparatorTest.this.method2.getName();
        this.result = "Test";
    }
};

I made changes in two places.

On line 39 in JMockitUtils.java, this criterion need to be changed, or else OpenRewrite will just ignore such blocks.

if (nc.getBody() == null || nc.getBody().getStatements().size() != 1)

On line 53 in SetupStatementsRewriter.java, this rewrite logic need to be able to loop over all statements, instead of only the first one, in the Expectations block.

J.Block expectationsBlock = (J.Block) nc.getBody().getStatements().get(0);

// statement needs to be moved directly before expectations class instantiation
JavaCoordinates coordinates = nc.getCoordinates().before();
List<Statement> newExpectationsBlockStatements = new ArrayList<>();
for (Statement expectationStatement : expectationsBlock.getStatements()) {
    if (!isSetupStatement(expectationStatement, spies)) {
        newExpectationsBlockStatements.add(expectationStatement);
        continue;
    }
    rewriteBodyStatement(expectationStatement, coordinates);
    // subsequent setup statements are moved in order
    coordinates = expectationStatement.getCoordinates().after();
}
// the new expectations block has the setup statements removed
J.Block newExpectationsBlock = expectationsBlock.withStatements(newExpectationsBlockStatements);
nc = nc.withBody(nc.getBody().withStatements(Collections.singletonList(newExpectationsBlock)));

I also added one test case in JMockitExpectationsToMockitoTest.java

What's your motivation?

We encountered problem in dealing with several legacy projects. Unfortunately, OpenWrite just kept most of those test cases untouched. It took us a couple of days to locate the problem. Benefited a lot from the OpenRewrite project previously, we believe that more people and more legacy projects should be able to take advantage of it.

Anything in particular you'd like reviewers to focus on?

I read the best practice so I kept the LST untouched. Moreover, I tried my best not to affect the previous logic, so I limit my changes to "curly brace blocks in one Expectations" specifically. The change has been tested by the entire test suite. As far as I can see, everything works fine.

Have you considered any alternatives or workarounds?

Maybe changing the LST is a better solution, but I do not think it is worthy since we only need to handle a rare case. Users can also use a script to remove the curly braces before applying this recipe, but that is an invasive way.

Checklist

yurii-yu commented 2 months ago

@timtebeek Hello Amigo. I saw the latest commit from you. Those changes do make sense. You are so nice and so patient, thank you.

timtebeek commented 2 months ago

@timtebeek Hello Amigo. I saw the latest commit from you. Those changes do make sense. You are so nice and so patient, thank you.

Thanks so much! You're welcome; glad to have you contribute back to the project. :)

yurii-yu commented 2 months ago

@timtebeek Hello I am back. I encountered similar problem, when dealing with multiple blocks in one Verifications block.

new Verifications() {
    {
    myObject.wait(anyLong, anyInt);
    times = 3;
    }
    {
    myObject.wait(anyLong, anyInt);
    times = 4;
    }
};

I will create another pull-request for that in the next couple of days. Have a nice weekend.

timtebeek commented 2 months ago

I will create another pull-request for that in the next couple of days. Have a nice weekend.

Awesome, thanks! Looking forward to it. Enjoy your weekend as well. :)