From 38c0a24ccc0619da3c03bb9d6cbcd85501f4dd80 Mon Sep 17 00:00:00 2001 From: Gonzalo Ortiz <1913993+gortiz@users.noreply.github.com> Date: Tue, 16 Jun 2026 16:11:05 +0200 Subject: [PATCH] Run tests on the JUnit Platform (TestNG + Jupiter engines) Move the test build onto the JUnit Platform so new JUnit 5 (Jupiter) tests can be written while the existing ~10k TestNG tests keep running unchanged via the TestNG engine for the JUnit Platform (org.junit.support:testng-engine). - Import the JUnit BOM (unified 6.x), add testng-engine, junit-jupiter and junit-platform-launcher as inherited test dependencies, and drop the surefire-testng provider so Surefire auto-selects the junit-platform provider. The launcher is on the test classpath so it stays version-aligned with the engines in the forked JVM. - The engine does not support testng.xml suite files. Replace the controller statefull/stateless split and the custom-cluster integration suite with JUnit Platform tag / include filters (TestNG groups map to platform tags). Use reuseForks=true on the controller executions to keep the shared-cluster setup that the single-JVM suites relied on; remove the now-dead ControllerTestSetup (@BeforeGroups/@AfterGroups, no @Test). - Add a Jupiter example test (BooleanUtilsTest) and docs/junit5-migration.md. --- docs/junit-migration.md | 82 +++++++++++++++++++ pinot-controller/pom.xml | 41 ++++++++-- .../pinot/controller/ControllerTestSetup.java | 53 ------------ .../controller/helix/ControllerTest.java | 6 +- pinot-controller/testng-statefull.xml | 39 --------- pinot-controller/testng-stateless.xml | 41 ---------- pinot-integration-tests/pom.xml | 10 ++- .../custom-cluster-integration-test-suite.xml | 28 ------- .../pinot/spi/utils/BooleanUtilsTest.java | 77 +++++++++++++++++ pom.xml | 62 +++++++++++--- 10 files changed, 254 insertions(+), 185 deletions(-) create mode 100644 docs/junit-migration.md delete mode 100644 pinot-controller/src/test/java/org/apache/pinot/controller/ControllerTestSetup.java delete mode 100644 pinot-controller/testng-statefull.xml delete mode 100644 pinot-controller/testng-stateless.xml delete mode 100644 pinot-integration-tests/src/test/resources/custom-cluster-integration-test-suite.xml create mode 100644 pinot-spi/src/test/java/org/apache/pinot/spi/utils/BooleanUtilsTest.java diff --git a/docs/junit-migration.md b/docs/junit-migration.md new file mode 100644 index 000000000000..1dd52759c959 --- /dev/null +++ b/docs/junit-migration.md @@ -0,0 +1,82 @@ + + +# JUnit (Jupiter) in Apache Pinot + +Pinot runs its tests on the **JUnit Platform**. The platform is engine-pluggable, and +Pinot enables two engines at once: + +- **JUnit Jupiter** — runs new tests written with the JUnit Jupiter API. Pinot pins the + **JUnit 6** BOM; "JUnit 5" is the same Jupiter programming model under the pre-unified + version numbers. +- **TestNG engine** (`org.junit.support:testng-engine`) — runs the existing TestNG suite + unchanged, analogous to how JUnit Vintage runs JUnit 4 tests. + +Both engines run in a single Surefire execution (Surefire's `junit-platform` provider). +**You do not need to migrate existing TestNG tests.** They keep their TestNG annotations +and `org.testng.Assert` assertions and run as-is. Write *new* tests in JUnit Jupiter. + +## How it is wired (root `pom.xml`) + +- The JUnit BOM (`org.junit:junit-bom`, unified version since JUnit 6) is imported in + `dependencyManagement`; `junit-jupiter`, `testng-engine`, and `junit-platform-launcher` + are inherited by every module as test-scoped dependencies. +- Surefire has **no explicit provider** dependency — it auto-selects the `junit-platform` + provider because a JUnit Platform engine is on the test classpath. The launcher is on the + test classpath so it stays version-aligned with the engines inside the forked JVM. +- TestNG `@Test(groups = "...")` are exposed to the platform as **tags**. Group-based + selection therefore uses Surefire `` / `` (e.g. the controller + module splits the `stateless` group into a separate execution). `testng.xml` suite files + are **not** supported by the engine and are not used. + +## Writing a new Jupiter test + +Use the `org.junit.jupiter.api` API. Reference example: +`pinot-spi/src/test/java/org/apache/pinot/spi/utils/BooleanUtilsTest.java`. + +| Need | TestNG | JUnit Jupiter | +|------|--------|---------------| +| Test method | `@Test` (`org.testng.annotations`) | `@Test` (`org.junit.jupiter.api`) | +| Setup/teardown per method | `@BeforeMethod` / `@AfterMethod` | `@BeforeEach` / `@AfterEach` | +| Setup/teardown per class | `@BeforeClass` / `@AfterClass` | `@BeforeAll` / `@AfterAll` (static) | +| Data-driven | `@DataProvider` + `dataProvider=` | `@ParameterizedTest` + `@ValueSource` / `@MethodSource` / `@CsvSource` | +| Expected exception | `@Test(expectedExceptions = X.class)` | `assertThrows(X.class, () -> ...)` | +| Disable | `@Test(enabled = false)` | `@Disabled` | +| Order | `@Test(priority = n)` | `@TestMethodOrder` + `@Order(n)` | +| Group/tag | `@Test(groups = "g")` | `@Tag("g")` | +| Conditional skip | `throw new SkipException(...)` | `Assumptions.assumeTrue(...)` | +| Mockito | `MockitoAnnotations.openMocks` | `@ExtendWith(MockitoExtension.class)` (add `org.mockito:mockito-junit-jupiter`) | + +Conventions: +- Prefer **AssertJ** (`assertThat(actual)...`, already available) or + `org.junit.jupiter.api.Assertions` for new tests. +- Do **not** mix TestNG and Jupiter annotations in the same class. + +### One footnote on assertions (only matters if you *rewrite* a TestNG test) + +JUnit's `assertEquals(expected, actual)` takes arguments in the **opposite order** from +TestNG's `assertEquals(actual, expected)`. This is irrelevant for tests left as TestNG +(they keep TestNG's `Assert`). It only matters when converting a test to Jupiter — flip the +argument order, or switch to AssertJ which sidesteps the ambiguity. + +## Optionally converting a module to Jupiter + +Converting is opt-in and should be done per-module in its own PR. Mechanics: swap the +annotations and assertions per the table above, mind the `assertEquals` arg order, and drop +the module's own `org.testng:testng` dependency once no TestNG test remains in it. diff --git a/pinot-controller/pom.xml b/pinot-controller/pom.xml index 3fa8183c99b2..9423e7e21eb1 100644 --- a/pinot-controller/pom.xml +++ b/pinot-controller/pom.xml @@ -171,12 +171,41 @@ org.apache.maven.plugins maven-surefire-plugin - - - testng-statefull.xml - testng-stateless.xml - - + + + + + default-test + + stateless + true + + + + + stateless + test + + test + + + stateless + true + + + diff --git a/pinot-controller/src/test/java/org/apache/pinot/controller/ControllerTestSetup.java b/pinot-controller/src/test/java/org/apache/pinot/controller/ControllerTestSetup.java deleted file mode 100644 index 7d39997f9799..000000000000 --- a/pinot-controller/src/test/java/org/apache/pinot/controller/ControllerTestSetup.java +++ /dev/null @@ -1,53 +0,0 @@ -/** - * Licensed to the Apache Software Foundation (ASF) under one - * or more contributor license agreements. See the NOTICE file - * distributed with this work for additional information - * regarding copyright ownership. The ASF licenses this file - * to you under the Apache License, Version 2.0 (the - * "License"); you may not use this file except in compliance - * with the License. You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, - * software distributed under the License is distributed on an - * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY - * KIND, either express or implied. See the License for the - * specific language governing permissions and limitations - * under the License. - */ -package org.apache.pinot.controller; - -import org.apache.pinot.controller.helix.ControllerTest; -import org.testng.annotations.AfterGroups; -import org.testng.annotations.BeforeGroups; - - -/** - * All test cases in {@link org.apache.pinot.controller} package are run as part a TestNG group (see testng_*.xml). - * This helps to setup (see {@link #setUpGroup()} and tear down (see {@link #tearDownGroup()} the shared state before - * and after all tests are run. Each test case class should implement a @BeforeClass method, which would call - * @link ControllerTest.getInstance()#validate()} method to validate shared state. Each test case class should also - * implement @AfterClass method where {@link ControllerTest#cleanup()} would be called to cleanup shared state. - * Additional cleanup may be needed depending upon test functionality. - */ -public class ControllerTestSetup { - /** - * TestNG will run this method once before all the test cases are run. We use this method to initialize - * common state for all the test cases. - */ - @BeforeGroups - public void setUpGroup() - throws Exception { - ControllerTest.getInstance().startSharedTestSetup(); - } - - /** - * TestNG will run this method once after all the test cases have run. We use this method to de-initialize - * common state used by all the test cases. - */ - @AfterGroups - public void tearDownGroup() { - ControllerTest.getInstance().stopSharedTestSetup(); - } -} diff --git a/pinot-controller/src/test/java/org/apache/pinot/controller/helix/ControllerTest.java b/pinot-controller/src/test/java/org/apache/pinot/controller/helix/ControllerTest.java index abccf734bc68..ae5f9e5738c9 100644 --- a/pinot-controller/src/test/java/org/apache/pinot/controller/helix/ControllerTest.java +++ b/pinot-controller/src/test/java/org/apache/pinot/controller/helix/ControllerTest.java @@ -1611,9 +1611,9 @@ public void checkNumOnlineInstancesFromExternalView(String resourceName, int exp public void setupSharedStateAndValidate() throws Exception { if (_zookeeperInstance == null || _helixResourceManager == null) { - // this is expected to happen only when running a single test case outside testNG group, i.e. when test - // cases are run one at a time within IntelliJ or through maven command line. When running under a testNG - // group, state will have already been setup by @BeforeGroups method in ControllerTestSetup. + // First stateful test class in the forked JVM starts the shared cluster; later classes reuse it (the + // surefire "default-test" execution runs the stateful set with reuseForks=true, so they share a JVM). + // This branch also covers running a single class in isolation (e.g. from an IDE or a -Dtest run). startSharedTestSetup(); } else { // Ensure the shared cluster starts clean between test classes. diff --git a/pinot-controller/testng-statefull.xml b/pinot-controller/testng-statefull.xml deleted file mode 100644 index fc61e096a0dd..000000000000 --- a/pinot-controller/testng-statefull.xml +++ /dev/null @@ -1,39 +0,0 @@ - - - - - - - - - - - - - - - - diff --git a/pinot-controller/testng-stateless.xml b/pinot-controller/testng-stateless.xml deleted file mode 100644 index ce6e14e4e69d..000000000000 --- a/pinot-controller/testng-stateless.xml +++ /dev/null @@ -1,41 +0,0 @@ - - - - - - - - - - - - - - - - diff --git a/pinot-integration-tests/pom.xml b/pinot-integration-tests/pom.xml index 761513d83eee..e16bc37ed9b5 100644 --- a/pinot-integration-tests/pom.xml +++ b/pinot-integration-tests/pom.xml @@ -151,11 +151,15 @@ org.apache.maven.plugins maven-surefire-plugin + false - - src/test/resources/custom-cluster-integration-test-suite.xml - + + **/integration/tests/custom/*Test.java + diff --git a/pinot-integration-tests/src/test/resources/custom-cluster-integration-test-suite.xml b/pinot-integration-tests/src/test/resources/custom-cluster-integration-test-suite.xml deleted file mode 100644 index f00fbd9ad2bd..000000000000 --- a/pinot-integration-tests/src/test/resources/custom-cluster-integration-test-suite.xml +++ /dev/null @@ -1,28 +0,0 @@ - - - - - - - - - diff --git a/pinot-spi/src/test/java/org/apache/pinot/spi/utils/BooleanUtilsTest.java b/pinot-spi/src/test/java/org/apache/pinot/spi/utils/BooleanUtilsTest.java new file mode 100644 index 000000000000..d536865ca980 --- /dev/null +++ b/pinot-spi/src/test/java/org/apache/pinot/spi/utils/BooleanUtilsTest.java @@ -0,0 +1,77 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.pinot.spi.utils; + +import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; + + +/** + * JUnit 5 (Jupiter) test, written to run on the JUnit Platform alongside the module's + * existing TestNG tests (which run via the testng-engine). It also serves as the reference + * example for new Jupiter tests: {@link ParameterizedTest} replaces TestNG's {@code @DataProvider}, + * and {@link org.junit.jupiter.api.Assertions#assertThrows} replaces {@code expectedExceptions}. + */ +public class BooleanUtilsTest { + + @ParameterizedTest + @ValueSource(strings = {"true", "TRUE", "True", "1"}) + void truthyStringsConvertToTrue(String value) { + assertTrue(BooleanUtils.toBoolean(value)); + assertEquals(BooleanUtils.INTERNAL_TRUE, BooleanUtils.toInt(value)); + } + + @ParameterizedTest + @ValueSource(strings = {"false", "FALSE", "0", "yes", "", "2"}) + void nonTruthyStringsConvertToFalse(String value) { + assertFalse(BooleanUtils.toBoolean(value)); + assertEquals(BooleanUtils.INTERNAL_FALSE, BooleanUtils.toInt(value)); + } + + @Test + void objectConversionCoversSupportedTypes() { + assertFalse(BooleanUtils.toBoolean((Object) null)); + assertTrue(BooleanUtils.toBoolean((Object) "1")); + assertTrue(BooleanUtils.toBoolean((Object) Integer.valueOf(BooleanUtils.INTERNAL_TRUE))); + assertFalse(BooleanUtils.toBoolean((Object) Integer.valueOf(BooleanUtils.INTERNAL_FALSE))); + assertTrue(BooleanUtils.toBoolean((Object) Boolean.TRUE)); + } + + @Test + void unsupportedObjectTypeThrows() { + // Not a String/Number/Boolean -> IllegalArgumentException (a Double would be treated as a Number). + assertThrows(IllegalArgumentException.class, () -> BooleanUtils.toBoolean(new Object())); + } + + @Test + void internalValuePredicatesMatchConstants() { + assertTrue(BooleanUtils.isTrueInternalValue(BooleanUtils.INTERNAL_TRUE)); + assertFalse(BooleanUtils.isTrueInternalValue(BooleanUtils.INTERNAL_FALSE)); + assertFalse(BooleanUtils.isTrueInternalValue(null)); + assertTrue(BooleanUtils.isFalseInternalValue(BooleanUtils.INTERNAL_FALSE)); + assertFalse(BooleanUtils.isFalseInternalValue(null)); + assertTrue(BooleanUtils.fromNonNullInternalValue(BooleanUtils.INTERNAL_TRUE)); + } +} diff --git a/pom.xml b/pom.xml index 85008dcf1350..ce060d0f9077 100644 --- a/pom.xml +++ b/pom.xml @@ -327,7 +327,10 @@ 7.12.0 - 5.14.4 + + 6.0.3 + + 1.1.0 5.23.0 3.19.4 2.0.5 @@ -1984,10 +1987,21 @@ test + - org.junit.jupiter - junit-jupiter-api - ${junit-jupiter-api.version} + org.junit + junit-bom + ${junit-bom.version} + pom + import + + + + + org.junit.support + testng-engine + ${testng-engine.version} test @@ -2046,6 +2060,33 @@ + + + + org.junit.support + testng-engine + test + + + org.junit.jupiter + junit-jupiter + test + + + + org.junit.platform + junit-platform-launcher + test + + + clean install @@ -2189,14 +2230,11 @@ -XX:+EnableDynamicAgentLoading - - - - org.apache.maven.surefire - surefire-testng - ${surefire.version} - - + org.apache.maven.plugins