openrewrite / openrewrite/rewrite-testing-frameworks

JUnit 4 to 5 migration discards the one-time setup a `TestSetup` decorator performed, breaking tests at run time

Open
#1,110 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

bug
Dominant language
Java
Stars
100
Forks
105
Avg merge
2h 25m
Merged PRs (30d)
9

Description

What happens

JUnit4to5Migration removes any static suite() method returning junit.framework.Test. When that
method returned a junit.extensions.TestSetup decorator, the removal also discards the class's
one-time setup and teardown, and no JUnit 5 lifecycle annotation takes their place.

Minimal reproduction

Before:

import junit.extensions.TestSetup;
import junit.framework.Test;
import junit.framework.TestCase;
import junit.framework.TestSuite;

public class MathTest extends TestCase {
    private static String resource;

    public static Test suite() {
        return new TestSetup(new TestSuite(MathTest.class)) {
            @Override
            protected void setUp() throws Exception {
                setUpOnce();
            }

            @Override
            protected void tearDown() throws Exception {
                tearDownOnce();
            }
        };
    }

    static void setUpOnce() throws Exception {
        resource = "open";
    }

    static void tearDownOnce() throws Exception {
        resource = null;
    }

    public void testResourceIsOpen() {
        assertEquals("open", resource);
    }
}

After org.openrewrite.java.testing.junit5.JUnit4to5Migration:

import org.junit.jupiter.api.Test;

import static org.junit.jupiter.api.Assertions.assertEquals;

public class MathTest {
    private static String resource;

    static void setUpOnce() throws Exception {
        resource = "open";
    }

    static void tearDownOnce() throws Exception {
        resource = null;
    }

    @Test
    public void testResourceIsOpen() {
        assertEquals("open", resource);
    }
}

Everything else is migrated correctly — superclass removed, @Test added, assertEquals
static-imported, JUnit 3 imports cleaned up. Only the fixture is wrong: setUpOnce and tearDownOnce
survive with no caller, so resource stays null and testResourceIsOpen fails.

The loss is larger when the decorator holds its statements inline rather than delegating to a named
method.

Before:

import junit.extensions.TestSetup;
import junit.framework.Test;
import junit.framework.TestCase;
import junit.framework.TestSuite;

public class MathTest extends TestCase {
    private static String resource;
    private static int counter;

    public static Test suite() {
        return new TestSetup(new TestSuite(MathTest.class)) {
            @Override
            protected void setUp() throws Exception {
                resource = "open";
                counter = 1;
                System.setProperty("mode", "test");
            }

            @Override
            protected void tearDown() throws Exception {
                resource = null;
                System.clearProperty("mode");
            }
        };
    }

    public void testResourceIsOpen() {
        assertEquals("open", resource);
        assertEquals(1, counter);
    }
}

After:

import org.junit.jupiter.api.Test;

import static org.junit.jupiter.api.Assertions.assertEquals;

public class MathTest {
    private static String resource;
    private static int counter;

    @Test
    public void testResourceIsOpen() {
        assertEquals("open", resource);
        assertEquals(1, counter);
    }
}

All five statements are gone, and unlike the first reproduction nothing is left behind to show that
the class ever had a fixture.

Why this matters

This is a silent behaviour change rather than a build break, so it is harder to catch than a compile
error. The output compiles, and CleanupJUnitImports removes the now-unused
junit.extensions.TestSetup import, so the diff reads as an ordinary tidy-up: JUnit 3 imports gone,
extends TestCase gone, suite() gone, @Test added. Nothing indicates that a fixture was dropped.

The failure appears only when the tests run, and some distance from the change. In a real codebase the
discarded statement is usually a client or connection, so what surfaces is a NullPointerException in
the test body:

java.lang.NullPointerException: Cannot invoke
"AlertServiceClient.getAlertsForCoverage(int)" because "AlertServiceTest._client" is null

The two shapes differ in how much evidence they leave behind:

Decorator body After migration
calls a named method, setUpOnce() method survives, now uncalled — some evidence remains
holds statements inline statements deleted — no evidence at all

TestSetup has an exact JUnit 5 counterpart. TestSetup.setUp runs once before the wrapped suite and
TestSetup.tearDown once after, which is @BeforeAll and @AfterAll, so the information needed to
migrate it is present in the source being removed.

Why the existing handling doesn't cover it

MigrateJUnitTestCase removes suite() on the strength of its signature alone, without looking at
what it returns:

if ("suite".equals(md.getSimpleName()) &&
    md.hasModifier(J.Modifier.Type.Static) &&
    md.getMethodType() != null &&
    (TypeUtils.isOfClassType(md.getMethodType().getReturnType(), "junit.framework.Test") ||
     TypeUtils.isOfClassType(md.getMethodType().getReturnType(), "junit.framework.TestSuite"))) {
    maybeRemoveImport("junit.framework.Test");
    maybeRemoveImport("junit.framework.TestSuite");
    return null;
}

That is correct for a plain new TestSuite(MathTest.class), which only enumerates tests JUnit 5
discovers by itself, and MigrateJUnitTestCaseTest.suiteMethodIsRemoved covers exactly that case:

public static Test suite() {
    return new TestSuite(AppTest.class);
}

There is nothing behind that suite() to lose, so removal is right and the test passes. TestSetup
appears nowhere in the test sources, and a decorator is the one case where suite() carries behaviour
rather than just listing tests.

A fix cannot simply keep suite(): JUnit 5 has no equivalent, and the method would not compile once
junit.framework.Test is gone. The decorator's setUp and tearDown bodies would need to move onto
the class as @BeforeAll and @AfterAll. Where the body is a single call to an existing static
method, annotating that method is enough; where it holds statements inline, they need a generated
static method. Both annotations require a static method unless the class is
@TestInstance(PER_CLASS), which matches TestSetup's own semantics.

Environment
  • rewrite-testing-frameworks 3.45.0
  • Recipe: org.openrewrite.java.testing.junit5.JUnit4to5Migration

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start in MigrateJUnitTestCase, where suite() is removed based only on its return type, and compare it with MigrateJUnitTestCaseTest.suiteMethodIsRemoved. Add focused coverage for TestSetup decorators with delegated and inline lifecycle bodies, then verify the migration preserves the one-time setup and teardown semantics through JUnit 5 lifecycle annotations.

Written by the indexing model from the issue text.

Assessment

Tech stack
java
Domain
testing
Issue type
Bug
Difficulty
5/5
Estimated time
Over a week
Activity status
Active
Clarity
Clearly specified
Newbie friendliness
48/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.