chia7712 commented on a change in pull request #9926:
URL: https://github.com/apache/kafka/pull/9926#discussion_r560314382
##########
File path: generator/src/test/java/org/apache/kafka/message/VersionsTest.java
##########
@@ -92,16 +90,16 @@ public void testContains() {
@Test
public void testSubtract() {
- assertEquals(Versions.NONE,
- Versions.NONE.subtract(Versions.NONE));
+ assertEquals(Versions.NONE.subtract(Versions.NONE),
+ Versions.NONE);
Review comment:
Could you make them in single line?
##########
File path: generator/src/test/java/org/apache/kafka/message/VersionsTest.java
##########
@@ -69,11 +67,11 @@ public void testIntersections() {
assertEquals(newVersions(3, 3),
newVersions(0, Short.MAX_VALUE).intersect(
newVersions(3, 3)));
- assertEquals(Versions.NONE,
- newVersions(9, Short.MAX_VALUE).intersect(
- newVersions(2, 8)));
- assertEquals(Versions.NONE,
- Versions.NONE.intersect(Versions.NONE));
+ assertEquals(newVersions(9, Short.MAX_VALUE).intersect(
+ newVersions(2, 8)),
+ Versions.NONE);
+ assertEquals(Versions.NONE.intersect(Versions.NONE),
Review comment:
ditto
##########
File path:
generator/src/test/java/org/apache/kafka/message/VersionConditionalTest.java
##########
@@ -37,7 +35,7 @@ static void assertEquals(CodeBuffer buffer, String... lines)
throws Exception {
for (String line : lines) {
expectedStringBuilder.append(String.format(line));
}
- Assert.assertEquals(expectedStringBuilder.toString(),
stringWriter.toString());
+ Assertions.assertEquals(stringWriter.toString(),
expectedStringBuilder.toString());
Review comment:
We don't use the prefix ```Assertions``` now.
##########
File path: generator/src/test/java/org/apache/kafka/message/VersionsTest.java
##########
@@ -92,16 +90,16 @@ public void testContains() {
@Test
public void testSubtract() {
- assertEquals(Versions.NONE,
- Versions.NONE.subtract(Versions.NONE));
+ assertEquals(Versions.NONE.subtract(Versions.NONE),
+ Versions.NONE);
assertEquals(newVersions(0, 0),
newVersions(0, 0).subtract(Versions.NONE));
assertEquals(newVersions(1, 1),
newVersions(1, 2).subtract(newVersions(2, 2)));
assertEquals(newVersions(2, 2),
newVersions(1, 2).subtract(newVersions(1, 1)));
- assertEquals(null,
- newVersions(0, Short.MAX_VALUE).subtract(newVersions(1, 100)));
+ assertEquals(newVersions(0, Short.MAX_VALUE).subtract(newVersions(1,
100)),
Review comment:
How about replacing it by ```assertNull```?
##########
File path: generator/src/test/java/org/apache/kafka/message/VersionsTest.java
##########
@@ -57,8 +55,8 @@ public void testRoundTrips() {
}
private void testRoundTrip(Versions versions, String string) {
- assertEquals(string, versions.toString());
- assertEquals(versions, Versions.parse(versions.toString(), null));
+ assertEquals(versions.toString(), string);
Review comment:
the fist argument is still "expected value" so we don't need to switch
the args. There are many similar occurrence.
##########
File path: generator/src/test/java/org/apache/kafka/message/VersionsTest.java
##########
@@ -69,11 +67,11 @@ public void testIntersections() {
assertEquals(newVersions(3, 3),
newVersions(0, Short.MAX_VALUE).intersect(
newVersions(3, 3)));
- assertEquals(Versions.NONE,
- newVersions(9, Short.MAX_VALUE).intersect(
- newVersions(2, 8)));
- assertEquals(Versions.NONE,
- Versions.NONE.intersect(Versions.NONE));
+ assertEquals(newVersions(9, Short.MAX_VALUE).intersect(
Review comment:
This change is unnecessary.
----------------------------------------------------------------
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
