C0urante commented on code in PR #16253: URL: https://github.com/apache/kafka/pull/16253#discussion_r1633406693
########## connect/runtime/src/test/java/org/apache/kafka/connect/runtime/distributed/IncrementalCooperativeAssignorTest.java: ########## @@ -1508,25 +1545,25 @@ private List<Integer> allocations(Function<ConnectorsAndTasks, ? extends Collect private void assertNoRevocations() { returnedAssignments.newlyRevokedConnectors().forEach((worker, revocations) -> assertEquals( - "Expected no revocations to take place during this round, but connector revocations were issued for worker " + worker, Collections.emptySet(), - new HashSet<>(revocations) + new HashSet<>(revocations), + "Expected no revocations to take place during this round, but connector revocations were issued for worker " + worker ) ); returnedAssignments.newlyRevokedTasks().forEach((worker, revocations) -> assertEquals( - "Expected no revocations to take place during this round, but task revocations were issued for worker " + worker, Collections.emptySet(), - new HashSet<>(revocations) Review Comment: These tests aren't flaky, but it is a bit tricky to make changes to them. It definitely saved me time to have these error messages the last time I was in this neck of the woods. With rudimentary checking logic (searching for `assertEquals(Collections.empty` in the code base), I also see this pattern used 94 times across 16 files in the Connect modules, so it's not exactly uncommon. IMO it'd be better to make it more common in other parts of the code base rather than try to "simplify" things in a way that drops information on assertion failures. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org