-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Create JUnit5 version of FlinkTestBase #9120
Conversation
cgpoh
commented
Nov 21, 2023
- Add FlinkTestBaseJU5 and MiniClusterResourceJU5 class
- Fixes Create JUnit5-version of FlinkTestBase #9078
- Add FlinkTestBaseJU5 and MiniClusterResourceJU5 class - Fixes apachegh-9078
@cgpoh: We usually only create a PR for a single version of Flink (latest). After that PR is merged, then we create a backport PR to get the changes to the older versions. This helps the reviewer (smaller number of changes), and the contributor (if changes was requested then it is a smaller work) too. Please avoid having the J5 in the class names - we would like to remove the old classes, and after that J5 classes will be hard to understand. Create a new package? Find a better name for the classes? Whatever works - but keep in mind the goal 😄 Please migrate at least one test class to the new base package, so we can try things out. I have found that |
@pvary , thanks! Will update the class name and revert the changes for Flink 1.15 and 1.16 |
import static org.junit.jupiter.api.Assertions.assertTrue; | ||
|
||
/** Test for {@link TableLoader}. */ | ||
public class TestCatalogTblLoader extends FlinkTestBaseUtils { |
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.
This can be an update of the original TestCatalogTableLoader
class, just instead of using the old FlinkTestBase
, extend the new FlinkTestBaseUtils
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.
So use the old class here, and extend the new base class. So it is easier to see, that all of the tests are the same, and also we do not need 2 test class for the same feature
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.
+1 to that. We don't want to duplicate test classes themselves, but rather convert them from their Junit4 base class to their new Junit5 base class (once the new base class exists)
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! Will update the old class instead
import org.junit.jupiter.api.extension.RegisterExtension; | ||
import org.junit.jupiter.api.io.TempDir; | ||
|
||
public abstract class FlinkTestBaseUtils extends TestBaseUtils { |
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.
Could this be only TestBase
, or ClusterTestBase
? Or something similar?
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.
+1 to using TestBase
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.
Thanks! Renaming to TestBase
…bleLoader to use the new TestBase
@cgpoh: Please run |
@pvary will do. Thanks! |
import org.apache.flink.runtime.testutils.MiniClusterResourceConfiguration; | ||
import org.apache.flink.test.junit5.MiniClusterExtension; | ||
|
||
public class MiniFlinkClusterResource { |
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.
nit: Do you mind renaming this to MiniClusterExtension
?
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.
@pvary , I think I rename to MiniFlinkClusterExtension
instead. If I rename to MiniClusterExtension
, the following method will need to import MiniClusterExtension
explicitly as below:
public static org.apache.flink.test.junit5.MiniClusterExtension createWithClassloaderCheckDisabled() {
return new org.apache.flink.test.junit5.MiniClusterExtension(
new MiniClusterResourceConfiguration.Builder()
.setNumberTaskManagers(DEFAULT_TM_NUM)
.setNumberSlotsPerTaskManager(DEFAULT_PARALLELISM)
.setConfiguration(DISABLE_CLASSLOADER_CHECK_CONFIG)
.build());
}
what do you think?
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.
Agreed
Thanks! Will create the PR later |