-
Notifications
You must be signed in to change notification settings - Fork 13k
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-33853][runtime][JUnit5 Migration] Migrate Junit5 for DeclarativeSlotPoolBridge test classes of runtime module #23932
Conversation
…veSlotPoolBridge test classes of runtime module
Hi, @1996fanrui Could you help review this pr if you're in 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.
Thanks @RocMarshal for the contribution!
I left some minor comments, please take a look in your free time, thanks~
@@ -45,10 +45,10 @@ | |||
import static org.assertj.core.api.Assertions.assertThat; | |||
|
|||
@ExtendWith(TestLoggerExtension.class) |
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.
@ExtendWith(TestLoggerExtension.class) |
TestLoggerExtension
has been added in the module level.
|
||
/** Tests how the {@link DeclarativeSlotPoolBridge} completes slot requests. */ | ||
public class DeclarativeSlotPoolBridgeRequestCompletionTest extends TestLogger { | ||
@ExtendWith(TestLoggerExtension.class) |
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.
@ExtendWith(TestLoggerExtension.class) |
|
||
/** Tests for the {@link DeclarativeSlotPoolBridge}. */ | ||
@RunWith(Parameterized.class) | ||
public class DeclarativeSlotPoolBridgeResourceDeclarationTest extends TestLogger { | ||
@ExtendWith({TestLoggerExtension.class, ParameterizedTestExtension.class}) |
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.
@ExtendWith({TestLoggerExtension.class, ParameterizedTestExtension.class}) | |
@ExtendWith(ParameterizedTestExtension.class) |
FlinkMatchers.futureWillCompleteExceptionally(Duration.ofMinutes(1))); | ||
|
||
assertThatFuture(allocationFuture).failsWithin(Duration.ofMinutes(1)); | ||
Thread.sleep(5L); |
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.
Why is it needed here?
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.
Nice catch!
sorry for it. Maybe it's a dirty line from the cp.
I'll remove it and check it on the other corresponding PRs.
|
||
/** Tests for the {@link DeclarativeSlotPoolBridge}. */ | ||
@RunWith(Parameterized.class) | ||
public class DeclarativeSlotPoolBridgeTest extends TestLogger { | ||
@ExtendWith({TestLoggerExtension.class, ParameterizedTestExtension.class}) |
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.
@ExtendWith({TestLoggerExtension.class, ParameterizedTestExtension.class}) | |
@ExtendWith(ParameterizedTestExtension.class) |
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 assuming CI is green.
What is the purpose of the change
[FLINK-33853][runtime][JUnit5 Migration] Migrate Junit5 for DeclarativeSlotPoolBridge test classes of runtime module
Brief change log
Verifying this change
N.A
Does this pull request potentially affect one of the following parts:
@Public(Evolving)
: (yes / no)Documentation