gaoyunhaii commented on a change in pull request #15466:
URL: https://github.com/apache/flink/pull/15466#discussion_r605754852



##########
File path: 
flink-runtime/src/test/java/org/apache/flink/runtime/checkpoint/DefaultCheckpointPlanCalculatorTest.java
##########
@@ -61,6 +64,17 @@
  */
 public class DefaultCheckpointPlanCalculatorTest {
 
+    @Test(expected = CheckpointException.class)
+    public void testCheckpointNotStartedIfSourceCancelled() throws Exception {

Review comment:
       Perhaps we could still leave the declaration way for normal tests to 
simplify the change? Currently there is already a test 
`testWithTriggeredTasksNotRunning`, but now it only tested the `CREATED` state. 
we might slight change the graph build logic to 
   
   ```
   JobVertexID jobVertexID = new JobVertexID();
           ExecutionGraph graph =
                   new 
CheckpointCoordinatorTestingUtils.CheckpointExecutionGraphBuilder()
                           .addJobVertex(jobVertexID)
                           .setTransitToRunning(false)
                           .build();
           graph.getJobVertex(jobVertexID)
                   .getTaskVertices()[0]
                   .getCurrentExecutionAttempt()
                   .transitionState(ExecutionState.xx);
   ```
   to simulate the case that the source is in specialized state. Then we do not 
need to modify the following utility processes. 
   
   Besides, perhaps we could also tests all the possible exceptional states 
with a loop ? namely something like 
   
   ```
   for (ExecutionState state : ExecutionState.values()) {
       if (state != ExecutionState.RUNNING && state != ExecutionState.FINISHED) 
{
           // build graph and run test
       }
   }
   
   ```




-- 
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.

For queries about this service, please contact Infrastructure at:
[email protected]


Reply via email to