Copilot commented on code in PR #17719:
URL:
https://github.com/apache/dolphinscheduler/pull/17719#discussion_r2562517210
##########
dolphinscheduler-task-plugin/dolphinscheduler-task-emr/src/test/java/org/apache/dolphinscheduler/plugin/task/emr/EmrAddStepsTaskTest.java:
##########
@@ -198,4 +204,34 @@ private EmrParameters buildErrorEmrTaskParameters() {
return emrParameters;
}
+
+ @Test
+ public void
cancelApplication_CancelStepsResultIsNull_ShouldThrowException() {
+
Mockito.when(emrClient.cancelSteps(any(CancelStepsRequest.class))).thenReturn(null);
+
+ TaskException exception = Assertions.assertThrows(TaskException.class,
() -> {
+ emrAddStepsTask.cancelApplication();
+ });
+
+ verify(emrClient, times(1)).shutdown();
+ }
+
+ @Test
+ public void
cancelApplication_CancelStepsRequestFails_ShouldThrowException() {
+ CancelStepsInfo cancelStepsInfo = new CancelStepsInfo()
+ .withStepId("step-123")
+ .withStatus(CancelStepsRequestStatus.FAILED.toString())
+ .withReason("Test failure");
+
+ CancelStepsResult cancelStepsResult = new CancelStepsResult()
+ .withCancelStepsInfoList(cancelStepsInfo);
+
+
Mockito.when(emrClient.cancelSteps(any(CancelStepsRequest.class))).thenReturn(cancelStepsResult);
+
+ TaskException exception = Assertions.assertThrows(TaskException.class,
() -> {
+ emrAddStepsTask.cancelApplication();
+ });
Review Comment:
[nitpick] The `exception` variable is declared but never used. Consider
either using it for additional assertions (e.g., checking the exception
message) or removing the variable assignment:
```java
Assertions.assertThrows(TaskException.class, () -> {
emrAddStepsTask.cancelApplication();
});
```
##########
dolphinscheduler-task-plugin/dolphinscheduler-task-emr/src/test/java/org/apache/dolphinscheduler/plugin/task/emr/EmrAddStepsTaskTest.java:
##########
@@ -198,4 +204,34 @@ private EmrParameters buildErrorEmrTaskParameters() {
return emrParameters;
}
+
+ @Test
+ public void
cancelApplication_CancelStepsResultIsNull_ShouldThrowException() {
+
Mockito.when(emrClient.cancelSteps(any(CancelStepsRequest.class))).thenReturn(null);
+
+ TaskException exception = Assertions.assertThrows(TaskException.class,
() -> {
+ emrAddStepsTask.cancelApplication();
+ });
+
+ verify(emrClient, times(1)).shutdown();
+ }
+
+ @Test
+ public void
cancelApplication_CancelStepsRequestFails_ShouldThrowException() {
+ CancelStepsInfo cancelStepsInfo = new CancelStepsInfo()
+ .withStepId("step-123")
+ .withStatus(CancelStepsRequestStatus.FAILED.toString())
+ .withReason("Test failure");
+
+ CancelStepsResult cancelStepsResult = new CancelStepsResult()
+ .withCancelStepsInfoList(cancelStepsInfo);
+
+
Mockito.when(emrClient.cancelSteps(any(CancelStepsRequest.class))).thenReturn(cancelStepsResult);
+
+ TaskException exception = Assertions.assertThrows(TaskException.class,
() -> {
+ emrAddStepsTask.cancelApplication();
+ });
+
+ verify(emrClient, times(1)).shutdown();
+ }
Review Comment:
Test coverage gap: There's no test for successful cancellation in
`EmrAddStepsTask`, while `EmrJobFlowTask` has
`testCancelApplication_Success()`. Consider adding a similar test to verify
that `shutdown()` is called in the success path:
```java
@Test
public void testCancelApplication_Success() throws TaskException {
CancelStepsInfo cancelStepsInfo = new CancelStepsInfo()
.withStepId("step-xx")
.withStatus(CancelStepsRequestStatus.SUBMITTED.toString());
CancelStepsResult cancelStepsResult = new CancelStepsResult()
.withCancelStepsInfoList(cancelStepsInfo);
Mockito.when(emrClient.cancelSteps(any(CancelStepsRequest.class))).thenReturn(cancelStepsResult);
Mockito.doNothing().when(emrClient).shutdown();
Assertions.assertDoesNotThrow(() -> emrAddStepsTask.cancelApplication());
verify(emrClient, times(1)).cancelSteps(any(CancelStepsRequest.class));
verify(emrClient, times(1)).shutdown();
}
```
Note: You'll need to initialize `stepId` to "step-xx" in the test setup or
use reflection to set it.
##########
dolphinscheduler-task-plugin/dolphinscheduler-task-emr/src/test/java/org/apache/dolphinscheduler/plugin/task/emr/EmrAddStepsTaskTest.java:
##########
@@ -198,4 +204,34 @@ private EmrParameters buildErrorEmrTaskParameters() {
return emrParameters;
}
+
+ @Test
+ public void
cancelApplication_CancelStepsResultIsNull_ShouldThrowException() {
+
Mockito.when(emrClient.cancelSteps(any(CancelStepsRequest.class))).thenReturn(null);
+
+ TaskException exception = Assertions.assertThrows(TaskException.class,
() -> {
+ emrAddStepsTask.cancelApplication();
+ });
+
+ verify(emrClient, times(1)).shutdown();
+ }
+
+ @Test
+ public void
cancelApplication_CancelStepsRequestFails_ShouldThrowException() {
+ CancelStepsInfo cancelStepsInfo = new CancelStepsInfo()
+ .withStepId("step-123")
+ .withStatus(CancelStepsRequestStatus.FAILED.toString())
+ .withReason("Test failure");
+
+ CancelStepsResult cancelStepsResult = new CancelStepsResult()
+ .withCancelStepsInfoList(cancelStepsInfo);
Review Comment:
Test issue: The test creates a `CancelStepsInfo` with stepId "step-123", but
the `emrAddStepsTask.stepId` is never initialized in this test (it remains
null). When `cancelApplication()` filters the cancel steps info list by stepId
at line 206, it won't find a match because it's filtering by `null` instead of
"step-123".
Consider initializing the stepId in the test setup to properly test the
FAILED status scenario, or use a stepId that matches what the task instance
would have.
##########
dolphinscheduler-task-plugin/dolphinscheduler-task-emr/src/test/java/org/apache/dolphinscheduler/plugin/task/emr/EmrAddStepsTaskTest.java:
##########
@@ -198,4 +204,34 @@ private EmrParameters buildErrorEmrTaskParameters() {
return emrParameters;
}
+
+ @Test
+ public void
cancelApplication_CancelStepsResultIsNull_ShouldThrowException() {
+
Mockito.when(emrClient.cancelSteps(any(CancelStepsRequest.class))).thenReturn(null);
+
+ TaskException exception = Assertions.assertThrows(TaskException.class,
() -> {
+ emrAddStepsTask.cancelApplication();
+ });
Review Comment:
[nitpick] The `exception` variable is declared but never used. Consider
either using it for additional assertions (e.g., checking the exception
message) or removing the variable assignment:
```java
Assertions.assertThrows(TaskException.class, () -> {
emrAddStepsTask.cancelApplication();
});
```
--
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]
