-
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-25228][table] Introduce flink-table-test-utils #18255
Conversation
Signed-off-by: slinkydeveloper <francescoguard@gmail.com>
…gh flink-table-test-utils Signed-off-by: slinkydeveloper <francescoguard@gmail.com>
Thanks a lot for your contribution to the Apache Flink project. I'm the @flinkbot. I help the community Automated ChecksLast check on commit 0fad0a9 (Mon Jan 03 08:22:36 UTC 2022) Warnings:
Mention the bot in a comment to re-run the automated checks. Review Progress
Please see the Pull Request Review Guide for a full explanation of the review process. The Bot is tracking the review progress through labels. Labels are applied according to the order of the review items. For consensus, approval by a Flink committer of PMC member is required Bot commandsThe @flinkbot bot supports the following commands:
|
0fad0a9
to
67b6ef7
Compare
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 for the PR @slinkydeveloper. I totally support this PR but I would like to remove confusion in the code base and duplicate code. Let's move classes from table-common
and other locations to the new test utils package. If it is only about DataStructureConverter
we can also perform a class lookup whether flink-table-runtime
is present or not. That should be fine for test utils.
</dependency> | ||
|
||
<!-- Required for the assertions --> | ||
<dependency> |
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 already comes in through flink-test-utils-junit
.
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.
flink-test-utils
pulls flink-test-utils-junit
, but we need to redeclare it here as the parent pom somewhere declares it with scope test, so we need to override the scope here. Removing flink-test-utils-junit
returns this:
[INFO] org.apache.flink:flink-table-test-utils:jar:1.15-SNAPSHOT
[INFO] +- org.apache.flink:flink-test-utils:jar:1.15-SNAPSHOT:compile
[INFO] | +- org.apache.flink:flink-test-utils-junit:jar:1.15-SNAPSHOT:test
<artifactId>flink-table-test-utils</artifactId> | ||
<name>Flink : Table : Test Utils</name> | ||
<description> | ||
This module contains test utilities for the Table API/SQL ecosystem. |
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.
Maybe document the intention as well: In the future we plan to expose this module to users. It should not contain internal/very specialized testing utilities
.
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.
I doubled down, check now :)
flink-table/flink-table-common/src/test/java/org/apache/flink/table/test/DataTypeAssert.java
Show resolved
Hide resolved
flink-table/flink-table-test-utils/src/test/java/TableAssertionTest.java
Show resolved
Hide resolved
@twalthr the reason for placing the classes in two places is to continue to use the asserts in table-common test classpath as well.
That's something I can do with some reflections. This way, we can still use the assertions within table-common (without some methods that requires the runtime dep), and then we don't have duplicated code. But we'll still need the shading. Is it ok for you? |
1fb27bd
to
ae781c9
Compare
@twalthr ready for another pass |
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 for the update @slinkydeveloper. We still need to remove the duplicate TableFactoryHarness
.
...able-planner/src/test/java/org/apache/flink/table/planner/factories/TableFactoryHarness.java
Outdated
Show resolved
Hide resolved
...able-test-utils/src/main/java/org/apache/flink/table/test/factories/TableFactoryHarness.java
Outdated
Show resolved
Hide resolved
Signed-off-by: slinkydeveloper <francescoguard@gmail.com>
Signed-off-by: slinkydeveloper <francescoguard@gmail.com>
Signed-off-by: slinkydeveloper <francescoguard@gmail.com>
ae781c9
to
8ec0cfb
Compare
@twalthr as discussed, I've removed the table factory harness |
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.
LGTM
What is the purpose of the change
Goal of this new module is to provide a easy to use module for testing formats, connectors and end users.
The module brings in transitively all the dependencies you need to run a table test and includes a couple of test utilities.
Brief change log
flink-table-test-utils
, shipping the existing table assertions andTableFactoryHarness
. Both assertions andTableFactoryHarness
are marked as@Experimental
Verifying this change
The newly introduced assertions are tested in a new test, showing how to use them.
Does this pull request potentially affect one of the following parts:
@Public(Evolving)
: noDocumentation