[FLINK-39770][tests][JUnit5 migration] Module: flink-state-processing-api#28561
[FLINK-39770][tests][JUnit5 migration] Module: flink-state-processing-api#28561spuru9 wants to merge 1 commit into
Conversation
spuru9
force-pushed
the
feature/junit5-state-processing-api
branch
from
f7b4dfc to
eded373
Compare
| @@ -67,7 +63,6 @@ | |||
| import static org.assertj.core.api.Assertions.assertThatThrownBy; | |||
|
|
|||
There was a problem hiding this comment.
JUnit 4's Parameterized runner requires a @Parameters factory method and @Parameter fields. This class has neither, so it was rdundant.
There was a problem hiding this comment.
have you checked amount of tests running before and after in maven output?
spuru9
force-pushed
the
feature/junit5-state-processing-api
branch
3 times, most recently
from
ed787bc to
6059cdf
Compare
|
@snuyanzin Can you review this? |
|
|
||
| Assert.assertEquals("Unexpected number of keys", 2, keys.size()); | ||
| Assert.assertEquals("Unexpected keys found", Arrays.asList(1, 2), keys); | ||
| assertThat(keys).isEqualTo(Arrays.asList(1, 2)); |
There was a problem hiding this comment.
why is messaged removed?
There was a problem hiding this comment.
I removed the JUnit-style assertion messages since AssertJ's failure output (expected … but was …) plus the test/method name already convey them.
Happy to preserve if you need the, I flt thy were redundant is most case. Or if you want it handled on case to case basis.
There was a problem hiding this comment.
we shouldn't drop such things silently without justification
I see lots of messages removed which are even don't match test name, any justification for this?
github-actions
Bot
added
the
community-reviewed
spuru9
force-pushed
the
feature/junit5-state-processing-api
branch
from
6059cdf to
6c997e0
Compare
| assertThat(formatRange.createInputSplits(10)) | ||
| .as("Range filters cannot prune key-group splits") | ||
| .hasSize(formatNoFilter.createInputSplits(10).length); | ||
| assertThat(splits.length).isEqualTo(128); |
|
|
||
| Assert.assertEquals( | ||
| "Incorrect data read from input split", Arrays.asList(1, 1, 2, 2, 3, 3), data); | ||
| assertThat(data).isEqualTo(List.of(1, 1, 2, 2, 3, 3)); |
There was a problem hiding this comment.
why not containsExactly?
3 participants
What is the purpose of the change
Migrate the remaining JUnit 4 tests in
flink-libraries/flink-state-processing-apito JUnit 5 (Jupiter). After this PR the module has zero JUnit 4 imports. Part of the umbrella JUnit 4 → 5 migration.JIRA: FLINK-39770 (sub-task of FLINK-25325)
Brief change log
20 test classes migrated across the module (
org.apache.flink.state.apiand itsinput/output/runtime/utilssub-packages):org.junit.*imports →org.junit.jupiter.api.*; lifecycle annotations@Before*/@After*→@BeforeEach/@AfterEach/@BeforeAll/@AfterAll.extends AbstractTestBaseJUnit4→extends AbstractTestBase(the JUnit 5 sibling) for the six ITCase / base classes.SavepointDeepCopyTest,SavepointWriterWindowITCase):@RunWith(Parameterized.class)→@ExtendWith(ParameterizedTestExtension.class)with@Parameterfield injection and@TestTemplate.@Rule TemporaryFolder→@TempDirfor single-dir cases;TempDirUtilsretained where a test needs several distinct dirs.@Test(expected = X.class)→assertThatThrownBy(...).isInstanceOf(X.class)(with message assertions where the original test checked them).org.junit.Assert.*→ AssertJ; droppublicmodifiers on test classes/methods per the Flink JUnit 5 Migration Guide.No production code is touched.
Verifying this change
This change is a test-only migration covered by the existing tests it migrates:
Result: 115 tests run (53 unit + 62 integration), 0 failures, 0 errors, 0 skipped, matching the pre-migration baseline.
Does this pull request potentially affect one of the following parts:
@Public(Evolving): (no)Documentation
Was generative AI tooling used to co-author this PR?