You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "tomtongue (via GitHub)" <gi...@apache.org> on 2023/11/22 08:47:57 UTC
[PR] Create SparkCatalogTestBase class for the migration to JUnit5 [iceberg]
tomtongue opened a new pull request, #9129:
URL: https://github.com/apache/iceberg/pull/9129
Base class creation for the migration to JUnit5 in regards to #9076 . Let me update other 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: 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
Re: [PR] Create SparkCatalogTestBase class for the migration to JUnit5 [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #9129:
URL: https://github.com/apache/iceberg/pull/9129#discussion_r1404012356
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/actions/TestSparkFileRewriter.java:
##########
@@ -190,9 +189,9 @@ private void checkDataFileGroupWithTooMuchData(SizeBasedDataRewriter rewriter) {
rewriter.init(options);
Iterable<List<FileScanTask>> groups = rewriter.planFileGroups(tasks);
- Assert.assertEquals("Must have 1 group", 1, Iterables.size(groups));
+ Assertions.assertThat(Iterables.size(groups)).as("Must have 1 group").isEqualTo(1);
List<FileScanTask> group = Iterables.getOnlyElement(groups);
- Assert.assertEquals("Must rewrite big file", 1, group.size());
+ Assertions.assertThat(group.size()).as("Must rewrite big file").isEqualTo(1);
Review Comment:
please adjust all of these to use `Assertions.assertThat(groups).as(..).hasSize(1);`
--
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
Re: [PR] Create SparkCatalogTestBase class for the migration to JUnit5 [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on PR #9129:
URL: https://github.com/apache/iceberg/pull/9129#issuecomment-1823947363
> Add the following classes that basically cover JUnit5 and AssertJ styles. Each class has a lot of inheritants so that new classes are created for now. And, can I update existing Spark tests to use the new base classes
Let's first get the new base classes in and convert a single test to the new base class. Then we can handle other classes in a separate PR
--
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
Re: [PR] Create SparkCatalogTestBase class for the migration to JUnit5 [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on PR #9129:
URL: https://github.com/apache/iceberg/pull/9129#issuecomment-1822596157
@tomtongue I think we first need to have a JUnit5 equivalent of `SparkTestBase` and `SparkTestBaseWithCatalog` before we can migrate `SparkCatalogTestBase`
--
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
Re: [PR] Create SparkCatalogTestBase class for the migration to JUnit5 [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #9129:
URL: https://github.com/apache/iceberg/pull/9129#discussion_r1403012159
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java:
##########
@@ -0,0 +1,131 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.spark;
+
+import java.io.File;
+import java.io.IOException;
+import java.util.Map;
+import org.apache.hadoop.fs.FileSystem;
+import org.apache.hadoop.fs.Path;
+import org.apache.iceberg.CatalogProperties;
+import org.apache.iceberg.PlanningMode;
+import org.apache.iceberg.TableProperties;
+import org.apache.iceberg.catalog.Catalog;
+import org.apache.iceberg.catalog.Namespace;
+import org.apache.iceberg.catalog.SupportsNamespaces;
+import org.apache.iceberg.catalog.TableIdentifier;
+import org.apache.iceberg.hadoop.HadoopCatalog;
+import org.apache.iceberg.util.PropertyUtil;
+import org.assertj.core.api.Assertions;
+import org.junit.jupiter.api.AfterAll;
+import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.api.io.TempDir;
+
+public abstract class TestBaseWithCatalog extends TestBase {
+ protected static File warehouse = null;
+
+ @BeforeAll
+ public static void createWarehouse() throws IOException {
+ TestBaseWithCatalog.warehouse = File.createTempFile("warehouse", null);
+ Assertions.assertThat(warehouse.delete()).isTrue();
+ }
+
+ @AfterAll
+ public static void dropWarehouse() throws IOException {
+ if (warehouse != null && warehouse.exists()) {
+ Path warehousePath = new Path(warehouse.getAbsolutePath());
+ FileSystem fs = warehousePath.getFileSystem(hiveConf);
+ Assertions.assertThat(fs.delete(warehousePath, true))
+ .as("Failed to delete " + warehousePath)
+ .isTrue();
+ }
+ }
+
+ @TempDir public File temp;
Review Comment:
for JUnit5 this can be `protected`
--
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
Re: [PR] Create SparkCatalogTestBase class for the migration to JUnit5 [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #9129:
URL: https://github.com/apache/iceberg/pull/9129#discussion_r1404009911
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/SparkTestHelperBase.java:
##########
@@ -55,12 +55,15 @@ private Object[] toJava(Row row) {
protected void assertEquals(
String context, List<Object[]> expectedRows, List<Object[]> actualRows) {
- Assert.assertEquals(
- context + ": number of results should match", expectedRows.size(), actualRows.size());
+ Assertions.assertThat(actualRows.size())
Review Comment:
it's better to have
```
Assertions.assertThat(actualRows)
.as(context + ": number of results should match")
.hasSameSizeAs(expectedRows);
```
because this will show the content when the assertion ever fails
--
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
Re: [PR] Create SparkCatalogTestBase class for the migration to JUnit5 [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #9129:
URL: https://github.com/apache/iceberg/pull/9129#discussion_r1404011633
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/actions/TestSparkFileRewriter.java:
##########
@@ -110,9 +109,9 @@ private void checkDataFileSizeFiltering(SizeBasedDataRewriter rewriter) {
rewriter.init(options);
Iterable<List<FileScanTask>> groups = rewriter.planFileGroups(tasks);
- Assert.assertEquals("Must have 1 group", 1, Iterables.size(groups));
+ Assertions.assertThat(Iterables.size(groups)).as("Must have 1 group").isEqualTo(1);
Review Comment:
```suggestion
Assertions.assertThat(groups).hasSize(1);
```
--
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
Re: [PR] Create SparkCatalogTestBase class for the migration to JUnit5 [iceberg]
Posted by "tomtongue (via GitHub)" <gi...@apache.org>.
tomtongue commented on PR #9129:
URL: https://github.com/apache/iceberg/pull/9129#issuecomment-1825406804
Thanks for the review again. Change the tests that are commented here. I reviewed all my changes on my end again, but If there're any part that should be changed, please let me know @nastra
--
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
Re: [PR] Create SparkCatalogTestBase class for the migration to JUnit5 [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra merged PR #9129:
URL: https://github.com/apache/iceberg/pull/9129
--
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
Re: [PR] Create SparkCatalogTestBase class for the migration to JUnit5 [iceberg]
Posted by "tomtongue (via GitHub)" <gi...@apache.org>.
tomtongue commented on PR #9129:
URL: https://github.com/apache/iceberg/pull/9129#issuecomment-1822698391
Thank you! I'm working on changes. Let me do this.
--
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
Re: [PR] Create SparkCatalogTestBase class for the migration to JUnit5 [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #9129:
URL: https://github.com/apache/iceberg/pull/9129#discussion_r1404011983
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/actions/TestSparkFileRewriter.java:
##########
@@ -110,9 +109,9 @@ private void checkDataFileSizeFiltering(SizeBasedDataRewriter rewriter) {
rewriter.init(options);
Iterable<List<FileScanTask>> groups = rewriter.planFileGroups(tasks);
- Assert.assertEquals("Must have 1 group", 1, Iterables.size(groups));
+ Assertions.assertThat(Iterables.size(groups)).as("Must have 1 group").isEqualTo(1);
List<FileScanTask> group = Iterables.getOnlyElement(groups);
- Assert.assertEquals("Must rewrite 2 files", 2, group.size());
+ Assertions.assertThat(group.size()).as("Must rewrite 2 files").isEqualTo(2);
Review Comment:
same as above
--
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
Re: [PR] Create SparkCatalogTestBase class for the migration to JUnit5 [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #9129:
URL: https://github.com/apache/iceberg/pull/9129#discussion_r1403012486
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/CatalogTestBase.java:
##########
@@ -0,0 +1,55 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.spark;
+
+import java.io.File;
+import java.util.Map;
+import java.util.stream.Stream;
+import org.junit.jupiter.api.io.TempDir;
+import org.junit.jupiter.params.provider.Arguments;
+
+public abstract class CatalogTestBase extends TestBaseWithCatalog {
+
+ // these parameters are broken out to avoid changes that need to modify lots of test suites
+ public static Stream<Arguments> parameters() {
+ return Stream.of(
+ Arguments.of(
+ SparkCatalogConfig.HIVE.catalogName(),
+ SparkCatalogConfig.HIVE.implementation(),
+ SparkCatalogConfig.HIVE.properties()),
+ Arguments.of(
+ SparkCatalogConfig.HADOOP.catalogName(),
+ SparkCatalogConfig.HADOOP.implementation(),
+ SparkCatalogConfig.HADOOP.properties()),
+ Arguments.of(
+ SparkCatalogConfig.SPARK.catalogName(),
+ SparkCatalogConfig.SPARK.implementation(),
+ SparkCatalogConfig.SPARK.properties()));
+ }
+
+ @TempDir public File temp;
Review Comment:
can be protected
--
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
Re: [PR] Create SparkCatalogTestBase class for the migration to JUnit5 [iceberg]
Posted by "tomtongue (via GitHub)" <gi...@apache.org>.
tomtongue commented on code in PR #9129:
URL: https://github.com/apache/iceberg/pull/9129#discussion_r1403559874
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestHelperBase.java:
##########
@@ -0,0 +1,96 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.spark;
+
+import java.util.List;
+import java.util.stream.Collectors;
+import java.util.stream.IntStream;
+import org.apache.spark.sql.Row;
+import org.assertj.core.api.Assertions;
+import org.junit.Assert;
+
+public class TestHelperBase {
Review Comment:
Yes, that's correct, thank you! I remove `TestHelperBase` and add changes to `SparkTestHelperBase` with JUnit5 updates.
--
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
Re: [PR] Create SparkCatalogTestBase class for the migration to JUnit5 [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #9129:
URL: https://github.com/apache/iceberg/pull/9129#discussion_r1401906293
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/SparkCatalogTestBaseForJU5.java:
##########
@@ -0,0 +1,62 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.spark;
+
+import java.io.File;
+import java.util.Map;
+import org.junit.jupiter.api.io.TempDir;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.MethodSource;
+
+public abstract class SparkCatalogTestBaseForJU5 extends SparkTestBaseWithCatalog {
Review Comment:
rather than having a `JU5` suffix, let's name this class `CatalogTestBase`
--
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
Re: [PR] Create SparkCatalogTestBase class for the migration to JUnit5 [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #9129:
URL: https://github.com/apache/iceberg/pull/9129#discussion_r1404011518
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/actions/TestSparkFileRewriter.java:
##########
@@ -129,9 +128,9 @@ private void checkDataFilesDeleteThreshold(SizeBasedDataRewriter rewriter) {
rewriter.init(options);
Iterable<List<FileScanTask>> groups = rewriter.planFileGroups(tasks);
- Assert.assertEquals("Must have 1 group", 1, Iterables.size(groups));
+ Assertions.assertThat(Iterables.size(groups)).as("Must have 1 group").isEqualTo(1);
Review Comment:
```suggestion
Assertions.assertThat(groups).hasSize(1);
```
--
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
Re: [PR] Create SparkCatalogTestBase class for the migration to JUnit5 [iceberg]
Posted by "tomtongue (via GitHub)" <gi...@apache.org>.
tomtongue commented on code in PR #9129:
URL: https://github.com/apache/iceberg/pull/9129#discussion_r1403559874
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestHelperBase.java:
##########
@@ -0,0 +1,96 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.spark;
+
+import java.util.List;
+import java.util.stream.Collectors;
+import java.util.stream.IntStream;
+import org.apache.spark.sql.Row;
+import org.assertj.core.api.Assertions;
+import org.junit.Assert;
+
+public class TestHelperBase {
Review Comment:
Yes, that's correct, I remove `TestHelperBase` and add changes to `SparkTestHelperBase` with JUnit5 updates.
--
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
Re: [PR] Create SparkCatalogTestBase class for the migration to JUnit5 [iceberg]
Posted by "tomtongue (via GitHub)" <gi...@apache.org>.
tomtongue commented on PR #9129:
URL: https://github.com/apache/iceberg/pull/9129#issuecomment-1822339977
Working on updating other tests for JUnit5 and AssertJ.
--
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
Re: [PR] Create SparkCatalogTestBase class for the migration to JUnit5 [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #9129:
URL: https://github.com/apache/iceberg/pull/9129#discussion_r1403010787
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBase.java:
##########
@@ -0,0 +1,287 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.spark;
+
+import static org.apache.hadoop.hive.conf.HiveConf.ConfVars.METASTOREURIS;
+
+import java.io.IOException;
+import java.io.UncheckedIOException;
+import java.net.URI;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+import java.util.List;
+import java.util.Map;
+import java.util.TimeZone;
+import java.util.concurrent.TimeoutException;
+import java.util.concurrent.atomic.AtomicReference;
+import org.apache.hadoop.hive.conf.HiveConf;
+import org.apache.iceberg.CatalogUtil;
+import org.apache.iceberg.ContentFile;
+import org.apache.iceberg.catalog.Namespace;
+import org.apache.iceberg.exceptions.AlreadyExistsException;
+import org.apache.iceberg.hive.HiveCatalog;
+import org.apache.iceberg.hive.TestHiveMetastore;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableList;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap;
+import org.apache.iceberg.relocated.com.google.common.collect.Iterables;
+import org.apache.iceberg.relocated.com.google.common.collect.Maps;
+import org.apache.spark.api.java.JavaSparkContext;
+import org.apache.spark.sql.Dataset;
+import org.apache.spark.sql.Encoders;
+import org.apache.spark.sql.Row;
+import org.apache.spark.sql.SparkSession;
+import org.apache.spark.sql.catalyst.analysis.NoSuchTableException;
+import org.apache.spark.sql.execution.QueryExecution;
+import org.apache.spark.sql.execution.SparkPlan;
+import org.apache.spark.sql.execution.adaptive.AdaptiveSparkPlanExec;
+import org.apache.spark.sql.internal.SQLConf;
+import org.apache.spark.sql.util.QueryExecutionListener;
+import org.assertj.core.api.Assertions;
+import org.junit.jupiter.api.AfterAll;
+import org.junit.jupiter.api.BeforeAll;
+
+public abstract class TestBase extends TestHelperBase {
Review Comment:
this should extend `SparkTestHelperBase`
--
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
Re: [PR] Create SparkCatalogTestBase class for the migration to JUnit5 [iceberg]
Posted by "tomtongue (via GitHub)" <gi...@apache.org>.
tomtongue commented on code in PR #9129:
URL: https://github.com/apache/iceberg/pull/9129#discussion_r1404151629
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/SparkTestHelperBase.java:
##########
@@ -55,12 +55,15 @@ private Object[] toJava(Row row) {
protected void assertEquals(
String context, List<Object[]> expectedRows, List<Object[]> actualRows) {
- Assert.assertEquals(
- context + ": number of results should match", expectedRows.size(), actualRows.size());
+ Assertions.assertThat(actualRows.size())
Review Comment:
Confirmed that on my end, thanks. Update the 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: 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
Re: [PR] Create SparkCatalogTestBase class for the migration to JUnit5 [iceberg]
Posted by "tomtongue (via GitHub)" <gi...@apache.org>.
tomtongue commented on PR #9129:
URL: https://github.com/apache/iceberg/pull/9129#issuecomment-1823273318
Add the following classes that basically cover JUnit5 and AssertJ styles. Each class has a lot of inheritants so that new classes are created for now. @nastra
--
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
Re: [PR] Create SparkCatalogTestBase class for the migration to JUnit5 [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #9129:
URL: https://github.com/apache/iceberg/pull/9129#discussion_r1404010724
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/SparkTestHelperBase.java:
##########
@@ -69,19 +72,25 @@ protected void assertEquals(
}
protected void assertEquals(String context, Object[] expectedRow, Object[] actualRow) {
- Assert.assertEquals("Number of columns should match", expectedRow.length, actualRow.length);
+ Assertions.assertThat(actualRow.length)
+ .as("Number of columns should match")
+ .isEqualTo(expectedRow.length);
for (int col = 0; col < actualRow.length; col += 1) {
Object expectedValue = expectedRow[col];
Object actualValue = actualRow[col];
if (expectedValue != null && expectedValue.getClass().isArray()) {
String newContext = String.format("%s (nested col %d)", context, col + 1);
if (expectedValue instanceof byte[]) {
- Assert.assertArrayEquals(newContext, (byte[]) expectedValue, (byte[]) actualValue);
+ Assertions.assertThat((byte[]) actualValue)
+ .as(newContext)
+ .isEqualTo((byte[]) expectedValue);
Review Comment:
this cast shouldn't be required
--
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
Re: [PR] Create SparkCatalogTestBase class for the migration to JUnit5 [iceberg]
Posted by "tomtongue (via GitHub)" <gi...@apache.org>.
tomtongue commented on code in PR #9129:
URL: https://github.com/apache/iceberg/pull/9129#discussion_r1404014878
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/actions/TestSparkFileRewriter.java:
##########
@@ -190,9 +189,9 @@ private void checkDataFileGroupWithTooMuchData(SizeBasedDataRewriter rewriter) {
rewriter.init(options);
Iterable<List<FileScanTask>> groups = rewriter.planFileGroups(tasks);
- Assert.assertEquals("Must have 1 group", 1, Iterables.size(groups));
+ Assertions.assertThat(Iterables.size(groups)).as("Must have 1 group").isEqualTo(1);
List<FileScanTask> group = Iterables.getOnlyElement(groups);
- Assert.assertEquals("Must rewrite big file", 1, group.size());
+ Assertions.assertThat(group.size()).as("Must rewrite big file").isEqualTo(1);
Review Comment:
sure, thanks for the review. I'm working on 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: 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
Re: [PR] Create SparkCatalogTestBase class for the migration to JUnit5 [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #9129:
URL: https://github.com/apache/iceberg/pull/9129#discussion_r1403010020
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestHelperBase.java:
##########
@@ -0,0 +1,96 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.spark;
+
+import java.util.List;
+import java.util.stream.Collectors;
+import java.util.stream.IntStream;
+import org.apache.spark.sql.Row;
+import org.assertj.core.api.Assertions;
+import org.junit.Assert;
+
+public class TestHelperBase {
Review Comment:
I don't think we need this class, we can just use `SparkTestHelperBase` because there's nothing JUnit4-specific in there
--
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
Re: [PR] Create SparkCatalogTestBase class for the migration to JUnit5 [iceberg]
Posted by "tomtongue (via GitHub)" <gi...@apache.org>.
tomtongue commented on PR #9129:
URL: https://github.com/apache/iceberg/pull/9129#issuecomment-1824673027
Thanks for the review, @nastra! I add the following changes based on the reviews:
* Change `SparkTestHelperBase` with the AssertJ style (removed `TestHelperBase`)
* Switch `TestSparkFileRewriter`'s super class to `TestBase` (from `SparkTestBase`)
* Change `TestSparkFileRewriter` with the AssertJ style
* Remove `temp` from `CatalogTestBase` and change `temp` scope in its super class to `protected`
--
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
Re: [PR] Create SparkCatalogTestBase class for the migration to JUnit5 [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #9129:
URL: https://github.com/apache/iceberg/pull/9129#discussion_r1403014630
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/CatalogTestBase.java:
##########
@@ -0,0 +1,55 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.spark;
+
+import java.io.File;
+import java.util.Map;
+import java.util.stream.Stream;
+import org.junit.jupiter.api.io.TempDir;
+import org.junit.jupiter.params.provider.Arguments;
+
+public abstract class CatalogTestBase extends TestBaseWithCatalog {
+
+ // these parameters are broken out to avoid changes that need to modify lots of test suites
+ public static Stream<Arguments> parameters() {
+ return Stream.of(
+ Arguments.of(
+ SparkCatalogConfig.HIVE.catalogName(),
+ SparkCatalogConfig.HIVE.implementation(),
+ SparkCatalogConfig.HIVE.properties()),
+ Arguments.of(
+ SparkCatalogConfig.HADOOP.catalogName(),
+ SparkCatalogConfig.HADOOP.implementation(),
+ SparkCatalogConfig.HADOOP.properties()),
+ Arguments.of(
+ SparkCatalogConfig.SPARK.catalogName(),
+ SparkCatalogConfig.SPARK.implementation(),
+ SparkCatalogConfig.SPARK.properties()));
+ }
+
+ @TempDir public File temp;
Review Comment:
actually I don't think we need this here, because we already have it in the super class
--
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
Re: [PR] Create SparkCatalogTestBase class for the migration to JUnit5 [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #9129:
URL: https://github.com/apache/iceberg/pull/9129#discussion_r1404010177
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/SparkTestHelperBase.java:
##########
@@ -55,12 +55,15 @@ private Object[] toJava(Row row) {
protected void assertEquals(
String context, List<Object[]> expectedRows, List<Object[]> actualRows) {
- Assert.assertEquals(
- context + ": number of results should match", expectedRows.size(), actualRows.size());
+ Assertions.assertThat(actualRows.size())
+ .as(context + ": number of results should match")
+ .isEqualTo(expectedRows.size());
for (int row = 0; row < expectedRows.size(); row += 1) {
Object[] expected = expectedRows.get(row);
Object[] actual = actualRows.get(row);
- Assert.assertEquals("Number of columns should match", expected.length, actual.length);
+ Assertions.assertThat(actual.length)
Review Comment:
for the same reason that I mentioned above, it's better to have `Assertions.assertThat(actual).as("Number of columns should match").hasSameSizeAs(expected);`
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/SparkTestHelperBase.java:
##########
@@ -69,19 +72,25 @@ protected void assertEquals(
}
protected void assertEquals(String context, Object[] expectedRow, Object[] actualRow) {
- Assert.assertEquals("Number of columns should match", expectedRow.length, actualRow.length);
+ Assertions.assertThat(actualRow.length)
Review Comment:
same as above
--
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