You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "peterxcli (via GitHub)" <gi...@apache.org> on 2024/12/22 08:44:15 UTC
[PR] KAFKA-18173: Remove duplicate `assertFutureError` [kafka]
peterxcli opened a new pull request, #18296:
URL: https://github.com/apache/kafka/pull/18296
`assertFutureError` is equal to `assertFutureThrows` in TestUtils.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18173: Remove duplicate `assertFutureError` [kafka]
Posted by "peterxcli (via GitHub)" <gi...@apache.org>.
peterxcli commented on code in PR #18296:
URL: https://github.com/apache/kafka/pull/18296#discussion_r1898777862
##########
clients/src/test/java/org/apache/kafka/test/TestUtils.java:
##########
@@ -583,19 +583,6 @@ public static <T extends Throwable> void assertFutureThrows(
assertEquals(expectedMessage, receivedException.getMessage());
}
- public static void assertFutureError(Future<?> future, Class<? extends Throwable> exceptionClass)
- throws InterruptedException {
- try {
- future.get();
- fail("Expected a " + exceptionClass.getSimpleName() + " exception, but got success.");
- } catch (ExecutionException ee) {
- Throwable cause = ee.getCause();
- assertEquals(exceptionClass, cause.getClass(),
Review Comment:
The remaining method, `assertThrows`, appears as follows:
```java
private static <T extends Throwable> T assertThrows(Class<T> expectedType, Executable executable, Object messageOrSupplier) {
try {
executable.execute();
}
catch (Throwable actualException) {
if (expectedType.isInstance(actualException)) {
return (T) actualException;
}
else {
UnrecoverableExceptions.rethrowIfUnrecoverable(actualException);
throw assertionFailure() //
.message(messageOrSupplier) //
.expected(expectedType) //
.actual(actualException.getClass()) //
.reason("Unexpected exception type thrown") //
.cause(actualException) //
.build();
}
}
throw assertionFailure() //
.message(messageOrSupplier) //
.reason(format("Expected %s to be thrown, but nothing was thrown.", getCanonicalName(expectedType))) //
.build();
}
```
This implementation seems to have the same effect as the `assertFutureError` method in [TestUtils.java](https://github.com/peterxcli/kafka/blob/b4be1785998d382843002ffd8685d67fe8d9c97d/clients/src/test/java/org/apache/kafka/test/TestUtils.java#L586-L597)
Would like to know your thoughts on this. Thanks!
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18173: Remove duplicate `assertFutureError` [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on code in PR #18296:
URL: https://github.com/apache/kafka/pull/18296#discussion_r1899616344
##########
clients/src/test/java/org/apache/kafka/test/TestUtils.java:
##########
@@ -583,19 +583,6 @@ public static <T extends Throwable> void assertFutureThrows(
assertEquals(expectedMessage, receivedException.getMessage());
}
- public static void assertFutureError(Future<?> future, Class<? extends Throwable> exceptionClass)
- throws InterruptedException {
- try {
- future.get();
- fail("Expected a " + exceptionClass.getSimpleName() + " exception, but got success.");
- } catch (ExecutionException ee) {
- Throwable cause = ee.getCause();
- assertEquals(exceptionClass, cause.getClass(),
Review Comment:
the replacement `assertFutureThrows` is using `assertThrows` and `assertInstanceOf`:
```java
public static <T extends Throwable> T assertFutureThrows(Future<?> future, Class<T> exceptionCauseClass) {
ExecutionException exception = assertThrows(ExecutionException.class, future::get);
assertInstanceOf(exceptionCauseClass, exception.getCause(),
"Unexpected exception cause " + exception.getCause());
return exceptionCauseClass.cast(exception.getCause());
}
```
I think the `expected class` is already included by the error message.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18173: Remove duplicate `assertFutureError` [kafka]
Posted by "ijuma (via GitHub)" <gi...@apache.org>.
ijuma commented on code in PR #18296:
URL: https://github.com/apache/kafka/pull/18296#discussion_r1896832320
##########
clients/src/test/java/org/apache/kafka/test/TestUtils.java:
##########
@@ -583,19 +583,6 @@ public static <T extends Throwable> void assertFutureThrows(
assertEquals(expectedMessage, receivedException.getMessage());
}
- public static void assertFutureError(Future<?> future, Class<? extends Throwable> exceptionClass)
- throws InterruptedException {
- try {
- future.get();
- fail("Expected a " + exceptionClass.getSimpleName() + " exception, but got success.");
- } catch (ExecutionException ee) {
- Throwable cause = ee.getCause();
- assertEquals(exceptionClass, cause.getClass(),
Review Comment:
The remaining method has a less useful message, it doesn't include the expected class. Perhaps we should address it as part of this change.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18173: Remove duplicate `assertFutureError` [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on PR #18296:
URL: https://github.com/apache/kafka/pull/18296#issuecomment-2577541935
the failed test is traced by https://issues.apache.org/jira/browse/KAFKA-18298
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18173: Remove duplicate `assertFutureError` [kafka]
Posted by "ijuma (via GitHub)" <gi...@apache.org>.
ijuma commented on code in PR #18296:
URL: https://github.com/apache/kafka/pull/18296#discussion_r1899724466
##########
clients/src/test/java/org/apache/kafka/test/TestUtils.java:
##########
@@ -583,19 +583,6 @@ public static <T extends Throwable> void assertFutureThrows(
assertEquals(expectedMessage, receivedException.getMessage());
}
- public static void assertFutureError(Future<?> future, Class<? extends Throwable> exceptionClass)
- throws InterruptedException {
- try {
- future.get();
- fail("Expected a " + exceptionClass.getSimpleName() + " exception, but got success.");
- } catch (ExecutionException ee) {
- Throwable cause = ee.getCause();
- assertEquals(exceptionClass, cause.getClass(),
Review Comment:
That seems reasonable to me.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18173: Remove duplicate `assertFutureError` [kafka]
Posted by "TaiJuWu (via GitHub)" <gi...@apache.org>.
TaiJuWu commented on code in PR #18296:
URL: https://github.com/apache/kafka/pull/18296#discussion_r1900565069
##########
clients/src/test/java/org/apache/kafka/clients/admin/DeleteConsumerGroupOffsetsResultTest.java:
##########
@@ -63,7 +63,7 @@ public void testTopLevelErrorConstructor() throws InterruptedException {
partitionFutures.completeExceptionally(Errors.GROUP_AUTHORIZATION_FAILED.exception());
DeleteConsumerGroupOffsetsResult topLevelErrorResult =
new DeleteConsumerGroupOffsetsResult(partitionFutures, partitions);
- TestUtils.assertFutureError(topLevelErrorResult.all(), GroupAuthorizationException.class);
+ TestUtils.assertFutureThrows(topLevelErrorResult.all(), GroupAuthorizationException.class);
Review Comment:
Could we align junit conversation and make it become assertFutureThrows(Class<T> expectedCauseClassApiException, Future<?> future)?
Just a suggestion not a requirement.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18173: Remove duplicate `assertFutureError` [kafka]
Posted by "ijuma (via GitHub)" <gi...@apache.org>.
ijuma commented on code in PR #18296:
URL: https://github.com/apache/kafka/pull/18296#discussion_r1899687406
##########
clients/src/test/java/org/apache/kafka/test/TestUtils.java:
##########
@@ -583,19 +583,6 @@ public static <T extends Throwable> void assertFutureThrows(
assertEquals(expectedMessage, receivedException.getMessage());
}
- public static void assertFutureError(Future<?> future, Class<? extends Throwable> exceptionClass)
- throws InterruptedException {
- try {
- future.get();
- fail("Expected a " + exceptionClass.getSimpleName() + " exception, but got success.");
- } catch (ExecutionException ee) {
- Throwable cause = ee.getCause();
- assertEquals(exceptionClass, cause.getClass(),
Review Comment:
I was referring to the `assertInstanceOf` part. I tested and this is what I see:
Before:
```org.opentest4j.AssertionFailedError: Expected a IllegalArgumentException exception, but got TimeoutException ==>
Expected :class java.lang.IllegalArgumentException
Actual :class org.apache.kafka.common.errors.TimeoutException
```
After:
```
org.opentest4j.AssertionFailedError: Unexpected exception cause org.apache.kafka.common.errors.TimeoutException: Timed out waiting for a node assignment. Call: createTopics ==> Unexpected type,
Expected :class java.lang.IllegalArgumentException
Actual :class org.apache.kafka.common.errors.TimeoutException
```
I think the new message is ok, but we should probably say something like "ExecutionException thrown by Future has unexpected cause ".
One additional detail, the new check is less strict - it allows the class or subclass while the old one only allowed the exact class. We should probably make the behavior configurable - if code expects an exact exception to be thrown, we should check for that.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18173: Remove duplicate `assertFutureError` [kafka]
Posted by "peterxcli (via GitHub)" <gi...@apache.org>.
peterxcli commented on code in PR #18296:
URL: https://github.com/apache/kafka/pull/18296#discussion_r1898777862
##########
clients/src/test/java/org/apache/kafka/test/TestUtils.java:
##########
@@ -583,19 +583,6 @@ public static <T extends Throwable> void assertFutureThrows(
assertEquals(expectedMessage, receivedException.getMessage());
}
- public static void assertFutureError(Future<?> future, Class<? extends Throwable> exceptionClass)
- throws InterruptedException {
- try {
- future.get();
- fail("Expected a " + exceptionClass.getSimpleName() + " exception, but got success.");
- } catch (ExecutionException ee) {
- Throwable cause = ee.getCause();
- assertEquals(exceptionClass, cause.getClass(),
Review Comment:
The remaining method uses `assertThrows`, appears as follows:
```java
private static <T extends Throwable> T assertThrows(Class<T> expectedType, Executable executable, Object messageOrSupplier) {
try {
executable.execute();
}
catch (Throwable actualException) {
if (expectedType.isInstance(actualException)) {
return (T) actualException;
}
else {
UnrecoverableExceptions.rethrowIfUnrecoverable(actualException);
throw assertionFailure() //
.message(messageOrSupplier) //
.expected(expectedType) //
.actual(actualException.getClass()) //
.reason("Unexpected exception type thrown") //
.cause(actualException) //
.build();
}
}
throw assertionFailure() //
.message(messageOrSupplier) //
.reason(format("Expected %s to be thrown, but nothing was thrown.", getCanonicalName(expectedType))) //
.build();
}
```
This implementation seems to have the same effect as the `assertFutureError` method in [TestUtils.java](https://github.com/peterxcli/kafka/blob/b4be1785998d382843002ffd8685d67fe8d9c97d/clients/src/test/java/org/apache/kafka/test/TestUtils.java#L586-L597)
Would like to know your thoughts on this. Thanks!
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18173: Remove duplicate `assertFutureError` [kafka]
Posted by "peterxcli (via GitHub)" <gi...@apache.org>.
peterxcli commented on PR #18296:
URL: https://github.com/apache/kafka/pull/18296#issuecomment-2558455387
Non-related failed tests.
PlaintextConsumerCallbackTest | testSeekPositionAndPauseNewlyAssignedPartitionOnPartitionsAssignedCallback(String, String).quorum=kraft.groupProtocol=consumer | org.apache.kafka.common.KafkaException: User rebalance callback throws an error | 1.98s
-- | -- | -- | --
RemoteLogManagerTest | testRLMOpsWhenMetadataIsNotReady() | org.mockito.exceptions.verification.NoInteractionsWanted:No interactions wanted here:-> at kafka.log.remote.RemoteLogManagerTest.testRLMOpsWhenMetadataIsNotReady(RemoteLogManagerTest.java:3706)But found this interaction on mock 'remoteLogMetadataManager':-> at kafka.log.remote.RemoteLogManager$RLMTask.run(RemoteLogManager.java:815)***For your reference, here is the list of all invocations ([?] - means unverified).1. -> at kafka.log.remote.RemoteLogManager.configureRLMM(RemoteLogManager.java:413)2. -> at kafka.log.remote.RemoteLogManager.onLeadershipChange(RemoteLogManager.java:478)3. -> at kafka.log.remote.RemoteLogManager$RLMTask.run(RemoteLogManager.java:815)4. -> at kafka.log.remote.RemoteLogManager$RLMTask.run(RemoteLogManager.java:815)5. [?]-> at kafka.log.remote.RemoteLogManager$RLMTask.run(RemoteLogManager.java:815) | 0.07s
PlaintextAdminIntegrationTest | testConsumerGroupsDeprecatedConsumerGroupState(String, String).quorum=kraft.groupProtocol=consumer | org.opentest4j.AssertionFailedError: Expected the offset for partition 0 to eventually become 1. | 21.79s
DynamicBrokerReconfigurationTest | testMetricsReporterUpdate(String, String).quorum=kraft.groupProtocol=classic | org.opentest4j.AssertionFailedError: expected: <1> but was: <0> | 2.18s
-- | -- | -- | --
PlaintextAdminIntegrationTest | testConsumerGroupsDeprecatedConsumerGroupState(String, String).quorum=kraft.groupProtocol=consumer | org.opentest4j.AssertionFailedError: Expected the offset for partition 0 to eventually become 1. | 21.56s
PlaintextAdminIntegrationTest | testConsumerGroups(String, String).quorum=kraft.groupProtocol=consumer | org.opentest4j.AssertionFailedError: Expected the offset for partition 0 to eventually become 1. | 21.79s
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18173: Remove duplicate `assertFutureError` [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on code in PR #18296:
URL: https://github.com/apache/kafka/pull/18296#discussion_r1899690842
##########
clients/src/test/java/org/apache/kafka/test/TestUtils.java:
##########
@@ -583,19 +583,6 @@ public static <T extends Throwable> void assertFutureThrows(
assertEquals(expectedMessage, receivedException.getMessage());
}
- public static void assertFutureError(Future<?> future, Class<? extends Throwable> exceptionClass)
- throws InterruptedException {
- try {
- future.get();
- fail("Expected a " + exceptionClass.getSimpleName() + " exception, but got success.");
- } catch (ExecutionException ee) {
- Throwable cause = ee.getCause();
- assertEquals(exceptionClass, cause.getClass(),
Review Comment:
> One additional detail, the new check is less strict - it allows the class or subclass while the old one only allowed the exact class. We should probably make the behavior configurable - if code expects an exact exception to be thrown, we should check for that.
good point. Personally, I love the strict check. We could adjust `assertFutureThrows` to enforce this stricter behavior. While this might cause some existing tests to fail, I believe these failures should be relatively easy to address.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18173: Remove duplicate `assertFutureError` [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 merged PR #18296:
URL: https://github.com/apache/kafka/pull/18296
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org