You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "rakesh-das08 (via GitHub)" <gi...@apache.org> on 2023/06/03 11:27:31 UTC
[GitHub] [iceberg] rakesh-das08 opened a new pull request, #7758: Api: Switch tests to Junit5 (third PR for #7730)
rakesh-das08 opened a new pull request, #7758:
URL: https://github.com/apache/iceberg/pull/7758
(no comment)
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] nastra merged pull request #7758: API: Switch tests to Junit5 (parent, types, util packages)
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra merged PR #7758:
URL: https://github.com/apache/iceberg/pull/7758
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] nastra commented on a diff in pull request #7758: Api: Switch tests to Junit5 (parent, types, util packages)
Posted by "nastra (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rakesh-das08 commented on a diff in pull request #7758: Api: Switch tests to Junit5 (parent, types, util packages)
Posted by "rakesh-das08 (via GitHub)" <gi...@apache.org>.
rakesh-das08 commented on code in PR #7758:
URL: https://github.com/apache/iceberg/pull/7758#discussion_r1215637230
##########
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:
The inconsistency is because of the method under test. At one place we are testing `string.compareTo` whereas at one place we are testing the `Comparator.compare` method.
Please let me know if am missing something.
Thank you.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rakesh-das08 commented on a diff in pull request #7758: Api: Switch tests to Junit5 (parent, types, util packages)
Posted by "rakesh-das08 (via GitHub)" <gi...@apache.org>.
rakesh-das08 commented on code in PR #7758:
URL: https://github.com/apache/iceberg/pull/7758#discussion_r1216498728
##########
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:
In `TestBinaryComparator` the method under test is `Comparator.compare`
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rakesh-das08 commented on a diff in pull request #7758: Api: Switch tests to Junit5 (parent, types, util packages)
Posted by "rakesh-das08 (via GitHub)" <gi...@apache.org>.
rakesh-das08 commented on code in PR #7758:
URL: https://github.com/apache/iceberg/pull/7758#discussion_r1216498728
##########
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:
Here also the method under test is `Comparator.compare`
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rakesh-das08 commented on a diff in pull request #7758: Api: Switch tests to Junit5 (parent, types, util packages)
Posted by "rakesh-das08 (via GitHub)" <gi...@apache.org>.
rakesh-das08 commented on code in PR #7758:
URL: https://github.com/apache/iceberg/pull/7758#discussion_r1215642780
##########
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:
Sure
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] nastra commented on a diff in pull request #7758: Api: Switch tests to Junit5 (parent, types, util packages)
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #7758:
URL: https://github.com/apache/iceberg/pull/7758#discussion_r1216397734
##########
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:
ah I see, thanks for clarifying
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org