Re: [PR] KAFKA-16679 merge unit test down to the class of integration test [kafka]

[via GitHub]

chia7712 merged PR #15884:
URL: https://github.com/apache/kafka/pull/15884


-- 
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] KAFKA-16679 merge unit test down to the class of integration test [kafka]

[via GitHub]

KevinZTW commented on PR #15884:
URL: https://github.com/apache/kafka/pull/15884#issuecomment-2104711204

   > @KevinZTW Please fix the build error
   
   sorry I just fixed it


-- 
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] KAFKA-16679 merge unit test down to the class of integration test [kafka]

[via GitHub]

chia7712 commented on PR #15884:
URL: https://github.com/apache/kafka/pull/15884#issuecomment-2104544789

   @KevinZTW Please fix the build error


-- 
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] KAFKA-16679 merge unit test down to the class of integration test [kafka]

[via GitHub]

KevinZTW commented on code in PR #15884:
URL: https://github.com/apache/kafka/pull/15884#discussion_r1596572793


##
tools/src/test/java/org/apache/kafka/tools/FeatureCommandTest.java:
##
@@ -47,22 +47,16 @@
 @ClusterTestDefaults(clusterType = Type.KRAFT)

Review Comment:
   oh you are right, I didn't notice that part. Just remove it!



-- 
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] KAFKA-16679 merge unit test down to the class of integration test [kafka]

[via GitHub]

chia7712 commented on code in PR #15884:
URL: https://github.com/apache/kafka/pull/15884#discussion_r1594885585


##
tools/src/test/java/org/apache/kafka/tools/FeatureCommandTest.java:
##
@@ -47,22 +47,16 @@
 @ClusterTestDefaults(clusterType = Type.KRAFT)

Review Comment:
   It seems we don't need `ClusterTestDefaults` for this test class, since all 
test cases have defined 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-unsubscr...@kafka.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Re: [PR] KAFKA-16679 merge unit test down to the class of integration test [kafka]

[via GitHub]

KevinZTW commented on code in PR #15884:
URL: https://github.com/apache/kafka/pull/15884#discussion_r1594136562


##
tools/src/test/java/org/apache/kafka/tools/FeatureCommandTest.java:
##
@@ -160,28 +154,26 @@ private String outputWithoutEpoch(String output) {
 int pos = output.indexOf("Epoch: ");
 return (pos > 0) ? output.substring(0, pos) : output;
 }
-}
 
-class FeatureCommandUnitTest {

Review Comment:
   thanks for the advice! just add it back and do feel it clearer as you said 😄 



-- 
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] KAFKA-16679 merge unit test down to the class of integration test [kafka]

[via GitHub]

showuon commented on code in PR #15884:
URL: https://github.com/apache/kafka/pull/15884#discussion_r1593636792


##
tools/src/test/java/org/apache/kafka/tools/FeatureCommandTest.java:
##
@@ -160,28 +154,26 @@ private String outputWithoutEpoch(String output) {
 int pos = output.indexOf("Epoch: ");
 return (pos > 0) ? output.substring(0, pos) : output;
 }
-}
 
-class FeatureCommandUnitTest {

Review Comment:
   ditto, adding comments to make it clear the following tests are unit tests.



##
tools/src/test/java/org/apache/kafka/tools/DeleteRecordsCommandTest.java:
##
@@ -112,12 +107,7 @@ private static void executeAndAssertOutput(String json, 
String expOut, Admin adm
 });
 assertTrue(output.contains(expOut));
 }
-}
 
-/**
- * Unit test of {@link DeleteRecordsCommand} tool.
- */

Review Comment:
   I would keep the comment to make it clear the following tests are unit 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] KAFKA-16679 merge unit test down to the class of integration test [kafka]

[via GitHub]

KevinZTW commented on code in PR #15884:
URL: https://github.com/apache/kafka/pull/15884#discussion_r1593470507


##
tools/src/test/java/org/apache/kafka/tools/FeatureCommandTest.java:
##
@@ -160,28 +154,26 @@ private String outputWithoutEpoch(String output) {
 int pos = output.indexOf("Epoch: ");
 return (pos > 0) ? output.substring(0, pos) : output;
 }
-}
 
-class FeatureCommandUnitTest {
 @Test
 public void testLevelToString() {
 assertEquals("5", FeatureCommand.levelToString("foo.bar", (short) 5));
 assertEquals("3.3-IV0",
-FeatureCommand.levelToString(MetadataVersion.FEATURE_NAME, 
MetadataVersion.IBP_3_3_IV0.featureLevel()));
+FeatureCommand.levelToString(MetadataVersion.FEATURE_NAME, 
MetadataVersion.IBP_3_3_IV0.featureLevel()));
 }
 
 @Test
 public void testMetadataVersionsToString() {
 assertEquals("3.3-IV0, 3.3-IV1, 3.3-IV2, 3.3-IV3",
-
FeatureCommand.metadataVersionsToString(MetadataVersion.IBP_3_3_IV0, 
MetadataVersion.IBP_3_3_IV3));
+
FeatureCommand.metadataVersionsToString(MetadataVersion.IBP_3_3_IV0, 
MetadataVersion.IBP_3_3_IV3));
 }
 
 @Test
 public void testdowngradeType() {

Review Comment:
   thanks! just fix it in new 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