Copilot commented on code in PR #17718:
URL:
https://github.com/apache/dolphinscheduler/pull/17718#discussion_r2562518783
##########
dolphinscheduler-task-plugin/dolphinscheduler-task-dms/src/test/java/org/apache/dolphinscheduler/plugin/task/dms/DmsHookTest.java:
##########
@@ -238,4 +243,61 @@ public void testAwaitReplicationTaskStatus() {
}
});
}
+
+ @Test
+ public void replaceFileParameters_NullParameter_ReturnsNull() throws
IOException {
+ Assertions.assertTimeout(Duration.ofMillis(60000), () -> {
+ try (MockedStatic<DmsHook> mockHook =
Mockito.mockStatic(DmsHook.class)) {
+ mockHook.when(DmsHook::createClient).thenReturn(client);
+ DmsHook dmsHook = spy(new DmsHook());
+ String parameter = null;
+ String result = dmsHook.replaceFileParameters(parameter);
+ Assertions.assertEquals(null, result);
+ }
+ });
+ }
+
+ @Test
+ public void replaceFileParameters_NormalString_ReturnsSameString() throws
IOException {
+ Assertions.assertTimeout(Duration.ofMillis(60000), () -> {
+ try (MockedStatic<DmsHook> mockHook =
Mockito.mockStatic(DmsHook.class)) {
+ mockHook.when(DmsHook::createClient).thenReturn(client);
+ DmsHook dmsHook = spy(new DmsHook());
+ String parameter = "normal string";
+ String result = dmsHook.replaceFileParameters(parameter);
+ Assertions.assertEquals(parameter, result);
+ }
+ });
+ }
+
+ @Test
+ public void replaceFileParameters_FileExists_ReturnsFileContent() throws
IOException {
+ Assertions.assertTimeout(Duration.ofMillis(60000), () -> {
+ try (MockedStatic<DmsHook> mockHook =
Mockito.mockStatic(DmsHook.class)) {
+ mockHook.when(DmsHook::createClient).thenReturn(client);
+ DmsHook dmsHook = spy(new DmsHook());
+ File tempFile = new File("tempFile.txt");
+ String fileContent = "content of the file";
+ FileUtils.writeStringToFile(tempFile, fileContent,
StandardCharsets.UTF_8);
+ String parameter = "file://" + tempFile.getAbsolutePath();
+ String result = dmsHook.replaceFileParameters(parameter);
+ Assertions.assertEquals(fileContent, result);
Review Comment:
Resource leak: The temporary file created at line 279 is never deleted after
the test completes. Add cleanup code to delete the file, ideally using a
try-finally block or JUnit's `@TempDir` annotation to ensure the file is
deleted even if the test fails:
```java
try {
File tempFile = new File("tempFile.txt");
String fileContent = "content of the file";
FileUtils.writeStringToFile(tempFile, fileContent,
StandardCharsets.UTF_8);
String parameter = "file://" + tempFile.getAbsolutePath();
String result = dmsHook.replaceFileParameters(parameter);
Assertions.assertEquals(fileContent, result);
} finally {
new File("tempFile.txt").delete();
}
```
```suggestion
File tempFile = new File("tempFile.txt");
try (MockedStatic<DmsHook> mockHook =
Mockito.mockStatic(DmsHook.class)) {
mockHook.when(DmsHook::createClient).thenReturn(client);
DmsHook dmsHook = spy(new DmsHook());
String fileContent = "content of the file";
FileUtils.writeStringToFile(tempFile, fileContent,
StandardCharsets.UTF_8);
String parameter = "file://" + tempFile.getAbsolutePath();
String result = dmsHook.replaceFileParameters(parameter);
Assertions.assertEquals(fileContent, result);
} finally {
tempFile.delete();
```
##########
dolphinscheduler-task-plugin/dolphinscheduler-task-dms/src/test/java/org/apache/dolphinscheduler/plugin/task/dms/DmsHookTest.java:
##########
@@ -238,4 +243,61 @@ public void testAwaitReplicationTaskStatus() {
}
});
}
+
+ @Test
+ public void replaceFileParameters_NullParameter_ReturnsNull() throws
IOException {
+ Assertions.assertTimeout(Duration.ofMillis(60000), () -> {
+ try (MockedStatic<DmsHook> mockHook =
Mockito.mockStatic(DmsHook.class)) {
+ mockHook.when(DmsHook::createClient).thenReturn(client);
+ DmsHook dmsHook = spy(new DmsHook());
+ String parameter = null;
+ String result = dmsHook.replaceFileParameters(parameter);
+ Assertions.assertEquals(null, result);
+ }
+ });
+ }
+
+ @Test
+ public void replaceFileParameters_NormalString_ReturnsSameString() throws
IOException {
+ Assertions.assertTimeout(Duration.ofMillis(60000), () -> {
+ try (MockedStatic<DmsHook> mockHook =
Mockito.mockStatic(DmsHook.class)) {
+ mockHook.when(DmsHook::createClient).thenReturn(client);
+ DmsHook dmsHook = spy(new DmsHook());
+ String parameter = "normal string";
+ String result = dmsHook.replaceFileParameters(parameter);
+ Assertions.assertEquals(parameter, result);
+ }
+ });
+ }
+
+ @Test
+ public void replaceFileParameters_FileExists_ReturnsFileContent() throws
IOException {
+ Assertions.assertTimeout(Duration.ofMillis(60000), () -> {
+ try (MockedStatic<DmsHook> mockHook =
Mockito.mockStatic(DmsHook.class)) {
+ mockHook.when(DmsHook::createClient).thenReturn(client);
+ DmsHook dmsHook = spy(new DmsHook());
+ File tempFile = new File("tempFile.txt");
+ String fileContent = "content of the file";
+ FileUtils.writeStringToFile(tempFile, fileContent,
StandardCharsets.UTF_8);
+ String parameter = "file://" + tempFile.getAbsolutePath();
+ String result = dmsHook.replaceFileParameters(parameter);
+ Assertions.assertEquals(fileContent, result);
+ }
+ });
+ }
+
+ @Test
+ public void replaceFileParameters_FileNotExists_ThrowsIOException() {
+ Assertions.assertTimeout(Duration.ofMillis(60000), () -> {
+ try (MockedStatic<DmsHook> mockHook =
Mockito.mockStatic(DmsHook.class)) {
+ mockHook.when(DmsHook::createClient).thenReturn(client);
+ DmsHook dmsHook = spy(new DmsHook());
+ String parameter = "file://nonexistentfile.txt";
+ Assertions.assertThrows(IOException.class, () -> {
+ dmsHook.replaceFileParameters(parameter);
+ });
+ }
+ });
+ }
Review Comment:
[nitpick] Naming: Test method names should follow a consistent convention.
The existing tests use camelCase starting with 'test' (e.g.,
`testCreateReplicationTask`), but the new tests use a different convention with
underscores (e.g., `replaceFileParameters_NullParameter_ReturnsNull`). Consider
renaming to maintain consistency:
- `replaceFileParameters_NullParameter_ReturnsNull` →
`testReplaceFileParametersWithNull`
- `replaceFileParameters_NormalString_ReturnsSameString` →
`testReplaceFileParametersWithNormalString`
- `replaceFileParameters_FileExists_ReturnsFileContent` →
`testReplaceFileParametersWithExistingFile`
- `replaceFileParameters_FileNotExists_ThrowsIOException` →
`testReplaceFileParametersWithNonexistentFile`
##########
dolphinscheduler-task-plugin/dolphinscheduler-task-dms/src/test/java/org/apache/dolphinscheduler/plugin/task/dms/DmsHookTest.java:
##########
@@ -238,4 +243,61 @@ public void testAwaitReplicationTaskStatus() {
}
});
}
+
+ @Test
+ public void replaceFileParameters_NullParameter_ReturnsNull() throws
IOException {
+ Assertions.assertTimeout(Duration.ofMillis(60000), () -> {
+ try (MockedStatic<DmsHook> mockHook =
Mockito.mockStatic(DmsHook.class)) {
+ mockHook.when(DmsHook::createClient).thenReturn(client);
+ DmsHook dmsHook = spy(new DmsHook());
+ String parameter = null;
+ String result = dmsHook.replaceFileParameters(parameter);
+ Assertions.assertEquals(null, result);
Review Comment:
Best practices: Use `assertNull` instead of `assertEquals(null, result)` for
better test readability and more descriptive failure messages. Change:
```java
Assertions.assertEquals(null, result);
```
to:
```java
Assertions.assertNull(result);
```
```suggestion
Assertions.assertNull(result);
```
##########
dolphinscheduler-task-plugin/dolphinscheduler-task-dms/src/main/java/org/apache/dolphinscheduler/plugin/task/dms/DmsHook.java:
##########
@@ -132,6 +132,8 @@ public Boolean startReplicationTask() {
public Boolean checkFinishedReplicationTask() {
log.info("checkFinishedReplicationTask ......");
awaitReplicationTaskStatus(STATUS.STOPPED);
+ // shutdown client
+ client.shutdown();
String stopReason = describeReplicationTasks().getStopReason();
Review Comment:
Bug: The client is shut down on line 136, but then
`describeReplicationTasks()` is called on line 137, which attempts to use the
already-closed client. This will cause an error when trying to access the
client. The client shutdown should be moved after the
`describeReplicationTasks()` call.
```suggestion
String stopReason = describeReplicationTasks().getStopReason();
// shutdown client
client.shutdown();
```
--
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]
