You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "nastra (via GitHub)" <gi...@apache.org> on 2023/12/23 09:46:22 UTC
[PR] Spark 3.5: Remove constructor from parameterized base class [iceberg]
nastra opened a new pull request, #9368:
URL: https://github.com/apache/iceberg/pull/9368
Parameterized base classes should not have a constructor but rather have their parameters set via `@Parameter`. All other setup code should go into a `@BeforeEach` method to properly initialize the test environment
--
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] Spark 3.5: Remove constructor from parameterized base class [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra merged PR #9368:
URL: https://github.com/apache/iceberg/pull/9368
--
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] Spark 3.5: Remove constructor from parameterized base class [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #9368:
URL: https://github.com/apache/iceberg/pull/9368#discussion_r1435551103
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java:
##########
@@ -35,11 +38,25 @@
import org.assertj.core.api.Assertions;
import org.junit.jupiter.api.AfterAll;
import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.extension.ExtendWith;
import org.junit.jupiter.api.io.TempDir;
+@ExtendWith(ParameterizedTestExtension.class)
public abstract class TestBaseWithCatalog extends TestBase {
protected static File warehouse = null;
+ @Parameters(name = "catalogName = {0}, implementation = {1}, config = {2}")
+ protected static Object[][] parameters() {
+ return new Object[][] {
+ {
+ SparkCatalogConfig.HADOOP.catalogName(),
Review Comment:
all of the subclasses of this test that didn't specify any particular parameter were running with this configuration (this is also the configuration that was initialized in the default constructors in L70
--
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] Spark 3.5: Remove constructor from parameterized base class [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #9368:
URL: https://github.com/apache/iceberg/pull/9368#discussion_r1435551324
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/source/TestDataFrameWriterV2.java:
##########
@@ -50,7 +50,7 @@ public void removeTables() {
sql("DROP TABLE IF EXISTS %s", tableName);
}
- @Test
+ @TestTemplate
Review Comment:
all of the subclasses of `TestBaseWithCatalog` are parameterized 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] Spark 3.5: Remove constructor from parameterized base class [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #9368:
URL: https://github.com/apache/iceberg/pull/9368#discussion_r1435550886
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/CatalogTestBase.java:
##########
@@ -30,7 +29,7 @@ public abstract class CatalogTestBase extends TestBaseWithCatalog {
// these parameters are broken out to avoid changes that need to modify lots of test suites
@Parameters(name = "catalogName = {0}, implementation = {1}, config = {2}")
- public static Object[][] parameters() {
+ protected static Object[][] parameters() {
Review Comment:
JUnit4 required this to be public but for JUnit5 we can make it 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] Spark 3.5: Remove constructor from parameterized base class [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #9368:
URL: https://github.com/apache/iceberg/pull/9368#discussion_r1435551603
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/source/TestSparkStagedScan.java:
##########
@@ -39,11 +38,6 @@
public class TestSparkStagedScan extends CatalogTestBase {
- public TestSparkStagedScan(
Review Comment:
this and the class below were running with the three different catalog configurations defined in `CatalogTestBase` and I've verified that this is still the case
--
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