-
Notifications
You must be signed in to change notification settings - Fork 13.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[FLINK-32852][JUnit5 Migration] Migrate the scheduler package of flink-runtime module to JUnit5 #24732
Conversation
…k-runtime module to JUnit5
hi, @JingGe @1996fanrui Could you help take a look if you had the free time ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some classes should be changed, but they are missed, so I commented here:
- Remove @ExtendWith(TestLoggerExtension.class)
- EnforceMinimalIncreaseRescalingControllerTest
- DefaultExecutionDeployerTest
- StopWithSavepointTest
- Don't use
org.junit.Assert
- StateValidator
- SchedulerTestingUtils
- MockStateWithExecutionGraphContext
- public can be removed for test method
- AdaptiveSchedulerTest
- StateLocalitySlotAssignerTest
- public can be removed for test class
- you can search
public class
, I saw a lot of classes need to remove it. - Update
isEqualTo(0)
toisZero
- AdaptiveSchedulerTest
- AllToAllBlockingResultInfoTest
- Update
isEqualTo(1)
toisOne
- ExecutionTimeBasedSlowTaskDetectorTest
- SpeculativeExecutionTest
- PointwiseBlockingResultInfoTest
- DefaultVertexParallelismAndInputInfosDeciderTest
- AdaptiveSchedulerTest
- AllToAllBlockingResultInfoTest
- hasSameSizeAs
- DefaultVertexParallelismAndInputInfosDeciderTest (line 445)
|
||
@Override | ||
public void beforeEach(ExtensionContext context) throws Exception { | ||
miniClusterResource.before(); | ||
} | ||
|
||
@Override | ||
public void afterEach(ExtensionContext context) throws Exception { | ||
miniClusterResource.after(); | ||
} | ||
|
||
@Override | ||
public void before(ExtensionContext context) throws Exception { | ||
miniClusterResource.before(); | ||
} | ||
|
||
@Override | ||
public void after(ExtensionContext context) throws Exception { | ||
miniClusterResource.after(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
stateWithExecutionGraph.getGloballyTerminalStateFuture().get(), | ||
is(JobStatus.FINISHED)); | ||
assertThatFuture(stateWithExecutionGraph.getGloballyTerminalStateFuture()) | ||
.isCompletedWithValue(JobStatus.FINISHED); | ||
} | ||
|
||
@Test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All public can be removed.
finishedBackgroundTask.getTerminationFuture().join(); | ||
} | ||
|
||
@Test | ||
public void testFinishedBackgroundTaskDoesNotContainAResult() { | ||
final BackgroundTask<Void> finishedBackgroundTask = BackgroundTask.finishedBackgroundTask(); | ||
|
||
assertTrue(finishedBackgroundTask.getResultFuture().isCompletedExceptionally()); | ||
assertThatFuture(finishedBackgroundTask.getResultFuture()).isCompletedExceptionally(); | ||
} | ||
|
||
@Test | ||
public void testNormalCompletionOfBackgroundTask() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All public can be removed.
@@ -182,21 +182,20 @@ private void triggerComputeNumOfSubpartitions(IntermediateResult result) { | |||
private void assertNetworkMemory( | |||
List<SlotSharingGroup> slotSharingGroups, List<MemorySize> networkMemory) { | |||
|
|||
assertEquals(slotSharingGroups.size(), networkMemory.size()); | |||
assertThat(networkMemory.size()).isEqualTo(slotSharingGroups.size()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hasSameSizeAs
assertThat(slotSharingGroups.size()).isEqualTo(3); | ||
assertThat(parallelisms.size()).isEqualTo(3); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hasSize
Thank you very much for the patient-check and review. There're some class referenced by other class. so I kept the As shown in the commit messages, I split the patches by commit. |
@flinkbot run azure |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @RocMarshal for the update!
LGTM
What is the purpose of the change
[FLINK-32852][JUnit5 Migration] Migrate the scheduler package of flink-runtime module to JUnit5
Brief change log
[FLINK-32852][JUnit5 Migration] Migrate the scheduler package of flink-runtime module to JUnit5
Verifying this change
This change is already covered by existing tests, such as (please describe tests).
Does this pull request potentially affect one of the following parts:
@Public(Evolving)
: (yes / no)Documentation