MINOR: Split up ShareConsumerTest#22017
MINOR: Split up ShareConsumerTest#22017AndrewJSchofield wants to merge 4 commits intoapache:trunkfrom
Conversation
...gration-tests/src/test/java/org/apache/kafka/clients/consumer/ShareConsumerCallbackTest.java
Outdated
Show resolved
Hide resolved
| import static org.junit.jupiter.api.Assertions.assertTrue; | ||
|
|
||
| @Timeout(1200) | ||
| @Tag("integration") |
There was a problem hiding this comment.
Do we need to add @Tag("integration") here? as I can see in @ClusterTest already covers this and is applied consistently across other test suites.
There was a problem hiding this comment.
This one was an outlier and I'm not really sure why. I've added it.
chia7712
left a comment
chia7712
left a comment
There was a problem hiding this comment.
@AndrewJSchofield thanks for this excellent refactor
| @@ -131,7 +97,6 @@ | |||
| import static org.junit.jupiter.api.Assertions.assertTrue; | |||
| import static org.junit.jupiter.api.Assertions.fail; | |||
|
|
|||
| @SuppressWarnings({"ClassFanOutComplexity", "ClassDataAbstractionCoupling"}) | |||
| @Timeout(1200) | |||
| @Tag("integration") | |||
There was a problem hiding this comment.
ClusterTest covers this tag out of box, so we are good to skip it here
| import static org.junit.jupiter.api.Assertions.assertTrue; | ||
|
|
||
| @Timeout(1200) | ||
| @Tag("integration") |
| import static org.junit.jupiter.api.Assertions.assertTrue; | ||
|
|
||
| @Timeout(1200) | ||
| @Tag("integration") |
| import static org.junit.jupiter.api.Assertions.fail; | ||
|
|
||
| @Timeout(1200) | ||
| @Tag("integration") |
| } | ||
| ) | ||
| public static class WithAssignmentBatchingDisabledTest extends ShareConsumerRackAwareTest { | ||
| @Tag("integration") |
| import static org.junit.jupiter.api.Assertions.assertTrue; | ||
|
|
||
| @Timeout(1200) | ||
| @Tag("integration") |
| import static org.junit.jupiter.api.Assertions.assertFalse; | ||
| import static org.junit.jupiter.api.Assertions.fail; | ||
|
|
||
| public class ShareConsumerTestBase { |
| assertEquals(1, records.count()); | ||
|
|
||
| // Waiting until the acquisition lock expires. | ||
| Thread.sleep(20000); |
There was a problem hiding this comment.
Can we use a smaller group.share.record.lock.duration.ms here to make the test run faster?
There was a problem hiding this comment.
It's taken effort to get these tests to run reliably so I'm nervous about messing with the timeouts. It's a bit unpredictable ensuring that the timeout only happens when we want it to. Luckily there are few tests that wait like this.
see-quick
left a comment
see-quick
left a comment
There was a problem hiding this comment.
👍, btw , I think there may be additional places in the integration tests where we are doing this redundantly (i.e., tagging integration) . It might be worth opening an issue so someone can clean it up and we can avoid these niche definitions. WDYT?
That's a great idea! If you have some free cycles, please feel free to open a minor PR to address it. :) |
4 participants
Split up the excessively large
ShareConsumerTestinto a handful ofsmaller source files.
Reviewers: Chia-Ping Tsai chia7712@gmail.com