Copilot commented on code in PR #19920:
URL: https://github.com/apache/druid/pull/19920#discussion_r3737514646
##########
indexing-service/src/test/java/org/apache/druid/indexing/compact/OverlordCompactionSchedulerTest.java:
##########
@@ -311,15 +311,15 @@ public void test_enableSupervisors_triggersStart()
// Becoming leader does not trigger start since scheduler is disabled
scheduler.becomeLeader();
runScheduledJob();
- Assert.assertFalse(scheduler.isRunning());
+ Assertions.assertFalse(scheduler.isRunning());
// Enable the schduler to trigger start
Review Comment:
Typo in comment: "schduler" "scheduler".
##########
indexing-service/src/test/java/org/apache/druid/indexing/common/tasklogs/FileTaskLogsTest.java:
##########
@@ -66,7 +57,7 @@ public void testSimple() throws Exception
for (Map.Entry<Long, String> entry : expected.entrySet()) {
final byte[] bytes =
ByteStreams.toByteArray(taskLogs.streamTaskLog("foo", entry.getKey()).get());
final String string = StringUtils.fromUtf8(bytes);
- Assert.assertEquals(StringUtils.format("Read with offset %,d",
entry.getKey()), string, entry.getValue());
+ Assertions.assertEquals(string, entry.getValue(),
StringUtils.format("Read with offset %,d", entry.getKey()));
Review Comment:
`Assertions.assertEquals` uses the signature `assertEquals(expected, actual,
message)`. Here the actual and expected values are swapped, which makes
failures harder to interpret (expected/actual will be reversed in the assertion
output).
##########
indexing-service/src/test/java/org/apache/druid/indexing/input/DruidSegmentReaderTest.java:
##########
@@ -1022,7 +1017,7 @@ private void persistSegment(List<InputRow> rows) throws
IOException
.rows(rows)
.buildIncrementalIndex();
- segmentDirectory = temporaryFolder.newFolder();
+ segmentDirectory = FileUtils.createTempDir();
Review Comment:
`FileUtils.createTempDir()` does not automatically clean up (it just calls
`Files.createTempDirectory`), so `segmentDirectory` will be left behind after
each test run unless it is explicitly deleted (previously `TemporaryFolder`
handled cleanup). Consider adding an `@AfterEach` that deletes
`segmentDirectory` (and ideally reusing a single temp directory for the reader
temp dirs as well).
##########
indexing-service/src/test/java/org/apache/druid/indexing/common/TaskToolboxTest.java:
##########
@@ -118,7 +114,7 @@ public void setUp() throws IOException
EasyMock.replay(task, mockHandoffNotifierFactory, mockIndexMergerV9);
TaskConfig taskConfig = new TaskConfigBuilder()
- .setBaseDir(temporaryFolder.newFile().toString())
+ .setBaseDir(FileUtils.createTempDir().toString())
.build();
Review Comment:
`TaskConfigBuilder.setBaseDir(FileUtils.createTempDir().toString())` creates
a fresh temp directory for every test method, but there is no corresponding
cleanup. This can leak directories under `java.io.tmpdir` across test runs.
Prefer using a JUnit 5 temp directory lifecycle (`@TempDir`) or deleting the
created directory in an `@AfterEach`.
##########
indexing-service/src/test/java/org/apache/druid/indexing/common/tasklogs/FileTaskLogsTest.java:
##########
@@ -78,7 +69,7 @@ public void testSimple() throws Exception
public void testSimpleReport() throws Exception
{
final ObjectMapper mapper = TestHelper.makeJsonMapper();
- final File tmpDir = temporaryFolder.newFolder();
+ final File tmpDir = FileUtils.createTempDir();
final File logDir = new File(tmpDir, "druid/logs");
final File reportFile = new File(tmpDir, "report.json");
Review Comment:
This test creates a temp directory via `FileUtils.createTempDir()` but never
deletes it. Unlike `TemporaryFolder`, `FileUtils.createTempDir()` does not
clean up automatically, so repeated test runs can leak directories under
`java.io.tmpdir`.
This issue also appears on line 94 of the same file.
##########
indexing-service/src/test/java/org/apache/druid/indexing/common/task/AbstractTaskTest.java:
##########
@@ -56,15 +54,19 @@ public class AbstractTaskTest
{
private ObjectMapper objectMapper;
- @Rule
- public TemporaryFolder temporaryFolder = new TemporaryFolder();
-
- @Before
+ @BeforeEach
public void setup()
{
objectMapper = new TestUtils().getTestObjectMapper();
}
+ private static File createTempReportFile() throws Exception
+ {
+ final File reportsFile = new
File(org.apache.druid.java.util.common.FileUtils.createTempDir(),
"report.json");
+ FileUtils.write(reportsFile, "", StandardCharsets.UTF_8);
+ return reportsFile;
Review Comment:
`createTempReportFile()` creates a new temp directory and file but never
deletes it. Since `FileUtils.createTempDir()` does not auto-cleanup, repeated
runs can leave many `report.json` temp dirs behind. Consider using `@TempDir`
or deleting the parent directory after the test completes.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
