KAFKA-20689: Migrate reset integration tests to ClusterInstance#22579
KAFKA-20689: Migrate reset integration tests to ClusterInstance#22579Russole wants to merge 8 commits into
Conversation
| readRecords(topic, consumer, waitTime, expectedNumRecords); | ||
| accumData.addAll(readData); | ||
| assertThat(reason, accumData.size(), is(greaterThanOrEqualTo(expectedNumRecords))); | ||
| assertTrue(accumData.size() >= expectedNumRecords, reason); |
There was a problem hiding this comment.
Would you mind removing those unrelated changes?
There was a problem hiding this comment.
Thanks @chia7712 for the review. I can remove the IntegrationTestUtils changes.
One thing I noticed locally is that if I revert those changes but still remove Hamcrest from tools, :tools:test fails with NoClassDefFoundError: org/hamcrest/Matchers.
So even though tools does not use Hamcrest directly, some tools tests call IntegrationTestUtils from the Streams integration tests, and that class still uses Hamcrest. That means tools still needs Hamcrest indirectly unless we also remove the Hamcrest usage there.
For example, these tests fail after reverting the IntegrationTestUtils change:
ResetIntegrationWithSslTest.testReprocessingFromScratchAfterResetWithIntermediateInternalTopicResetIntegrationWithSslTest.testReprocessingFromScratchAfterResetWithIntermediateUserTopicResetIntegrationWithSslTest.testReprocessingFromScratchAfterResetWithoutIntermediateUserTopicResetIntegrationWithSslTest.testResetWhenInternalTopicsAreSpecified
Do you prefer that I keep the IntegrationTestUtils change in this PR, or should I leave the tools Hamcrest dependency as-is?
There was a problem hiding this comment.
Could we rewrite both ResetIntegrationTest and ResetIntegrationWithSslTest using ClusterInstance?
There was a problem hiding this comment.
Yes, that makes sense. I’ll look into migrating ResetIntegrationTest and ResetIntegrationWithSslTest to ClusterInstance so we can keep the dependency cleanup scoped to tools instead of changing IntegrationTestUtils.
Migrate the reset integration tests in the
toolsmodule to useClusterInstance.The
toolsmodule previously depended on Streams integration testutilities through test outputs. Those utilities still use Hamcrest, so
removing the explicit Hamcrest runtime dependency from
toolsexposedan indirect runtime dependency.
Instead of changing
IntegrationTestUtils, this patch updatesResetIntegrationTestandResetIntegrationWithSslTestto use theKafka common test cluster framework directly. The reset tests now keep
local copies of the small subset of test utility behavior they need,
staying as close as possible to the original
IntegrationTestUtilsandEmbeddedKafkaClustersemantics while removing the dependency fromtools.This allows the unnecessary Hamcrest test runtime dependency to be
removed from the
toolsmodule.Reviewers: Chia-Ping Tsai chia7712@gmail.com
Testing