You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by "FrankChen021 (via GitHub)" <gi...@apache.org> on 2026/08/07 18:37:16 UTC
[PR] test(indexing-service): migrate worker tests to JUnit 5 (druid)
FrankChen021 opened a new pull request, #19925:
URL: https://github.com/apache/druid/pull/19925
## Summary
- Migrate the indexing-service worker, executor, HTTP resource, and shuffle tests to JUnit 5.
- Replace JUnit 4 temporary-folder fixtures with Druid's `FileUtils` helpers.
- Add the JUnit 5 parameterized-test dependency required by the migrated worker tests.
## Validation
- Targeted worker tests: 61 passed, 0 failures, 0 errors, 0 skipped.
- `mvn verify -pl indexing-service -am -Dweb.console.skip=true -T1C`: passed.
- Checkstyle, PMD, Enforcer, forbidden-API checks, test compilation, and `git diff --check`: passed.
Part of #13948.
Related migration PRs: #19910, #19920, #19922, #19923.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] test(indexing-service): migrate worker tests to JUnit 5 (druid)
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #19925:
URL: https://github.com/apache/druid/pull/19925#discussion_r3738016304
##########
indexing-service/src/test/java/org/apache/druid/indexing/worker/WorkerTaskManagerTest.java:
##########
@@ -91,22 +95,21 @@ public class WorkerTaskManagerTest
private final IndexMergerV9Factory indexMergerV9Factory;
private final IndexIO indexIO;
- private final boolean restoreTasksOnRestart;
+ @Parameter(0)
+ private boolean restoreTasksOnRestart;
Review Comment:
`@Parameter` fields are consistently declared `public` for `@ParameterizedClass` tests in this codebase (for example `processing/src/test/java/org/apache/druid/common/guava/CombiningSequenceTest.java:63-65`). Keeping this one `private` risks the parameter injection not being applied (and the default `false` being used), depending on how `@ParameterizedClass` populates fields. Please align with the established pattern by making the field `public`.
##########
indexing-service/src/test/java/org/apache/druid/indexing/worker/shuffle/ShuffleDataSegmentPusherTest.java:
##########
@@ -68,45 +67,41 @@
import java.util.List;
import java.util.Optional;
-@RunWith(Parameterized.class)
+@ParameterizedClass
+@MethodSource("data")
public class ShuffleDataSegmentPusherTest
{
private static final String LOCAL = "local";
private static final String DEEPSTORE = "deepstore";
- @Parameterized.Parameters(name = "intermediateDataManager={0}")
public static Collection<Object[]> data()
{
return ImmutableList.of(new Object[]{LOCAL}, new Object[]{DEEPSTORE});
}
- @Rule
- public final TemporaryFolder temporaryFolder = new TemporaryFolder();
+ @Parameter(0)
+ private String intermediateDataStore;
+
Review Comment:
`@Parameter` fields in existing `@ParameterizedClass` tests are declared `public` (for example `processing/src/test/java/org/apache/druid/common/guava/CombiningSequenceTest.java:63-65`). Using `private` here risks the parameter injection not being applied, depending on the `@ParameterizedClass` implementation. Please make the field `public` to match the established pattern.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] test(indexing-service): migrate worker tests to JUnit 5 (druid)
Posted by "github-advanced-security[bot] (via GitHub)" <gi...@apache.org>.
github-advanced-security[bot] commented on code in PR #19925:
URL: https://github.com/apache/druid/pull/19925#discussion_r3738147451
##########
indexing-service/src/test/java/org/apache/druid/indexing/worker/shuffle/ShuffleDataSegmentPusherTest.java:
##########
@@ -169,34 +165,41 @@
);
} else if (intermediaryDataManager instanceof DeepStorageIntermediaryDataManager) {
final LoadSpec loadSpec = mapper.convertValue(pushed.getLoadSpec(), LoadSpec.class);
- Assert.assertTrue(pushed.getLoadSpec()
- .get("path")
- .toString()
- .startsWith(localDeepStore.getAbsolutePath()
- + "/"
- + DeepStorageIntermediaryDataManager.SHUFFLE_DATA_DIR_PREFIX));
+ Assertions.assertTrue(pushed.getLoadSpec()
+ .get("path")
+ .toString()
+ .startsWith(localDeepStore.getAbsolutePath()
+ + "/"
+ + DeepStorageIntermediaryDataManager.SHUFFLE_DATA_DIR_PREFIX));
loadSpec.loadSegment(tempDir);
}
final List<File> unzippedFiles = Arrays.asList(tempDir.listFiles());
unzippedFiles.sort(Comparator.comparing(File::getName));
final File dataFile = unzippedFiles.get(0);
- Assert.assertEquals("test", dataFile.getName());
- Assert.assertEquals("test data.", Files.readFirstLine(dataFile, StandardCharsets.UTF_8));
+ Assertions.assertEquals("test", dataFile.getName());
+ Assertions.assertEquals("test data.", Files.readFirstLine(dataFile, StandardCharsets.UTF_8));
Review Comment:
## CodeQL / Deprecated method or constructor invocation
Invoking [Files.readFirstLine](1) should be avoided because it has been deprecated.
[Show more details](https://github.com/apache/druid/security/code-scanning/11740)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] test(indexing-service): migrate worker tests to JUnit 5 (druid)
Posted by "FrankChen021 (via GitHub)" <gi...@apache.org>.
FrankChen021 commented on code in PR #19925:
URL: https://github.com/apache/druid/pull/19925#discussion_r3739914348
##########
indexing-service/src/test/java/org/apache/druid/indexing/worker/shuffle/ShuffleDataSegmentPusherTest.java:
##########
@@ -169,34 +165,41 @@
);
} else if (intermediaryDataManager instanceof DeepStorageIntermediaryDataManager) {
final LoadSpec loadSpec = mapper.convertValue(pushed.getLoadSpec(), LoadSpec.class);
- Assert.assertTrue(pushed.getLoadSpec()
- .get("path")
- .toString()
- .startsWith(localDeepStore.getAbsolutePath()
- + "/"
- + DeepStorageIntermediaryDataManager.SHUFFLE_DATA_DIR_PREFIX));
+ Assertions.assertTrue(pushed.getLoadSpec()
+ .get("path")
+ .toString()
+ .startsWith(localDeepStore.getAbsolutePath()
+ + "/"
+ + DeepStorageIntermediaryDataManager.SHUFFLE_DATA_DIR_PREFIX));
loadSpec.loadSegment(tempDir);
}
final List<File> unzippedFiles = Arrays.asList(tempDir.listFiles());
unzippedFiles.sort(Comparator.comparing(File::getName));
final File dataFile = unzippedFiles.get(0);
- Assert.assertEquals("test", dataFile.getName());
- Assert.assertEquals("test data.", Files.readFirstLine(dataFile, StandardCharsets.UTF_8));
+ Assertions.assertEquals("test", dataFile.getName());
+ Assertions.assertEquals("test data.", Files.readFirstLine(dataFile, StandardCharsets.UTF_8));
Review Comment:
Addressed in the existing local edit: replaced deprecated Files.readFirstLine(...) with Files.asCharSource(...).readFirstLine().
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] test(indexing-service): migrate worker tests to JUnit 5 (druid)
Posted by "FrankChen021 (via GitHub)" <gi...@apache.org>.
FrankChen021 merged PR #19925:
URL: https://github.com/apache/druid/pull/19925
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] test(indexing-service): migrate worker tests to JUnit 5 (druid)
Posted by "FrankChen021 (via GitHub)" <gi...@apache.org>.
FrankChen021 commented on code in PR #19925:
URL: https://github.com/apache/druid/pull/19925#discussion_r3739914081
##########
indexing-service/src/test/java/org/apache/druid/indexing/worker/WorkerTaskManagerTest.java:
##########
@@ -91,22 +95,21 @@ public class WorkerTaskManagerTest
private final IndexMergerV9Factory indexMergerV9Factory;
private final IndexIO indexIO;
- private final boolean restoreTasksOnRestart;
+ @Parameter(0)
+ private boolean restoreTasksOnRestart;
Review Comment:
Addressed in the existing local edit: changed restoreTasksOnRestart from private to public so JUnit 5 ParameterizedClass can inject it consistently.
##########
indexing-service/src/test/java/org/apache/druid/indexing/worker/shuffle/ShuffleDataSegmentPusherTest.java:
##########
@@ -68,45 +67,41 @@
import java.util.List;
import java.util.Optional;
-@RunWith(Parameterized.class)
+@ParameterizedClass
+@MethodSource("data")
public class ShuffleDataSegmentPusherTest
{
private static final String LOCAL = "local";
private static final String DEEPSTORE = "deepstore";
- @Parameterized.Parameters(name = "intermediateDataManager={0}")
public static Collection<Object[]> data()
{
return ImmutableList.of(new Object[]{LOCAL}, new Object[]{DEEPSTORE});
}
- @Rule
- public final TemporaryFolder temporaryFolder = new TemporaryFolder();
+ @Parameter(0)
+ private String intermediateDataStore;
+
Review Comment:
Addressed in the existing local edit: changed intermediateDataStore from private to public to match the established ParameterizedClass injection pattern.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org