[FLINK-27970][tests][JUnit5 migration] flink-hadoop-bulk #19970
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
RyanSkraba
force-pushed
the
FLINK-27970-flink-hadoop-bulk-junit5
branch
from
df24747
to
49ff7d3
Compare
RyanSkraba
changed the title
|
@flinkbot run azure |
RyanSkraba
force-pushed
the
FLINK-27970-flink-hadoop-bulk-junit5
branch
from
49ff7d3
to
1bcdaa4
Compare
RyanSkraba
force-pushed
the
FLINK-27970-flink-hadoop-bulk-junit5
branch
from
1bcdaa4
to
a4cf5e4
Compare
|
Hello! I've rebased this PR to master and fixed the merge conflicts in the meantime. I will be mostly away from my computer for a couple of weeks, but I'll check in if anything changes! |
XComp
reviewed
Feb 10, 2023
flink-formats/flink-hadoop-bulk/archunit-violations/db4de53e-d09e-4fb0-bdbc-429c1b64686f
Outdated
Show resolved
Hide resolved
RyanSkraba
force-pushed
the
FLINK-27970-flink-hadoop-bulk-junit5
branch
from
a4cf5e4
to
27712b0
Compare
snuyanzin
reviewed
Aug 24, 2023
...ache/flink/streaming/api/functions/sink/filesystem/HadoopPathBasedBulkFormatBuilderTest.java
Outdated
Show resolved
Hide resolved
RyanSkraba
force-pushed
the
FLINK-27970-flink-hadoop-bulk-junit5
branch
from
b2c8bbe
to
19cb84e
Compare
snuyanzin
reviewed
Feb 21, 2024
| @@ -232,7 +227,7 @@ private void verifyFileNotExists( | |||
| FileSystem fileSystem = FileSystem.get(basePath.toUri(), configuration); | |||
| for (String targetFileName : targetFileNames) { | |||
| assertThat(fileSystem.exists(new Path(basePath, targetFileName))) | |||
| .as("Pre-committed file should not exists: " + targetFileName) | |||
| .as("Pre-committed file should not exist: " + targetFileName) | |||
There was a problem hiding this comment.
very nit:
Suggested change
| .as("Pre-committed file should not exist: " + targetFileName) | |
| .as(() -> "Pre-committed file should not exist: " + targetFileName) |
we can use suppliers in order to concat string only in case of failure and do not dot it for success cases
There was a problem hiding this comment.
interesting feature. I didn't notice that before. But keep in mind two things when writing code:
- There's also a trade-off between readability and performance improvement: Not every performance-related change is needed/useful. Here we have a test implementation where we don't need to worry too much about the performance of a String operation in case of a failure. Readability (i.e. less code) might be preferable here because the additional code snippet doesn't add any value. ...at least as far as I can see.
- "Workarounds" that are way more "uncommon" in the code base might cause hotfix PRs later on by contributors who do not know this particular feature (which increases the review efforts on the committers' side or, at least, increases the projects PR count). I see that specific risk for this change proposal here. But to be fair, that's a subjective judgement on my end.
There was a problem hiding this comment.
Agree about the points you've mentioned
I suggested that since it seems it is becoming a common approach, e.g. similar things are quite often in Calcite and othe projects...
anyway I'm not insisting on this, up to @RyanSkraba
snuyanzin
reviewed
Feb 21, 2024
| @@ -245,7 +240,7 @@ private void verifyCommittedFiles( | |||
| for (String targetFileName : targetFileNames) { | |||
| Path targetFilePath = new Path(basePath, targetFileName); | |||
| assertThat(fileSystem.exists(targetFilePath)) | |||
| .as("Committed file should exists: " + targetFileName) | |||
| .as("Committed file should exist: " + targetFileName) | |||
There was a problem hiding this comment.
Suggested change
| .as("Committed file should exist: " + targetFileName) | |
| .as(() -> "Committed file should exist: " + targetFileName) |
we can use suppliers in order to concat string only in case of failure and do not dot it for success cases
snuyanzin
reviewed
Feb 21, 2024
| @Test | ||
| @Ignore | ||
| @Disabled | ||
| public void prepareDeserialization() throws IOException { |
There was a problem hiding this comment.
should we also harden modifier here?
|
In general it looks good to me I left some minor comments |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What is the purpose of the change
Update the
flink-formats/flink-hadoop-bulkmodule to AssertJ and JUnit 5 following the JUnit 5 Migration GuideBrief change log
Verifying this change
This change is a trivial rework / code cleanup without any test coverage.
I've verified that there are 31 tests run in this module before and after the refactoring.
Does this pull request potentially affect one of the following parts:
@Public(Evolving): noDocumentation