You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by GitBox <gi...@apache.org> on 2021/03/04 10:51:05 UTC
[GitHub] [kafka] dajac commented on a change in pull request #10078: MINOR: make sure all generated data tests cover all versions
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:
users@infra.apache.org