[FLINK-27885][tests][JUnit5 migration] flink-csv

[RyanSkraba]

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

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

[flinkbot]

CI report:

• ce69d92 UNKNOWN
• 2bdf90e Azure: SUCCESS

Bot commands

The @flinkbot bot supports the following commands:

• @flinkbot run azure re-run the last Azure build

[RyanSkraba]

Rebased with no additional code changes.

[snuyanzin]

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

[RyanSkraba]

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).

[snuyanzin]

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?

[XComp]

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. 🤔

[snuyanzin]

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

[RyanSkraba]

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.

[RyanSkraba]

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.

[RyanSkraba]

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!

[RyanSkraba]

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!