[FLINK-26232][tests][JUnit5 migration] Module:flink-avro #18871

RyanSkraba · 2022-02-22T09:19:31Z

What is the purpose of the change

Update the flink-formats/flink-avro module to AssertJ and JUnit 5 following the JUnit 5 Migration Guide

The migration of this module isn't fully finished -- notably those that depend on test base classes outside this module, which I either left untouched or just did the AssertJ changes if they could be done independently.

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

[FLINK-26232][tests][JUnit5 migration] Module:flink-avro AvroTypeExtractionTest

flinkbot · 2022-02-22T09:23:13Z

Thanks a lot for your contribution to the Apache Flink project. I'm the @flinkbot. I help the community
to review your pull request. We will use this comment to track the progress of the review.

Automated Checks

Last check on commit 6a31168 (Tue Feb 22 09:23:13 UTC 2022)

Warnings:

No documentation files were touched! Remember to keep the Flink docs up to date!

_{Mention the bot in a comment to re-run the automated checks.}

Review Progress

❓ 1. The [description] looks good.
❓ 2. There is [consensus] that the contribution should go into to Flink.
❓ 3. Needs [attention] from.
❓ 4. The change fits into the overall [architecture].
❓ 5. Overall code [quality] is good.

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 commands

The @flinkbot bot supports the following commands:

@flinkbot approve description to approve one or more aspects (aspects: description, consensus, architecture and quality)
@flinkbot approve all to approve all aspects
@flinkbot approve-until architecture to approve everything until architecture
@flinkbot attention @username1 [@username2 ..] to require somebody's attention
@flinkbot disapprove architecture to remove an approval you gave earlier

flinkbot · 2022-02-22T09:23:13Z

CI report:

7da0f26 Azure: SUCCESS

Bot commands

The @flinkbot bot supports the following commands:

@flinkbot run azure re-run the last Azure build

…ng public. Extra assertJ cleanup on some tests.

RyanSkraba · 2022-02-22T09:23:58Z

...k-formats/flink-avro/src/test/java/org/apache/flink/formats/avro/AvroOutputFormatITCase.java

@@ -46,6 +45,8 @@
 import java.util.List;
 import java.util.Objects;

