dajac commented on a change in pull request #10078:
URL: https://github.com/apache/kafka/pull/10078#discussion_r587357462
##########
File path:
clients/src/test/java/org/apache/kafka/common/requests/ProduceRequestTest.java
##########
@@ -291,19 +292,11 @@ public void testMixedIdempotentData() {
assertTrue(RequestTestUtils.hasIdempotentRecords(request));
}
- private void
assertThrowsInvalidRecordExceptionForAllVersions(ProduceRequest.Builder
builder) {
- for (short version = builder.oldestAllowedVersion(); version <
builder.latestAllowedVersion(); version++) {
- assertThrowsInvalidRecordException(builder, version);
- }
- }
-
- private void assertThrowsInvalidRecordException(ProduceRequest.Builder
builder, short version) {
- try {
- builder.build(version).serialize();
- fail("Builder did not raise " +
InvalidRecordException.class.getName() + " as expected");
- } catch (RuntimeException e) {
-
assertTrue(InvalidRecordException.class.isAssignableFrom(e.getClass()),
- "Unexpected exception type " + e.getClass().getName());
+ private static <T extends Throwable> void
assertThrowsInvalidRecordExceptionForAllVersions(ProduceRequest.Builder builder,
Review comment:
Should we rename the method now that the exception is passed as an
argument?
##########
File path:
clients/src/test/java/org/apache/kafka/common/requests/RequestResponseTest.java
##########
@@ -295,7 +295,7 @@ public void testSerialization() throws Exception {
checkRequest(createDeleteGroupsRequest(), true);
checkErrorResponse(createDeleteGroupsRequest(),
unknownServerException, true);
checkResponse(createDeleteGroupsResponse(), 0, true);
- for (int i = 0; i < ApiKeys.LIST_OFFSETS.latestVersion(); i++) {
+ for (int i = 0; i <= ApiKeys.LIST_OFFSETS.latestVersion(); i++) {
Review comment:
Could we use `allVersions` here as well?
##########
File path:
clients/src/test/java/org/apache/kafka/common/requests/ProduceRequestTest.java
##########
@@ -291,19 +292,11 @@ public void testMixedIdempotentData() {
assertTrue(RequestTestUtils.hasIdempotentRecords(request));
}
- private void
assertThrowsInvalidRecordExceptionForAllVersions(ProduceRequest.Builder
builder) {
- for (short version = builder.oldestAllowedVersion(); version <
builder.latestAllowedVersion(); version++) {
- assertThrowsInvalidRecordException(builder, version);
- }
- }
-
- private void assertThrowsInvalidRecordException(ProduceRequest.Builder
builder, short version) {
- try {
- builder.build(version).serialize();
- fail("Builder did not raise " +
InvalidRecordException.class.getName() + " as expected");
- } catch (RuntimeException e) {
-
assertTrue(InvalidRecordException.class.isAssignableFrom(e.getClass()),
- "Unexpected exception type " + e.getClass().getName());
+ private static <T extends Throwable> void
assertThrowsInvalidRecordExceptionForAllVersions(ProduceRequest.Builder builder,
+
Class<T> expectedType) {
+ for (short version = builder.oldestAllowedVersion(); version <=
builder.latestAllowedVersion(); version++) {
+ short v = version;
Review comment:
Could we get rid of `v` and use `version` below directly?
##########
File path:
clients/src/test/java/org/apache/kafka/common/message/MessageTest.java
##########
@@ -485,7 +485,7 @@ public void testOffsetCommitRequestVersions() throws
Exception {
if (version == 1) {
testEquivalentMessageRoundTrip(version, requestData);
} else if (version >= 2 && version <= 4) {
- testAllMessageRoundTripsBetweenVersions(version, (short) 4,
requestData, requestData);
+ testAllMessageRoundTripsBetweenVersions(version, (short) 5,
requestData, requestData);
Review comment:
Why are we changing this?
##########
File path:
clients/src/test/java/org/apache/kafka/common/requests/OffsetsForLeaderEpochRequestTest.java
##########
@@ -34,15 +34,15 @@ public void testForConsumerRequiresVersion3() {
assertThrows(UnsupportedVersionException.class, () ->
builder.build(v));
}
- for (short version = 3; version <
ApiKeys.OFFSET_FOR_LEADER_EPOCH.latestVersion(); version++) {
+ for (short version = 3; version <=
ApiKeys.OFFSET_FOR_LEADER_EPOCH.latestVersion(); version++) {
OffsetsForLeaderEpochRequest request = builder.build((short) 3);
Review comment:
Should we replace `3` by `version` here?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
