You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by GitBox <gi...@apache.org> on 2022/06/07 16:15:05 UTC
[GitHub] [flink] RyanSkraba opened a new pull request, #19897: [FLINK-27885][tests][JUnit5 migration] flink-csv
RyanSkraba opened a new pull request, #19897:
URL: https://github.com/apache/flink/pull/19897
## What is the purpose of the change
Update the `flink-formats/flink-csv` module to AssertJ and JUnit 5 following the [JUnit 5 Migration Guide](https://docs.google.com/document/d/1514Wa_aNB9bJUen4xm5uiuXOooOJTtXqS_Jqk9KJitU/edit)
I used the https://github.com/slinkydeveloper/assertj-migrator as the starting point
Most of the AssertJ work was already finished on this module, with some exceptions around error handling.
There are some tests that still depend on JUnit4 test base classes outside this module. These cross-module tests should probably be simultaneously migrated in a separate PR.
I've verified that there are 132 tests run before and after the refactoring.
## Brief change log
* Removed dependences on JUnit 4, JUnit 5 Assertions and Hamcrest where possible.
## Verifying this change
This change is a code cleanup without any test coverage.
## Does this pull request potentially affect one of the following parts:
- Dependencies (does it add or upgrade a dependency): no
- The public API, i.e., is any changed class annotated with `@Public(Evolving)`: no
- The serializers: no
- The runtime per-record code paths (performance sensitive):no
- Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Kubernetes/Yarn, ZooKeeper: no
- The S3 file system connector: no
## Documentation
- Does this pull request introduce a new feature? no
## Brief change log
*(for example:)*
- *The TaskInfo is stored in the blob store on job creation time as a persistent artifact*
- *Deployments RPC transmits only the blob storage reference*
- *TaskManagers retrieve the TaskInfo from the blob cache*
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] RyanSkraba commented on pull request #19897: [FLINK-27885][tests][JUnit5 migration] flink-csv
Posted by "RyanSkraba (via GitHub)" <gi...@apache.org>.
RyanSkraba commented on PR #19897:
URL: https://github.com/apache/flink/pull/19897#issuecomment-1490420339
Hello! I've rebased this PR, did a quick check for changes that may have occurred on master in the meantime, and did some JIRA secretarial work for tracking the changes that were left for later. Thanks!
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] snuyanzin commented on pull request #19897: [FLINK-27885][tests][JUnit5 migration] flink-csv
Posted by GitBox <gi...@apache.org>.
snuyanzin commented on PR #19897:
URL: https://github.com/apache/flink/pull/19897#issuecomment-1285279850
Thanks for the contribution @RyanSkraba
I have some proposal to improve
1. It seems _org.apache.flink.formats.csv.CsvFilesystemBatchITCase_ is still not migrated
2. _org.apache.flink.formats.csv.CsvFormatFilesystemStatisticsReportTest_ looks migrated however modifiers could be hardened
3. _org.apache.flink.formats.csv.CsvFormatStatisticsReportTest_ is with junit5 however modifiers could be hardened
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] RyanSkraba commented on pull request #19897: [FLINK-27885][tests][JUnit5 migration] flink-csv
Posted by GitBox <gi...@apache.org>.
RyanSkraba commented on PR #19897:
URL: https://github.com/apache/flink/pull/19897#issuecomment-1287185674
Thanks for the review -- I've addressed the comments, except for `org.apache.flink.formats.csv.CsvFilesystemBatchITCase` which is based on a hierarchy of tests across modules that need to be migrated together.
I typically leave these to be migrated at the same time later (and in this case will likely be a big undertaking as there are at least 76 classes in the hierarchy).
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] snuyanzin commented on a diff in pull request #19897: [FLINK-27885][tests][JUnit5 migration] flink-csv
Posted by GitBox <gi...@apache.org>.
snuyanzin commented on code in PR #19897:
URL: https://github.com/apache/flink/pull/19897#discussion_r1006642754
##########
flink-formats/flink-csv/src/test/java/org/apache/flink/formats/csv/CsvFormatFilesystemStatisticsReportTest.java:
##########
@@ -34,16 +34,16 @@
/**
* Test for statistics functionality in {@link CsvFormatFactory} in the case of file system source.
*/
-public class CsvFormatFilesystemStatisticsReportTest extends CsvFormatStatisticsReportTest {
+class CsvFormatFilesystemStatisticsReportTest extends CsvFormatStatisticsReportTest {
+ @Override
@BeforeEach
public void setup(@TempDir File file) throws Exception {
Review Comment:
nit: since the hierarchy is in different packages package private does not work here however probably it should either changed to `protected` for the whole hierarchy or marked like `@VisibleForTestting`
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] flinkbot commented on pull request #19897: [FLINK-27885][tests][JUnit5 migration] flink-csv
Posted by GitBox <gi...@apache.org>.
flinkbot commented on PR #19897:
URL: https://github.com/apache/flink/pull/19897#issuecomment-1148892610
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "49c801402279376f3565f74fafa9b0791e225249",
"status" : "UNKNOWN",
"url" : "TBD",
"triggerID" : "49c801402279376f3565f74fafa9b0791e225249",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* 49c801402279376f3565f74fafa9b0791e225249 UNKNOWN
<details>
<summary>Bot commands</summary>
The @flinkbot bot supports the following commands:
- `@flinkbot run azure` re-run the last Azure build
</details>
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] snuyanzin commented on a diff in pull request #19897: [FLINK-27885][tests][JUnit5 migration] flink-csv
Posted by GitBox <gi...@apache.org>.
snuyanzin commented on code in PR #19897:
URL: https://github.com/apache/flink/pull/19897#discussion_r1005519182
##########
flink-formats/flink-csv/src/test/java/org/apache/flink/formats/csv/CsvRowDeSerializationSchemaTest.java:
##########
@@ -40,7 +40,7 @@
import static org.assertj.core.api.Assertions.assertThatThrownBy;
/** Tests for {@link CsvRowSerializationSchema} and {@link CsvRowDeserializationSchema}. */
-public class CsvRowDeSerializationSchemaTest {
+class CsvRowDeSerializationSchemaTest {
@Test
@SuppressWarnings("unchecked")
Review Comment:
This method could be also package private since now it's junit5
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] RyanSkraba commented on pull request #19897: [FLINK-27885][tests][JUnit5 migration] flink-csv
Posted by GitBox <gi...@apache.org>.
RyanSkraba commented on PR #19897:
URL: https://github.com/apache/flink/pull/19897#issuecomment-1285088453
Rebased with no additional code changes.
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] RyanSkraba commented on pull request #19897: [FLINK-27885][tests][JUnit5 migration] flink-csv
Posted by GitBox <gi...@apache.org>.
RyanSkraba commented on PR #19897:
URL: https://github.com/apache/flink/pull/19897#issuecomment-1310471015
Hello! I missed replying to this last comment -- my strategy for tests that are part of a large (or in this case, **very** large hierarchy) of related, cross-modules tests is to:
1. note it in the PR description and
2. go ahead with migrating as much as possible outside of those tests.
I don't see any satisfying incremental approach to cross-module JUnit tests otherwise.
I have no objection to leaving the JIRA open of course! Two PRs can address a single JIRA.
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] snuyanzin commented on pull request #19897: [FLINK-27885][tests][JUnit5 migration] flink-csv
Posted by GitBox <gi...@apache.org>.
snuyanzin commented on PR #19897:
URL: https://github.com/apache/flink/pull/19897#issuecomment-1291851895
thanks for addressing comments
I noticed that there is a bunch of classes still relying on junit4
`org.apache.flink.formats.csv.CsvFileCompactionITCase`
`org.apache.flink.formats.csv.CsvFilesystemBatchITCase`
`org.apache.flink.formats.csv.CsvFilesystemStreamITCase`
`org.apache.flink.formats.csv.CsvFilesystemStreamSinkITCase`
because they depend on classes/hierarchies from other modules ...
I wonder if in this case there should be a dedicated follow up task created to fix that...
Because currently we can not say that flink-csv is moved to junit5...
WDYT?
Also cc @XComp may be you have some clue about that?
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] RyanSkraba commented on pull request #19897: [FLINK-27885][tests][JUnit5 migration] flink-csv
Posted by GitBox <gi...@apache.org>.
RyanSkraba commented on PR #19897:
URL: https://github.com/apache/flink/pull/19897#issuecomment-1397317096
Hello! I've rebased this PR to master. I don't believe there are any remaining requested changes that I haven't addressed (by comment or other).
I will be mostly away from my computer for a couple of weeks, but I'll check in if anything changes!
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] snuyanzin commented on a diff in pull request #19897: [FLINK-27885][tests][JUnit5 migration] flink-csv
Posted by GitBox <gi...@apache.org>.
snuyanzin commented on code in PR #19897:
URL: https://github.com/apache/flink/pull/19897#discussion_r1006642754
##########
flink-formats/flink-csv/src/test/java/org/apache/flink/formats/csv/CsvFormatFilesystemStatisticsReportTest.java:
##########
@@ -34,16 +34,16 @@
/**
* Test for statistics functionality in {@link CsvFormatFactory} in the case of file system source.
*/
-public class CsvFormatFilesystemStatisticsReportTest extends CsvFormatStatisticsReportTest {
+class CsvFormatFilesystemStatisticsReportTest extends CsvFormatStatisticsReportTest {
+ @Override
@BeforeEach
public void setup(@TempDir File file) throws Exception {
Review Comment:
nit: since the hierarchy is in different packages package private does not work here however probably it could be either changed to `protected` for the whole hierarchy or marked like `@VisibleForTestting`
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] snuyanzin commented on a diff in pull request #19897: [FLINK-27885][tests][JUnit5 migration] flink-csv
Posted by GitBox <gi...@apache.org>.
snuyanzin commented on code in PR #19897:
URL: https://github.com/apache/flink/pull/19897#discussion_r1000385236
##########
flink-formats/flink-csv/src/test/java/org/apache/flink/formats/csv/CsvFormatFactoryTest.java:
##########
@@ -58,13 +56,14 @@
import static org.apache.flink.table.factories.utils.FactoryMocks.createTableSink;
import static org.apache.flink.table.factories.utils.FactoryMocks.createTableSource;
import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
/** Tests for {@link CsvFormatFactory}. */
-public class CsvFormatFactoryTest extends TestLogger {
- @Rule public ExpectedException thrown = ExpectedException.none();
+@ExtendWith({TestLoggerExtension.class})
Review Comment:
Probably using the `META-INF/services/org.junit.jupiter.api.extension.Extension` is the better approach here
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] RyanSkraba commented on a diff in pull request #19897: [FLINK-27885][tests][JUnit5 migration] flink-csv
Posted by GitBox <gi...@apache.org>.
RyanSkraba commented on code in PR #19897:
URL: https://github.com/apache/flink/pull/19897#discussion_r1019286172
##########
flink-formats/flink-csv/src/test/java/org/apache/flink/formats/csv/CsvFormatFilesystemStatisticsReportTest.java:
##########
@@ -34,16 +34,16 @@
/**
* Test for statistics functionality in {@link CsvFormatFactory} in the case of file system source.
*/
-public class CsvFormatFilesystemStatisticsReportTest extends CsvFormatStatisticsReportTest {
+class CsvFormatFilesystemStatisticsReportTest extends CsvFormatStatisticsReportTest {
+ @Override
@BeforeEach
public void setup(@TempDir File file) throws Exception {
Review Comment:
I guess this is true, but I'd prefer to avoid `@VisibleForTesting` for classes in the `src/test/java` hierarchy... There are a few places in the Flink code where I've seen this, but it's pretty rare and probably not what the contributor really intended!
I'll change it to protected.
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] snuyanzin commented on a diff in pull request #19897: [FLINK-27885][tests][JUnit5 migration] flink-csv
Posted by "snuyanzin (via GitHub)" <gi...@apache.org>.
snuyanzin commented on code in PR #19897:
URL: https://github.com/apache/flink/pull/19897#discussion_r1153486137
##########
flink-clients/src/test/resources/META-INF/services/org.junit.jupiter.api.extension.Extension:
##########
@@ -14,3 +14,4 @@
# limitations under the License.
org.apache.flink.util.TestLoggerExtension
+org.junit.jupiter.api.extension.Extension
Review Comment:
This one I didn't get.
Should it be changed within `flink-csv` changes?
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] snuyanzin commented on a diff in pull request #19897: [FLINK-27885][tests][JUnit5 migration] flink-csv
Posted by "snuyanzin (via GitHub)" <gi...@apache.org>.
snuyanzin commented on code in PR #19897:
URL: https://github.com/apache/flink/pull/19897#discussion_r1153473868
##########
flink-formats/flink-csv/src/test/java/org/apache/flink/formats/csv/CsvFormatStatisticsReportTest.java:
##########
@@ -61,7 +62,7 @@ protected String[] properties() {
}
@Test
- public void testCsvFormatStatsReportWithSingleFile() throws Exception {
+ void testCsvFormatStatsReportWithSingleFile() throws Exception {
// insert data and get statistics.
DataType dataType = tEnv.from("sourceTable").getResolvedSchema().toPhysicalRowDataType();
tEnv.fromValues(dataType, getData()).executeInsert("sourceTable").await();
Review Comment:
this is not related to your changes however it relates to assertj.
In the next line
```java
assertThat(folder.listFiles()).isNotNull().hasSize(1);
```
`isNotNull()` is redundant check since same check is done inside `hasSize`
Also it's better to replace `assert files != null;` with assertj's version
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] snuyanzin commented on a diff in pull request #19897: [FLINK-27885][tests][JUnit5 migration] flink-csv
Posted by "snuyanzin (via GitHub)" <gi...@apache.org>.
snuyanzin commented on code in PR #19897:
URL: https://github.com/apache/flink/pull/19897#discussion_r1153475324
##########
flink-formats/flink-csv/src/test/java/org/apache/flink/formats/csv/CsvFormatStatisticsReportTest.java:
##########
@@ -75,7 +76,7 @@ public void testCsvFormatStatsReportWithSingleFile() throws Exception {
}
@Test
- public void testCsvFormatStatsReportWithMultiFile() throws Exception {
+ void testCsvFormatStatsReportWithMultiFile() throws Exception {
// insert data and get statistics.
DataType dataType = tEnv.from("sourceTable").getResolvedSchema().toPhysicalRowDataType();
tEnv.fromValues(dataType, getData()).executeInsert("sourceTable").await();
Review Comment:
this is not related to your changes however it relates to assertj.
In the next line
```java
assertThat(folder.listFiles()).isNotNull().hasSize(1);
```
`isNotNull()` is redundant check since same check is done inside `hasSize`
Also it's better to replace `assert files != null;` with assertj's version
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] XComp commented on pull request #19897: [FLINK-27885][tests][JUnit5 migration] flink-csv
Posted by GitBox <gi...@apache.org>.
XComp commented on PR #19897:
URL: https://github.com/apache/flink/pull/19897#issuecomment-1292017835
Theoretically, we might want to link FLINK-27885 with FLINK-29541. I guess, mentioning in FLINK-29541 that files in other modules need to be migrated as well is sufficient enough. :thinking:
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] RyanSkraba commented on a diff in pull request #19897: [FLINK-27885][tests][JUnit5 migration] flink-csv
Posted by GitBox <gi...@apache.org>.
RyanSkraba commented on code in PR #19897:
URL: https://github.com/apache/flink/pull/19897#discussion_r1019286172
##########
flink-formats/flink-csv/src/test/java/org/apache/flink/formats/csv/CsvFormatFilesystemStatisticsReportTest.java:
##########
@@ -34,16 +34,16 @@
/**
* Test for statistics functionality in {@link CsvFormatFactory} in the case of file system source.
*/
-public class CsvFormatFilesystemStatisticsReportTest extends CsvFormatStatisticsReportTest {
+class CsvFormatFilesystemStatisticsReportTest extends CsvFormatStatisticsReportTest {
+ @Override
@BeforeEach
public void setup(@TempDir File file) throws Exception {
Review Comment:
I guess this is true, but I'd prefer to avoid `@VisibleForTesting` for classes in the `src/test/java` hierarchy... There are a few places in the Flink code where I've seen this, but it's pretty rare and probably not what the contributor really intended!
~I'll change it to protected.~ Sorry, I misread -- this can't be changed to protected without changing the entire hierarchy of tests that depend on it across many modules.
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org