[FLINK-39876][tests] Migrate flink-streaming-java assertions to AssertJ - #28835
Closed
spuru9 wants to merge 1 commit into
Closed
[FLINK-39876][tests] Migrate flink-streaming-java assertions to AssertJ#28835spuru9 wants to merge 1 commit into
spuru9 wants to merge 1 commit into
Conversation
spuru9
marked this pull request as draft
July 28, 2026 16:36
Collaborator
Contributor
Author
|
As per the https://flink.apache.org/how-to-contribute/code-style-and-quality-common/#7-testing |
spuru9
force-pushed
the
feature/junit5-streaming-java-assertj
branch
from
July 28, 2026 17:38
08a6354 to
d30fea1
Compare
spuru9
marked this pull request as ready for review
July 28, 2026 17:42
The contributor guide asks for JUnit 5 and AssertJ and states "Don't use Hamcrest, JUnit assertions and `assert` directive". flink-streaming-java was already on JUnit 5 (FLINK-25544), but ten test classes still reached for Hamcrest. This removes the last of them. * TypeSafeMatcher and FeatureMatcher instances that were bridged into AssertJ via HamcrestCondition.matching() become native AssertJ Conditions. * MockitoHamcrest.argThat() becomes Mockito's own ArgumentMatchers.argThat(). The Hamcrest matchers it wrapped (containsInAnyOrder, contains, hasEntry, allOf) are expressed as predicates, which also removes the casts their call sites needed. CheckpointExceptionMatcher now implements Mockito's ArgumentMatcher rather than Hamcrest's BaseMatcher. * Drops the unused EqualsResourceSpecMatcher from StreamGraphGeneratorTest. The replacements keep the semantics of the matchers they replace, which AssertJ and Mockito do not provide for free: the predicates reject null explicitly, because TypeSafeMatcher never invoked matchesSafely() on a null actual; containsInAnyOrder counts duplicates; and the map matcher checks for the expected entries rather than map equality, as hasEntry did. No production code is touched. Each migrated predicate was inverted in turn to confirm the tests using it fail. Generated-by: Claude Code (Claude Opus 5)
spuru9
force-pushed
the
feature/junit5-streaming-java-assertj
branch
from
July 28, 2026 18:31
d30fea1 to
0efae72
Compare
Contributor
Author
|
cc: @snuyanzin @raminqaf |
snuyanzin
reviewed
Aug 1, 2026
Comment on lines
+220
to
+234
| private static Collection<TimeWindow> containsInAnyOrder(TimeWindow... windows) { | ||
| return argThat( | ||
| actual -> { | ||
| if (actual == null) { | ||
| return false; | ||
| } | ||
| final List<TimeWindow> remaining = new ArrayList<>(actual); | ||
| for (TimeWindow window : windows) { | ||
| if (!remaining.remove(window)) { | ||
| return false; | ||
| } | ||
| } | ||
| return remaining.isEmpty(); | ||
| }); | ||
| } |
Contributor
There was a problem hiding this comment.
It does not look like a solution
we have multiple tests using this method (hamcrest's containsInAnyOrder).
How should we continue there? Create same helper for every class instead of hamcrest?
Please do not blindly trust AI
snuyanzin
reviewed
Aug 1, 2026
| * Matches a collection containing exactly the expected windows, in any order, counting | ||
| * duplicates. | ||
| */ | ||
| private static Collection<TimeWindow> containsInAnyOrder(TimeWindow... windows) { |
Contributor
There was a problem hiding this comment.
snuyanzin
requested changes
Aug 1, 2026
snuyanzin
left a comment
snuyanzin
left a comment
Contributor
There was a problem hiding this comment.
given this approach https://github.com/apache/flink/pull/28835/changes#r3695518844
I would prefer existing code since no need to have code duplication for every usage of hamcrest method
Contributor
Author
|
Ok, Got it. Picked it as the wiki not discourages Hamcrest. |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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
flink-streaming-javais already on JUnit 5 (FLINK-25544), but ten test classes still usedHamcrest — either bridged into AssertJ via
HamcrestCondition.matching(...)or through theMockitoHamcrestargument matchers. This removes the last of them, per§7 Testing:
"Don't use Hamcrest, JUnit assertions and
assertdirective". Test-only; no production codeis touched.
Brief change log
TypeSafeMatcher/FeatureMatcherwrapped inHamcrestCondition.matching(...)become nativeAssertJ
Conditions.MockitoHamcrest.argThat(...)becomesArgumentMatchers.argThat(...), with the Hamcrestmatchers it wrapped expressed as predicates;
CheckpointExceptionMatchernow implementsMockito's
ArgumentMatcherinstead of Hamcrest'sBaseMatcher.EqualsResourceSpecMatcherfromStreamGraphGeneratorTest.Verifying this change
This change is a trivial rework / code cleanup without any test coverage.
Covered by the tests it touches (99 tests, passing). To confirm no assertion became vacuously
true, each new predicate was inverted in turn and the tests using it failed as expected.
Checkstyle and Spotless are clean.
Does this pull request potentially affect one of the following parts:
@Public(Evolving): noDocumentation
Was generative AI tooling used to co-author this PR?
Generated-by: Claude Code (Claude Opus 5)