nastra commented on code in PR #7758:
URL: https://github.com/apache/iceberg/pull/7758#discussion_r1215472692
##########
api/src/test/java/org/apache/iceberg/types/TestCharSeqComparator.java:
##########
@@ -52,18 +53,20 @@ public void testSeqLength() {
Comparator<CharSequence> cmp = Literal.of(s1).comparator();
// Sanity check that String.compareTo gives the same result
- Assert.assertTrue(
- "When one string is a substring of the other, the longer is greater",
s1.compareTo(s2) < 0);
- Assert.assertTrue(
- "When one string is a substring of the other, the longer is greater",
s2.compareTo(s1) > 0);
+ assertThat(s1)
+ .as("When one string is a substring of the other, the longer is
greater")
+ .isLessThan(s2);
+ assertThat(s2)
+ .as("When one string is a substring of the other, the longer is
greater")
+ .isGreaterThan(s1);
// Test the comparator
- Assert.assertTrue(
- "When one string is a substring of the other, the longer is greater",
- cmp.compare(s1, s2) < 0);
- Assert.assertTrue(
- "When one string is a substring of the other, the longer is greater",
- cmp.compare(s2, s1) > 0);
+ assertThat(cmp.compare(s1, s2))
Review Comment:
this seems inconsistent with the assertions a few lines above. I'd suggest
to pick one of the approaches and go with that (preferrably using
`isLessThan()` / `isGreatherThan()` as that is more descriptive)
##########
api/src/test/java/org/apache/iceberg/types/TestCharSeqComparator.java:
##########
@@ -52,18 +53,20 @@ public void testSeqLength() {
Comparator<CharSequence> cmp = Literal.of(s1).comparator();
// Sanity check that String.compareTo gives the same result
- Assert.assertTrue(
- "When one string is a substring of the other, the longer is greater",
s1.compareTo(s2) < 0);
- Assert.assertTrue(
- "When one string is a substring of the other, the longer is greater",
s2.compareTo(s1) > 0);
+ assertThat(s1)
+ .as("When one string is a substring of the other, the longer is
greater")
+ .isLessThan(s2);
Review Comment:
can we use the same assertion check in `TestBinaryComparator`?
##########
api/src/test/java/org/apache/iceberg/TestIcebergBuild.java:
##########
@@ -18,33 +18,40 @@
*/
package org.apache.iceberg;
+import static org.assertj.core.api.Assertions.assertThat;
+
import java.util.Locale;
import java.util.regex.Pattern;
-import org.junit.Assert;
-import org.junit.Test;
+import org.junit.jupiter.api.Test;
public class TestIcebergBuild {
@Test
public void testFullVersion() {
- Assert.assertEquals(
- "Should build full version from version and commit ID",
- "Apache Iceberg " + IcebergBuild.version() + " (commit " +
IcebergBuild.gitCommitId() + ")",
- IcebergBuild.fullVersion());
+ assertThat(IcebergBuild.fullVersion())
+ .as("Should build full version from version and commit ID")
+ .isEqualTo(
+ "Apache Iceberg "
+ + IcebergBuild.version()
+ + " (commit "
+ + IcebergBuild.gitCommitId()
+ + ")");
}
@Test
public void testVersion() {
- Assert.assertNotEquals("Should not use unknown version", "unknown",
IcebergBuild.version());
+ assertThat(IcebergBuild.version()).as("Should not use unknown
version").isNotEqualTo("unknown");
}
@Test
public void testGitCommitId() {
- Assert.assertNotEquals(
- "Should not use unknown commit ID", "unknown",
IcebergBuild.gitCommitId());
- Assert.assertTrue(
- "Should be a hexadecimal string of 20 bytes",
- Pattern.compile("[0-9a-f]{40}")
- .matcher(IcebergBuild.gitCommitId().toLowerCase(Locale.ROOT))
- .matches());
+ assertThat(IcebergBuild.gitCommitId())
+ .as("Should not use unknown commit ID")
+ .isNotEqualTo("unknown");
+ assertThat(
+ Pattern.compile("[0-9a-f]{40}")
+ .matcher(IcebergBuild.gitCommitId().toLowerCase(Locale.ROOT))
+ .matches())
+ .as("Should be a hexadecimal string of 20 bytes")
+ .isTrue();
Review Comment:
AssertJ has `matches()` so better to use that instead of `isTrue()`
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
