Re: [PR] MINOR: Add junit properties to display parameterized test names [kafka]
chia7712 commented on PR #14983: URL: https://github.com/apache/kafka/pull/14983#issuecomment-2071780823 I have backport this PR to 3.7 see https://github.com/apache/kafka/commit/6e998cffdd33e343945877ccee1fec8337c7d57d -- 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
Re: [PR] MINOR: Add junit properties to display parameterized test names [kafka]
chia7712 commented on PR #14983: URL: https://github.com/apache/kafka/pull/14983#issuecomment-2069776278 When testing 3.7 branch for #15776, I encountered following error: ``` java.util.concurrent.ExecutionException: org.apache.kafka.common.errors.InvalidTopicException: Topic name is invalid: the length of 'appId_1713797371220_HighAvailabilityTaskAssignorIntegrationTestshouldScaleOutWithWarmupTasksAndPersistentStores_3__rackAwareStrategy_balance-storeHighAvailabilityTaskAssignorIntegrationTestshouldScaleOutWithWarmupTasksAndPersistentStores_3__rackAwareStrategy_balance-changelog' is longer than the max allowed length 249 at java.base/java.util.concurrent.CompletableFuture.reportGet(CompletableFuture.java:396) at java.base/java.util.concurrent.CompletableFuture.get(CompletableFuture.java:2073) at org.apache.kafka.common.internals.KafkaFutureImpl.get(KafkaFutureImpl.java:165) at org.apache.kafka.streams.integration.utils.KafkaEmbedded.createTopic(KafkaEmbedded.java:178) ... 5 more Caused by: org.apache.kafka.common.errors.InvalidTopicException: Topic name is invalid: the length of 'appId_1713797371220_HighAvailabilityTaskAssignorIntegrationTestshouldScaleOutWithWarmupTasksAndPersistentStores_3__rackAwareStrategy_balance-storeHighAvailabilityTaskAssignorIntegrationTestshouldScaleOutWithWarmupTasksAndPersistentStores_3__rackAwareStrategy_balance-changelog' is longer than the max allowed length 249 ``` Hence, I'd like to backport this PR to 3.7. WDYT? -- 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
Re: [PR] MINOR: Add junit properties to display parameterized test names [kafka]
cadonna merged PR #14983: URL: https://github.com/apache/kafka/pull/14983 -- 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
Re: [PR] MINOR: Add junit properties to display parameterized test names [kafka]
cadonna commented on PR #14983: URL: https://github.com/apache/kafka/pull/14983#issuecomment-1853482186 The failed Streams tests are either known to be flaky (I checked the failure reason) or I ran into a timeout. For the ones that run into a timeout I checked if they run locally w/o 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
Re: [PR] MINOR: Add junit properties to display parameterized test names [kafka]
cadonna commented on code in PR #14983: URL: https://github.com/apache/kafka/pull/14983#discussion_r1424055596 ## clients/src/test/resources/junit-platform.properties: ## Review Comment: The goal is to migrate them to JUnit 5 but sadly there is no execution plan. -- 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
Re: [PR] MINOR: Add junit properties to display parameterized test names [kafka]
dajac commented on code in PR #14983: URL: https://github.com/apache/kafka/pull/14983#discussion_r1424053661 ## clients/src/test/resources/junit-platform.properties: ## Review Comment: Would you know if there is an equivalent config for JUnit4? Or, do we have a plan to migrate those to JUnit5? -- 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
Re: [PR] MINOR: Add junit properties to display parameterized test names [kafka]
cadonna commented on code in PR #14983: URL: https://github.com/apache/kafka/pull/14983#discussion_r1424005229 ## clients/src/test/resources/junit-platform.properties: ## Review Comment: Yes, their display name does not change compared to before this PR. -- 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
Re: [PR] MINOR: Add junit properties to display parameterized test names [kafka]
cadonna commented on code in PR #14983: URL: https://github.com/apache/kafka/pull/14983#discussion_r1424005229 ## clients/src/test/resources/junit-platform.properties: ## Review Comment: Yes, their display name does not 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] MINOR: Add junit properties to display parameterized test names [kafka]
dajac commented on code in PR #14983: URL: https://github.com/apache/kafka/pull/14983#discussion_r1423998585 ## clients/src/test/resources/junit-platform.properties: ## Review Comment: I see. Do we also have parameterized tests using JUnit4? -- 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
Re: [PR] MINOR: Add junit properties to display parameterized test names [kafka]
cadonna commented on code in PR #14983: URL: https://github.com/apache/kafka/pull/14983#discussion_r1423995936 ## clients/src/test/resources/junit-platform.properties: ## Review Comment: There is not a change with JUnit4 tests. -- 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
Re: [PR] MINOR: Add junit properties to display parameterized test names [kafka]
cadonna commented on code in PR #14983: URL: https://github.com/apache/kafka/pull/14983#discussion_r1423993764 ## clients/src/test/resources/junit-platform.properties: ## Review Comment: Apparently, it gets both. I see: ``` Dec 12, 2023 2:22:50 PM org.junit.platform.launcher.core.LauncherConfigurationParameters loadClasspathResource WARNING: Discovered 2 'junit-platform.properties' configuration files in the classpath; only the first will be used. Dec 12, 2023 2:22:50 PM org.junit.platform.launcher.core.LauncherConfigurationParameters loadClasspathResource WARNING: Discovered 2 'junit-platform.properties' configuration files in the classpath; only the first will be used. ``` -- 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
Re: [PR] MINOR: Add junit properties to display parameterized test names [kafka]
dajac commented on code in PR #14983: URL: https://github.com/apache/kafka/pull/14983#discussion_r1423948035 ## clients/src/test/resources/junit-platform.properties: ## Review Comment: I am not sure. I thought that Streams would get it from the clients package. We could perhaps verify that the test s use the expected display name. -- 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
Re: [PR] MINOR: Add junit properties to display parameterized test names [kafka]
cadonna commented on code in PR #14983: URL: https://github.com/apache/kafka/pull/14983#discussion_r1423945890 ## clients/src/test/resources/junit-platform.properties: ## Review Comment: @dajac Do you know whether we need this file also in the streams package? In the streams package, we have a mix of JUnit 4 and JUnit 5 tests. -- 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
Re: [PR] MINOR: Add junit properties to display parameterized test names [kafka]
lucasbru commented on code in PR #14983: URL: https://github.com/apache/kafka/pull/14983#discussion_r1423829493 ## clients/src/test/resources/junit-platform.properties: ## Review Comment: Can you check if we need this file as well in the streams package? -- 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
Re: [PR] MINOR: Add junit properties to display parameterized test names [kafka]
cadonna commented on PR #14983: URL: https://github.com/apache/kafka/pull/14983#issuecomment-1850549930 There was not a single streams failure in the builds: https://ci-builds.apache.org/job/Kafka/job/kafka-pr/job/PR-14983/1/#showFailuresLink -- 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
Re: [PR] MINOR: Add junit properties to display parameterized test names [kafka]
dajac merged PR #14687: URL: https://github.com/apache/kafka/pull/14687 -- 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
Re: [PR] MINOR: Add junit properties to display parameterized test names [kafka]
dajac commented on code in PR #14687:
URL: https://github.com/apache/kafka/pull/14687#discussion_r1415612525
##
streams/src/test/java/org/apache/kafka/streams/integration/utils/IntegrationTestUtils.java:
##
@@ -239,10 +240,14 @@ public static String safeUniqueTestName(final Class
testClass, final TestName
* Used by tests migrated to JUnit 5.
*/
public static String safeUniqueTestName(final Class testClass, final
TestInfo testInfo) {
-final String displayName = testInfo.getDisplayName();
final String methodName =
testInfo.getTestMethod().map(Method::getName).orElse("unknownMethodName");
-final String testName = displayName.contains(methodName) ? methodName
: methodName + displayName;
-return safeUniqueTestName(testClass, testName);
+// Generate a random uuid without an `-`. The `-` is used in Streams'
thread name as
+// a separator and some tests rely on this.
+String randomUuid;
+do {
+randomUuid = Uuid.randomUuid().toString();
+} while (randomUuid.contains("-"));
Review Comment:
Addressed with the last commit.
--
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
Re: [PR] MINOR: Add junit properties to display parameterized test names [kafka]
cadonna commented on code in PR #14687:
URL: https://github.com/apache/kafka/pull/14687#discussion_r1415253303
##
streams/src/test/java/org/apache/kafka/streams/integration/utils/IntegrationTestUtils.java:
##
@@ -239,10 +240,14 @@ public static String safeUniqueTestName(final Class
testClass, final TestName
* Used by tests migrated to JUnit 5.
*/
public static String safeUniqueTestName(final Class testClass, final
TestInfo testInfo) {
-final String displayName = testInfo.getDisplayName();
final String methodName =
testInfo.getTestMethod().map(Method::getName).orElse("unknownMethodName");
-final String testName = displayName.contains(methodName) ? methodName
: methodName + displayName;
-return safeUniqueTestName(testClass, testName);
+// Generate a random uuid without an `-`. The `-` is used in Streams'
thread name as
+// a separator and some tests rely on this.
+String randomUuid;
+do {
+randomUuid = Uuid.randomUuid().toString();
+} while (randomUuid.contains("-"));
Review Comment:
I think we can use whatever we want, also `_`. There is an override of
`safeUniqueTestName()` that does a similar replacement. I think you could add
there `-`. It just seemed costly and somehow overly complicated to me to
re-generate a random sequence just to get rid of a character. But probably it
is not costly because in most cases we only need to re-generate once.
--
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
Re: [PR] MINOR: Add junit properties to display parameterized test names [kafka]
dajac commented on code in PR #14687:
URL: https://github.com/apache/kafka/pull/14687#discussion_r1415122463
##
streams/src/test/java/org/apache/kafka/streams/integration/utils/IntegrationTestUtils.java:
##
@@ -239,10 +240,14 @@ public static String safeUniqueTestName(final Class
testClass, final TestName
* Used by tests migrated to JUnit 5.
*/
public static String safeUniqueTestName(final Class testClass, final
TestInfo testInfo) {
-final String displayName = testInfo.getDisplayName();
final String methodName =
testInfo.getTestMethod().map(Method::getName).orElse("unknownMethodName");
-final String testName = displayName.contains(methodName) ? methodName
: methodName + displayName;
-return safeUniqueTestName(testClass, testName);
+// Generate a random uuid without an `-`. The `-` is used in Streams'
thread name as
+// a separator and some tests rely on this.
+String randomUuid;
+do {
+randomUuid = Uuid.randomUuid().toString();
+} while (randomUuid.contains("-"));
Review Comment:
Yeah, I considered this as well. The question is which one should we use? We
could use `_` but then the uuid looks a bit weird when you get one with `_` in
the middle. I thought that it is ok to re-generate a new one in this case. I
can change it if you prefer.
--
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
Re: [PR] MINOR: Add junit properties to display parameterized test names [kafka]
cadonna commented on code in PR #14687:
URL: https://github.com/apache/kafka/pull/14687#discussion_r1412039372
##
streams/src/test/java/org/apache/kafka/streams/integration/utils/IntegrationTestUtils.java:
##
@@ -239,10 +240,14 @@ public static String safeUniqueTestName(final Class
testClass, final TestName
* Used by tests migrated to JUnit 5.
*/
public static String safeUniqueTestName(final Class testClass, final
TestInfo testInfo) {
-final String displayName = testInfo.getDisplayName();
final String methodName =
testInfo.getTestMethod().map(Method::getName).orElse("unknownMethodName");
-final String testName = displayName.contains(methodName) ? methodName
: methodName + displayName;
-return safeUniqueTestName(testClass, testName);
+// Generate a random uuid without an `-`. The `-` is used in Streams'
thread name as
+// a separator and some tests rely on this.
+String randomUuid;
+do {
+randomUuid = Uuid.randomUuid().toString();
+} while (randomUuid.contains("-"));
Review Comment:
Why not just replacing `-` with a different character?
--
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
Re: [PR] MINOR: Add junit properties to display parameterized test names [kafka]
dajac commented on PR #14687: URL: https://github.com/apache/kafka/pull/14687#issuecomment-1834494230 I cannot get a clean build for this one... -- 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
Re: [PR] MINOR: Add junit properties to display parameterized test names [kafka]
dajac commented on code in PR #14687: URL: https://github.com/apache/kafka/pull/14687#discussion_r1410811193 ## streams/src/test/java/org/apache/kafka/streams/integration/HighAvailabilityTaskAssignorIntegrationTest.java: ## @@ -137,8 +138,8 @@ private void shouldScaleOutWithWarmupTasks(final Function
Re: [PR] MINOR: Add junit properties to display parameterized test names [kafka]
cadonna commented on code in PR #14687: URL: https://github.com/apache/kafka/pull/14687#discussion_r1410730716 ## streams/src/test/java/org/apache/kafka/streams/integration/HighAvailabilityTaskAssignorIntegrationTest.java: ## @@ -137,8 +138,8 @@ private void shouldScaleOutWithWarmupTasks(final Function
Re: [PR] MINOR: Add junit properties to display parameterized test names [kafka]
dajac commented on code in PR #14687: URL: https://github.com/apache/kafka/pull/14687#discussion_r1408976485 ## streams/src/test/java/org/apache/kafka/streams/integration/HighAvailabilityTaskAssignorIntegrationTest.java: ## @@ -138,7 +138,7 @@ private void shouldScaleOutWithWarmupTasks(final Function
Re: [PR] MINOR: Add junit properties to display parameterized test names [kafka]
dajac commented on code in PR #14687:
URL: https://github.com/apache/kafka/pull/14687#discussion_r1408975349
##
streams/src/test/java/org/apache/kafka/streams/integration/utils/IntegrationTestUtils.java:
##
@@ -239,10 +240,8 @@ public static String safeUniqueTestName(final Class
testClass, final TestName
* Used by tests migrated to JUnit 5.
*/
public static String safeUniqueTestName(final Class testClass, final
TestInfo testInfo) {
-final String displayName = testInfo.getDisplayName();
final String methodName =
testInfo.getTestMethod().map(Method::getName).orElse("unknownMethodName");
-final String testName = displayName.contains(methodName) ? methodName
: methodName + displayName;
-return safeUniqueTestName(testClass, testName);
+return safeUniqueTestName(testClass, methodName + "_" +
Uuid.randomUuid().toString());
Review Comment:
With the current method, we end up with a non-unique name for the
parameterized tests. Therefore, I suggest to just use the name of the method
with a uuid.
--
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
Re: [PR] MINOR: Add junit properties to display parameterized test names [kafka]
dajac commented on PR #14687: URL: https://github.com/apache/kafka/pull/14687#issuecomment-1830424668 We need to investigate the failed tests before we can merge this one. -- 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
