UpdateBeforeAfterAnnotations does not check for existing target annotations

[timo-abele]

What version of OpenRewrite are you using?

I was using the latest version as of 2024-02-14, I bet this hasn't been fixed in the meantime because it is an edge case.

How are you running OpenRewrite?

I am using the Maven plugin, and my project is a single module project.

What is the problem?

A test base class uses both JUnit 4 and Jupiter, as a consequence its setup method is annotated with both @Before and @BeforeEach. The recipe will now replace the old @Before with the already present @BeforeEach annotation causing

Duplicate annotation. The declaration of 'org.junit.jupiter.api.BeforeEach' does not have a valid java.lang.annotation.Repeatable annotation

...
class TestBase {
    @Before
    @BeforeEach
    void setup() {
      //...
    }
}

While rare this issue could pop up in all kinds of unexpected places, so it might be worth introducing something like MaybeChangeType that considers the already present annotations.

What is the full stack trace of any errors you encountered?

see above

Are you interested in contributing a fix to OpenRewrite?

No

[timtebeek]

Oh wow, hadn't ever seen that pattern indeed. It'll be caused by us changing the type of the existing Before annotations to the new BeforeEach annotations.

rewrite-testing-frameworks/src/main/java/org/openrewrite/java/testing/junit5/UpdateBeforeAfterAnnotations.java

Lines 55 to 58 in 41a01da

	doAfterVisit(new ChangeType("org.junit.Before", "org.junit.jupiter.api.BeforeEach", true).getVisitor());
	doAfterVisit(new ChangeType("org.junit.After", "org.junit.jupiter.api.AfterEach", true).getVisitor());
	doAfterVisit(new ChangeType("org.junit.BeforeClass", "org.junit.jupiter.api.BeforeAll", true).getVisitor());
	doAfterVisit(new ChangeType("org.junit.AfterClass", "org.junit.jupiter.api.AfterAll", true).getVisitor());

If we would want to support this we'd have to revise that logic to also look for existing target annotations and remove those. That's sure possible, but I'd expect that to be rare and perhaps then not immediately worth the trouble for a one time migration. Would you agree there?