+import static org.assertj.core.api.Assertions.assertThat;
+
 /** IT cases for the {@link AvroOutputFormat}. */
 @SuppressWarnings("serial")
 public class AvroOutputFormatITCase extends JavaProgramTestBase {


This is an example of a class that moved to AssertJ matchers while leaving the base class still uses JUnit 4 to run. The public modifiers are still necessary, but fortunately there's nothing else JUnit4 to change here.

flink-formats/flink-avro/src/test/java/org/apache/flink/formats/avro/AvroOutputFormatTest.java

RyanSkraba · 2022-02-22T09:44:45Z

...k-formats/flink-avro/src/test/java/org/apache/flink/formats/avro/AvroOutputFormatITCase.java

@@ -99,8 +100,7 @@ protected void postSubmit() throws Exception {
            output1 = file1.listFiles();
            // check for avro ext in dir.
            for (File avroOutput : Objects.requireNonNull(output1)) {
-                Assert.assertTrue(
-                        "Expect extension '.avro'", avroOutput.toString().endsWith(".avro"));
+                assertThat(avroOutput.toString()).endsWith(".avro");


AssertJ does nice enough descriptions, so I removed a few throughout the code. This, for example, produces:

Expecting actual: "/tmp/junit5763721249197813393/junit1392834020197511051/avro_output1/4.avrox" to end with: ".avro"

flink-formats/flink-avro/src/test/java/org/apache/flink/formats/avro/AvroOutputFormatTest.java

RyanSkraba · 2022-02-22T09:59:00Z

...k-avro/src/test/java/org/apache/flink/formats/avro/AvroRowDataDeSerializationSchemaTest.java

-            serializationSchema.serialize(rowData);
-            fail("expecting exception message: " + errorMessage);
-        } catch (Throwable t) {
-            assertThat(t, FlinkMatchers.containsMessage(errorMessage));


Ooooh, found a bug here: rowData is actually serialized correctly , but the fail call afterwards throws an assertion exception (containing the expected error message by definition). I suspect that the setField(1, 2) is what was intended, actually testing a type mismatch.

The withStackTraceContaining looks like a nice AssertJ replacement for finding messages in the cause exceptions.

...k-avro/src/test/java/org/apache/flink/formats/avro/typeutils/AvroSerializerSnapshotTest.java

RyanSkraba · 2022-02-22T10:37:46Z

...flink-avro/src/test/java/org/apache/flink/formats/avro/typeutils/AvroTypeExtractionTest.java

-    public AvroTypeExtractionTest(TestExecutionMode mode) {
-        super(mode);
+/** Tests for the {@link AvroInputFormat} reading Pojos. */
+class AvroTypeExtractionTest {


This was the most "complicated" test in the module (that I attempted):

Removed base class to provide 2 ExecutionEnvironment instance through a JUnit 5 parameterization, and a MiniClusterExtension directly in the class.

The call to teardown in the base class no longer occurs.

It looks like there's no impact: since the ExecutionEvironment isn't set as context, it doesn't need to be unset?

...ts/flink-avro/src/test/java/org/apache/flink/formats/avro/AvroSplittableInputFormatTest.java

...ormats/flink-avro/src/test/java/org/apache/flink/formats/avro/AvroRecordInputFormatTest.java

...k-avro/src/test/java/org/apache/flink/formats/avro/typeutils/AvroSerializerSnapshotTest.java

...flink-avro/src/test/java/org/apache/flink/formats/avro/typeutils/AvroTypeExtractionTest.java

Moved to assertThatThrownBy for exception testing Some additional cleanup for best AssertJ practices Use containsOnly for Maps

RyanSkraba · 2022-02-22T12:22:00Z

Thanks for the review, let me know if you have any advice for the next step: would it be better to start migrating the base classes so in order to "finish" flink-avro, or to attack the "low-hanging" fruit in other modules?

zentol · 2022-03-09T11:35:11Z

@RyanSkraba I'd focus on low-hanging fruits for now (i.e., where we can migrate an entire module at once); the core modules such as flink-runtime will likely have to migrated piece-by-piece.

fsk119 · 2022-03-10T03:53:43Z

@flinkbot run azure

In my local environment, failed to compile AvroTypeExractionTest.

[FLINK-26232][tests][JUnit5 migration] Module:flink-avro

6a31168

[FLINK-26232][tests][JUnit5 migration] Module:flink-avro AvroTypeExtractionTest

rmetzger added the component=<none> label Feb 22, 2022

FLINK-26232][tests][JUnit5 migration] Module:flink-avro Restore missi…

85833a7

…ng public. Extra assertJ cleanup on some tests.

RyanSkraba commented Feb 22, 2022

View reviewed changes

zentol reviewed Feb 22, 2022

View reviewed changes

FLINK-26232][tests][JUnit5 migration] Module:flink-avro PR feedback

7da0f26

Moved to assertThatThrownBy for exception testing Some additional cleanup for best AssertJ practices Use containsOnly for Maps

RyanSkraba requested a review from zentol February 22, 2022 15:06

zentol merged commit a3dff68 into apache:master Mar 9, 2022

RyanSkraba deleted the rskraba/FLINK-26232-flink-avro-junit branch March 9, 2022 12:36

RyanSkraba restored the rskraba/FLINK-26232-flink-avro-junit branch March 9, 2022 17:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FLINK-26232][tests][JUnit5 migration] Module:flink-avro #18871

[FLINK-26232][tests][JUnit5 migration] Module:flink-avro #18871

RyanSkraba commented Feb 22, 2022

flinkbot commented Feb 22, 2022

flinkbot commented Feb 22, 2022 •

edited

RyanSkraba Feb 22, 2022

RyanSkraba Feb 22, 2022

RyanSkraba Feb 22, 2022

RyanSkraba Feb 22, 2022

RyanSkraba commented Feb 22, 2022

zentol commented Mar 9, 2022 •

edited

fsk119 commented Mar 10, 2022 •

edited

[FLINK-26232][tests][JUnit5 migration] Module:flink-avro #18871

[FLINK-26232][tests][JUnit5 migration] Module:flink-avro #18871

Conversation

RyanSkraba commented Feb 22, 2022

What is the purpose of the change

Brief change log

Verifying this change

Does this pull request potentially affect one of the following parts:

Documentation

flinkbot commented Feb 22, 2022

Automated Checks

Review Progress

flinkbot commented Feb 22, 2022 • edited

CI report:

RyanSkraba Feb 22, 2022

Choose a reason for hiding this comment

RyanSkraba Feb 22, 2022

Choose a reason for hiding this comment

RyanSkraba Feb 22, 2022

Choose a reason for hiding this comment

RyanSkraba Feb 22, 2022

Choose a reason for hiding this comment

RyanSkraba commented Feb 22, 2022

zentol commented Mar 9, 2022 • edited

fsk119 commented Mar 10, 2022 • edited

flinkbot commented Feb 22, 2022 •

edited

zentol commented Mar 9, 2022 •

edited

fsk119 commented Mar 10, 2022 •

edited