GJL commented on a change in pull request #7659: [FLINK-11514][client] Port
ClusterClientTest
URL: https://github.com/apache/flink/pull/7659#discussion_r254661621
##########
File path:
flink-clients/src/test/java/org/apache/flink/client/program/rest/RestClusterClientTest.java
##########
@@ -448,20 +457,24 @@ public void testTriggerSavepoint() throws Exception {
try {
JobID id = new JobID();
{
- CompletableFuture<String>
savepointPathFuture = restClusterClient.triggerSavepoint(id, null);
- String savepointPath =
savepointPathFuture.get();
+ String savepointPath = cancelJob
Review comment:
If we only assert on the job cancel flag being set to `true`, a single test
would be enough imo:
```
@Test
public void
testCancelJobWithSavepointShouldTriggerSavepointWithCancelJobFlag() throws
Exception {
final boolean cancelJob = true;
final String expectedSavepointLocation =
"/savepoints/savepoint-0d2fb9-8d5e0106041a";
final TestSavepointHandlers testSavepointHandlers = new
TestSavepointHandlers();
final TestSavepointHandlers.TestSavepointTriggerHandler
triggerHandler =
testSavepointHandlers.new TestSavepointTriggerHandler(
cancelJob, "/savepoints");
final TestSavepointHandlers.TestSavepointHandler
savepointHandler =
testSavepointHandlers.new TestSavepointHandler(new
SavepointInfo(
expectedSavepointLocation,
null));
try (TestRestServerEndpoint restServerEndpoint =
createRestServerEndpoint(
triggerHandler,
savepointHandler)) {
final RestClusterClient<?> restClusterClient =
createRestClusterClient(restServerEndpoint.getServerAddress().getPort());
final String savepointLocation =
restClusterClient.cancelWithSavepoint(new JobID(), "/savepoints");
assertEquals(expectedSavepointLocation,
savepointLocation);
}
}
```
It was a mistake to have that many scenarios + asserts in
`testTriggerSavepoint`. The test was hard to read to begin with and this change
makes it even harder.
If you think the current state is readable enough, maybe consider
eliminating the code duplication:
```
private static String triggerSavepoint(final boolean cancelJob, final
String targetSavepointDirectory, final RestClusterClient<?> restClusterClient)
throws Exception {
return cancelJob
? restClusterClient.cancelWithSavepoint(new JobID(),
targetSavepointDirectory)
: restClusterClient.triggerSavepoint(new JobID(),
targetSavepointDirectory).get();
}
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
With regards,
Apache Git Services
