zhuzhurk commented on code in PR #20153:
URL: https://github.com/apache/flink/pull/20153#discussion_r917217609
##########
flink-runtime/src/test/java/org/apache/flink/runtime/jobmaster/JobMasterTest.java:
##########
@@ -1384,8 +1374,9 @@ public void testRequestPartitionState() throws Exception {
fail("Expected failure.");
Review Comment:
Let's replace the try-catch block with a `assertThatThrownBy`
##########
flink-runtime/src/test/java/org/apache/flink/runtime/jobmaster/JobMasterTest.java:
##########
@@ -959,20 +951,20 @@ public void testReconnectionAfterDisconnect() throws
Exception {
// wait for first registration attempt
final JobMasterId firstRegistrationAttempt =
registrationsQueue.take();
- assertThat(firstRegistrationAttempt, equalTo(jobMasterId));
+ assertThat(firstRegistrationAttempt).isEqualTo(jobMasterId);
- assertThat(registrationsQueue.isEmpty(), is(true));
+ assertThat(registrationsQueue.isEmpty()).isTrue();
Review Comment:
can be `assertThat(registrationsQueue).isEmpty();`
##########
flink-runtime/src/test/java/org/apache/flink/runtime/jobmaster/JobMasterTest.java:
##########
@@ -1384,8 +1374,9 @@ public void testRequestPartitionState() throws Exception {
fail("Expected failure.");
} catch (ExecutionException e) {
assertThat(
- ExceptionUtils.findThrowable(e,
IllegalArgumentException.class).isPresent(),
- is(true));
+ ExceptionUtils.findThrowable(e,
IllegalArgumentException.class)
Review Comment:
Would `hasRootCauseInstanceOf(IllegalArgumentException.class)` work here?
If not, we can still simplify it to be
`assertThat(ExceptionUtils.findThrowable(e,
IllegalArgumentException.class)).isPresent()`.
##########
flink-runtime/src/test/java/org/apache/flink/runtime/jobmaster/JobMasterTest.java:
##########
@@ -1462,19 +1455,19 @@ public void testTriggerSavepointTimeout() throws
Exception {
try {
savepointFutureLowTimeout.get(testingTimeout.getSize(),
testingTimeout.getUnit());
- fail();
+ fail("Expected TimeoutException");
} catch (final ExecutionException e) {
final Throwable cause =
ExceptionUtils.stripExecutionException(e);
- assertThat(cause, instanceOf(TimeoutException.class));
+ assertThat(cause).isInstanceOf(TimeoutException.class);
}
- assertThat(savepointFutureHighTimeout.isDone(),
is(equalTo(false)));
+ assertThat(savepointFutureHighTimeout.isDone()).isFalse();
Review Comment:
can be `assertThat(savepointFutureHighTimeout).isNotDone();`
##########
flink-runtime/src/test/java/org/apache/flink/runtime/jobmaster/JobMasterTest.java:
##########
@@ -1414,9 +1406,10 @@ public void testRequestPartitionState() throws Exception
{
fail("Expected failure.");
} catch (ExecutionException e) {
assertThat(
- ExceptionUtils.findThrowable(e,
PartitionProducerDisposedException.class)
- .isPresent(),
- is(true));
Review Comment:
See above comment.
##########
flink-runtime/src/test/java/org/apache/flink/runtime/jobmaster/JobMasterTest.java:
##########
@@ -1398,8 +1389,9 @@ public void testRequestPartitionState() throws Exception {
fail("Expected failure.");
} catch (ExecutionException e) {
assertThat(
Review Comment:
See above 2 comments.
##########
flink-runtime/src/test/java/org/apache/flink/runtime/jobmaster/JobMasterTest.java:
##########
@@ -1593,18 +1585,18 @@ public void
testTaskExecutorNotReleasedOnFailedAllocationIfPartitionIsAllocated(
// we should free the slot, but not disconnect from the
TaskExecutor as we still have an
// allocated partition
- assertThat(freedSlotFuture.get(), equalTo(allocationId));
+ assertThat(freedSlotFuture.get()).isEqualTo(allocationId);
// trigger some request to guarantee ensure the
slotAllocationFailure processing if
// complete
jobMasterGateway.requestJobStatus(Time.seconds(5)).get();
- assertThat(disconnectTaskExecutorFuture.isDone(), is(false));
+ assertThat(disconnectTaskExecutorFuture.isDone()).isFalse();
Review Comment:
can be `assertThat(disconnectTaskExecutorFuture).isNotDone();`
##########
flink-runtime/src/test/java/org/apache/flink/runtime/jobmaster/JobMasterTest.java:
##########
@@ -1462,19 +1455,19 @@ public void testTriggerSavepointTimeout() throws
Exception {
try {
savepointFutureLowTimeout.get(testingTimeout.getSize(),
testingTimeout.getUnit());
- fail();
+ fail("Expected TimeoutException");
Review Comment:
better to use `assertThatThrownBy`
##########
flink-runtime/src/test/java/org/apache/flink/runtime/jobmaster/slotpool/DeclarativeSlotPoolServiceTest.java:
##########
@@ -230,14 +228,13 @@ public void testCreateAllocatedSlotReport() throws
Exception {
declarativeSlotPoolService.createAllocatedSlotReport(
taskManagerLocation2.getResourceID());
- assertThat(
- allocatedSlotReport.getAllocatedSlotInfos(),
- contains(matchesWithSlotContext(simpleSlotContext2)));
+ assertThat(allocatedSlotReport.getAllocatedSlotInfos())
+
.is(matching(contains(matchesWithSlotContext(simpleSlotContext2))));
Review Comment:
This matcher is simpler so I prefer to drop it and rewrite the verification
to be:
```
assertThat(allocatedSlotReport.getAllocatedSlotInfos())
.allMatches(
context ->
context.getAllocationId()
.equals(simpleSlotContext2.getAllocationId())
&& context.getSlotIndex()
==
simpleSlotContext2.getPhysicalSlotNumber());
```
--
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]
