ctubbsii commented on code in PR #2574:
URL: https://github.com/apache/thrift/pull/2574#discussion_r851783590
##########
lib/java/test/org/apache/thrift/TestFullCamel.java:
##########
@@ -42,10 +41,11 @@ public void testCamelCaseSyntax() throws Exception {
obj.setImFalse(true);
byte[] serBytes = binarySerializer.serialize(obj);
binaryDeserializer.deserialize(obj, serBytes);
- assertTrue( obj.getABite() == (byte) 0xae );
- assertTrue( obj.isImFalse() == true );
+ assertEquals(obj.getABite(), (byte) 0xae);
+ assertEquals(true, obj.isImFalse());
Review Comment:
```suggestion
assertTrue(obj.isImFalse());
```
##########
lib/java/test/org/apache/thrift/TestReuse.java:
##########
@@ -50,8 +54,8 @@ public void testReuseObject() throws Exception {
binaryDeserializer.deserialize(ru1, serBytes);
- assertTrue( ru1.getVal2() == hs1 );
- assertTrue( hs1.size() == 2 );
+ assertSame(ru1.getVal2(), hs1);
Review Comment:
```suggestion
assertSame(hs1, ru1.getVal2());
```
##########
lib/java/test/org/apache/thrift/TestFullCamel.java:
##########
@@ -42,10 +41,11 @@ public void testCamelCaseSyntax() throws Exception {
obj.setImFalse(true);
byte[] serBytes = binarySerializer.serialize(obj);
binaryDeserializer.deserialize(obj, serBytes);
- assertTrue( obj.getABite() == (byte) 0xae );
- assertTrue( obj.isImFalse() == true );
+ assertEquals(obj.getABite(), (byte) 0xae);
Review Comment:
Expected always comes before actual, in order for the output error message
to display correctly:
```suggestion
assertEquals((byte) 0xae, obj.getABite());
```
##########
lib/java/test/org/apache/thrift/TestStruct.java:
##########
@@ -48,7 +38,23 @@
import thrift.test.StructB;
import thrift.test.Xtruct;
-public class TestStruct extends TestCase {
+import java.io.ByteArrayInputStream;
+import java.io.ByteArrayOutputStream;
+import java.io.ObjectInputStream;
+import java.io.ObjectOutputStream;
+import java.nio.ByteBuffer;
+import java.util.HashMap;
+import java.util.Map;
Review Comment:
There seems to be an arbitrary reordering of imports based on your
particular style. In general, it's good to minimize these kinds of personal
style preferences changes in PRs to any project, as they can flip-flop
repeatedly between styles with different contributors.
Ideally, the project would normalize its imports. I don't know how this can
be done with Gradle, but I wrote a Maven plugin to do it:
https://github.com/revelc/impsort-maven-plugin
##########
lib/java/test/org/apache/thrift/partial/ExceptionAsserts.java:
##########
@@ -84,6 +85,10 @@ public static <E extends Exception> void assertThrows(
}
}
+ /**
+ * @deprecated use {@link
org.junit.jupiter.api.Assertions#assertThrows(Class, Executable)}
+ */
+ @Deprecated
Review Comment:
Why not just delete these helper methods? They probably shouldn't be used at
all anymore.
##########
lib/java/test/org/apache/thrift/test/EqualityTest.java:
##########
@@ -129,10 +129,11 @@ public static void main(String[] args) throws Exception {
package org.apache.thrift.test;
// Generated code
-import java.nio.ByteBuffer;
import thrift.test.JavaTestHelper;
+import java.nio.ByteBuffer;
Review Comment:
This import was moved, but there was a comment above it saying this code was
generated. I'm not sure what would have generated this code, because it doesn't
specify, but if that comment is still relevant, style changes should not be
made to this class. If it isn't relevant, then that comment that was above this
import before it moved should be deleted.
##########
lib/java/test/org/apache/thrift/async/TestTAsyncClient.java:
##########
@@ -1,13 +1,16 @@
package org.apache.thrift.async;
-import junit.framework.TestCase;
import org.apache.thrift.TException;
-
+import org.junit.jupiter.api.Test;
import thrift.test.Srv;
import thrift.test.Srv.AsyncClient;
-public class TestTAsyncClient extends TestCase {
+import static org.junit.jupiter.api.Assertions.fail;
Review Comment:
This case of using fail() should really use assertThrows(). There may be
other similar cases where fail() is used instead of assertThrows().
##########
lib/java/gradle/environment.gradle:
##########
@@ -71,7 +71,8 @@ dependencies {
compile "javax.annotation:javax.annotation-api:${javaxAnnotationVersion}"
compile "org.apache.commons:commons-lang3:3.12.0"
- testCompile "junit:junit:${junitVersion}"
+ testImplementation "org.junit.jupiter:junit-jupiter:${junitVersion}"
+ testImplementation "org.junit.jupiter:junit-jupiter-params:${junitVersion}"
Review Comment:
This might be redundant. I'm not sure, but I think the `junit-jupiter`
artifact includes `junit-jupiter-api`, `junit-jupiter-engine`, and
`junit-jupiter-params`. So, you might not need to include the params separately.
```suggestion
testImplementation "org.junit.jupiter:junit-jupiter:${junitVersion}"
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
