Re: [PR] [#4951] improvement(test): Reduce fields shares between different IT to make AbstractIT more independent. [gravitino]
yuqi1129 merged PR #4996: URL: https://github.com/apache/gravitino/pull/4996 -- 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]
Re: [PR] [#4951] improvement(test): Reduce fields shares between different IT to make AbstractIT more independent. [gravitino]
jerryshao commented on PR #4996: URL: https://github.com/apache/gravitino/pull/4996#issuecomment-2413298239 Please address the conflicts here. -- 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]
Re: [PR] [#4951] improvement(test): Reduce fields shares between different IT to make AbstractIT more independent. [gravitino]
mchades commented on code in PR #4996:
URL: https://github.com/apache/gravitino/pull/4996#discussion_r1800657782
##
trino-connector/integration-test/src/test/java/org/apache/gravitino/trino/connector/integration/test/TrinoQueryITBase.java:
##
@@ -68,14 +68,17 @@ public class TrinoQueryITBase {
protected static final String metalakeName = "test";
protected static GravitinoMetalake metalake;
- private static void setEnv() throws Exception {
+ private static BaseIT baseIT;
+
+ private void setEnv() throws Exception {
+baseIT = new BaseIT();
Review Comment:
If that's the case, I believe adding comments to the class `BaseIT` is
sufficient and there is no need to refactor 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] [#4951] improvement(test): Reduce fields shares between different IT to make AbstractIT more independent. [gravitino]
yuqi1129 commented on code in PR #4996:
URL: https://github.com/apache/gravitino/pull/4996#discussion_r1800653954
##
trino-connector/integration-test/src/test/java/org/apache/gravitino/trino/connector/integration/test/TrinoQueryITBase.java:
##
@@ -68,14 +68,17 @@ public class TrinoQueryITBase {
protected static final String metalakeName = "test";
protected static GravitinoMetalake metalake;
- private static void setEnv() throws Exception {
+ private static BaseIT baseIT;
+
+ private void setEnv() throws Exception {
+baseIT = new BaseIT();
Review Comment:
I think so, refactoring this part will be another work for me.
--
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]
Re: [PR] [#4951] improvement(test): Reduce fields shares between different IT to make AbstractIT more independent. [gravitino]
mchades commented on code in PR #4996:
URL: https://github.com/apache/gravitino/pull/4996#discussion_r1800622096
##
trino-connector/integration-test/src/test/java/org/apache/gravitino/trino/connector/integration/test/TrinoQueryITBase.java:
##
@@ -68,14 +68,17 @@ public class TrinoQueryITBase {
protected static final String metalakeName = "test";
protected static GravitinoMetalake metalake;
- private static void setEnv() throws Exception {
+ private static BaseIT baseIT;
+
+ private void setEnv() throws Exception {
+baseIT = new BaseIT();
Review Comment:
Let me summarize the differences between the two usages:
Extending `BaseIT` automatically starts the Gravitino server and client
before the test begins. If a developer wishes to start the Gravitino server
manually, they should create a `BaseIT` instance, is that correct?
--
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]
Re: [PR] [#4951] improvement(test): Reduce fields shares between different IT to make AbstractIT more independent. [gravitino]
diqiu50 commented on code in PR #4996:
URL: https://github.com/apache/gravitino/pull/4996#discussion_r1800401817
##
trino-connector/integration-test/src/test/java/org/apache/gravitino/trino/connector/integration/test/TrinoQueryITBase.java:
##
@@ -68,14 +68,17 @@ public class TrinoQueryITBase {
protected static final String metalakeName = "test";
protected static GravitinoMetalake metalake;
- private static void setEnv() throws Exception {
+ private static BaseIT baseIT;
+
+ private void setEnv() throws Exception {
+baseIT = new BaseIT();
Review Comment:
If refactoring is necessary, I think it would be better to move the logic
for starting the Gravitino server out of BaseIT. In BaseIT, the automatic
starting of the Gravitino server is implemented. Test classes that need to use
the default method to start the Gravitino server should inherit from BaseIT
--
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]
Re: [PR] [#4951] improvement(test): Reduce fields shares between different IT to make AbstractIT more independent. [gravitino]
diqiu50 commented on code in PR #4996:
URL: https://github.com/apache/gravitino/pull/4996#discussion_r1800395623
##
trino-connector/integration-test/src/test/java/org/apache/gravitino/trino/connector/integration/test/TrinoQueryITBase.java:
##
@@ -68,14 +68,17 @@ public class TrinoQueryITBase {
protected static final String metalakeName = "test";
protected static GravitinoMetalake metalake;
- private static void setEnv() throws Exception {
+ private static BaseIT baseIT;
+
+ private void setEnv() throws Exception {
+baseIT = new BaseIT();
Review Comment:
BaseIT is actually a utility for starting the Gravitino server and Gravitino
client for TrinoTesters.
In the TrinoQueryIT class, starting the Gravitino server is an option for
running the tests.
Therefore, it is not appropriate for the TrinoQueryIT class to inherit from
BaseIT
--
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]
Re: [PR] [#4951] improvement(test): Reduce fields shares between different IT to make AbstractIT more independent. [gravitino]
yuqi1129 commented on code in PR #4996:
URL: https://github.com/apache/gravitino/pull/4996#discussion_r1799603877
##
trino-connector/integration-test/src/test/java/org/apache/gravitino/trino/connector/integration/test/TrinoQueryITBase.java:
##
@@ -68,14 +68,17 @@ public class TrinoQueryITBase {
protected static final String metalakeName = "test";
protected static GravitinoMetalake metalake;
- private static void setEnv() throws Exception {
+ private static BaseIT baseIT;
+
+ private void setEnv() throws Exception {
+baseIT = new BaseIT();
Review Comment:
> What is the difference between creating an instance and inheritance
Some tests, for example, `SparkQueryRunner` just use `BaseIT` as a field to
start the `Gravitino` server and run TPCDS query. Others just take it as the
base test class.
> Can we add an abstract modifier to baseIT for uniform usage?
it's beyond the scope of this PR scope and we can do the refacor later with
the help of @FANNG1 @diqiu50 for the Spark and Trino related tests that use
`BaseIT` as the class field.
--
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]
Re: [PR] [#4951] improvement(test): Reduce fields shares between different IT to make AbstractIT more independent. [gravitino]
mchades commented on code in PR #4996:
URL: https://github.com/apache/gravitino/pull/4996#discussion_r1798996634
##
trino-connector/integration-test/src/test/java/org/apache/gravitino/trino/connector/integration/test/TrinoQueryITBase.java:
##
@@ -68,14 +68,17 @@ public class TrinoQueryITBase {
protected static final String metalakeName = "test";
protected static GravitinoMetalake metalake;
- private static void setEnv() throws Exception {
+ private static BaseIT baseIT;
+
+ private void setEnv() throws Exception {
+baseIT = new BaseIT();
Review Comment:
How to use `BaseIT`? What is the difference between creating an instance and
inheritance? Can we add an abstract modifier to `baseIT` for uniform usage?
--
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]
Re: [PR] [#4951] improvement(test): Reduce fields shares between different IT to make AbstractIT more independent. [gravitino]
yuqi1129 commented on code in PR #4996:
URL: https://github.com/apache/gravitino/pull/4996#discussion_r1798707585
##
integration-test-common/src/test/java/org/apache/gravitino/integration/test/util/AbstractIT.java:
##
@@ -58,61 +58,61 @@
import org.apache.gravitino.server.web.JettyServerConfig;
import org.junit.jupiter.api.AfterAll;
import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.api.TestInstance;
import org.junit.jupiter.api.extension.ExtendWith;
-import org.junit.jupiter.params.ParameterizedTest;
-import org.junit.jupiter.params.provider.CsvSource;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import org.testcontainers.shaded.org.awaitility.Awaitility;
@ExtendWith({PrintFuncNameExtension.class, CloseContainerExtension.class})
+@TestInstance(TestInstance.Lifecycle.PER_CLASS)
Review Comment:
OK, I have removed the redundant annotations.
--
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]
Re: [PR] [#4951] improvement(test): Reduce fields shares between different IT to make AbstractIT more independent. [gravitino]
diqiu50 commented on code in PR #4996:
URL: https://github.com/apache/gravitino/pull/4996#discussion_r1798703042
##
integration-test-common/src/test/java/org/apache/gravitino/integration/test/util/AbstractIT.java:
##
@@ -58,61 +58,61 @@
import org.apache.gravitino.server.web.JettyServerConfig;
import org.junit.jupiter.api.AfterAll;
import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.api.TestInstance;
import org.junit.jupiter.api.extension.ExtendWith;
-import org.junit.jupiter.params.ParameterizedTest;
-import org.junit.jupiter.params.provider.CsvSource;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import org.testcontainers.shaded.org.awaitility.Awaitility;
@ExtendWith({PrintFuncNameExtension.class, CloseContainerExtension.class})
+@TestInstance(TestInstance.Lifecycle.PER_CLASS)
Review Comment:
If that, we call remove all the annotation like
`@TestInstance(TestInstance.Lifecycle.PER_CLASS)` in the subclasses.
--
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]
Re: [PR] [#4951] improvement(test): Reduce fields shares between different IT to make AbstractIT more independent. [gravitino]
yuqi1129 commented on code in PR #4996:
URL: https://github.com/apache/gravitino/pull/4996#discussion_r1797625045
##
integration-test-common/src/test/java/org/apache/gravitino/integration/test/util/AbstractIT.java:
##
@@ -248,15 +248,8 @@ public static String startAndInitMySQLBackend() {
}
}
- @ParameterizedTest
- @CsvSource({
-"embedded, jdbcBackend",
-"embedded, kvBackend",
-"deploy, jdbcBackend",
-"deploy, kvBackend"
- })
Review Comment:
It's pointless and won't actually happen.
--
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]
Re: [PR] [#4951] improvement(test): Reduce fields shares between different IT to make AbstractIT more independent. [gravitino]
yuqi1129 commented on code in PR #4996:
URL: https://github.com/apache/gravitino/pull/4996#discussion_r1797624956
##
integration-test-common/src/test/java/org/apache/gravitino/integration/test/util/AbstractIT.java:
##
@@ -58,61 +58,61 @@
import org.apache.gravitino.server.web.JettyServerConfig;
import org.junit.jupiter.api.AfterAll;
import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.api.TestInstance;
import org.junit.jupiter.api.extension.ExtendWith;
-import org.junit.jupiter.params.ParameterizedTest;
-import org.junit.jupiter.params.provider.CsvSource;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import org.testcontainers.shaded.org.awaitility.Awaitility;
@ExtendWith({PrintFuncNameExtension.class, CloseContainerExtension.class})
+@TestInstance(TestInstance.Lifecycle.PER_CLASS)
Review Comment:
Yes. Please see `SparkHiveCatalogIT33` and `SparkHiveCatalogIT`
--
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]
Re: [PR] [#4951] improvement(test): Reduce fields shares between different IT to make AbstractIT more independent. [gravitino]
diqiu50 commented on code in PR #4996:
URL: https://github.com/apache/gravitino/pull/4996#discussion_r1797613036
##
integration-test-common/src/test/java/org/apache/gravitino/integration/test/util/AbstractIT.java:
##
@@ -58,61 +58,61 @@
import org.apache.gravitino.server.web.JettyServerConfig;
import org.junit.jupiter.api.AfterAll;
import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.api.TestInstance;
import org.junit.jupiter.api.extension.ExtendWith;
-import org.junit.jupiter.params.ParameterizedTest;
-import org.junit.jupiter.params.provider.CsvSource;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import org.testcontainers.shaded.org.awaitility.Awaitility;
@ExtendWith({PrintFuncNameExtension.class, CloseContainerExtension.class})
+@TestInstance(TestInstance.Lifecycle.PER_CLASS)
Review Comment:
Can this annotation be inherited by subclasses?
##
integration-test-common/src/test/java/org/apache/gravitino/integration/test/util/AbstractIT.java:
##
@@ -248,15 +248,8 @@ public static String startAndInitMySQLBackend() {
}
}
- @ParameterizedTest
- @CsvSource({
-"embedded, jdbcBackend",
-"embedded, kvBackend",
-"deploy, jdbcBackend",
-"deploy, kvBackend"
- })
Review Comment:
Would removing these have no effect?
--
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]
Re: [PR] [#4951] improvement(test): Reduce fields shares between different IT to make AbstractIT more independent. [gravitino]
mchades commented on code in PR #4996:
URL: https://github.com/apache/gravitino/pull/4996#discussion_r1797529498
##
integration-test-common/src/test/java/org/apache/gravitino/integration/test/util/AbstractIT.java:
##
@@ -58,61 +58,61 @@
import org.apache.gravitino.server.web.JettyServerConfig;
import org.junit.jupiter.api.AfterAll;
import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.api.TestInstance;
import org.junit.jupiter.api.extension.ExtendWith;
-import org.junit.jupiter.params.ParameterizedTest;
-import org.junit.jupiter.params.provider.CsvSource;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import org.testcontainers.shaded.org.awaitility.Awaitility;
@ExtendWith({PrintFuncNameExtension.class, CloseContainerExtension.class})
+@TestInstance(TestInstance.Lifecycle.PER_CLASS)
public class AbstractIT {
Review Comment:
suggest add `abstract` modifier
--
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]
Re: [PR] [#4951] improvement(test): Reduce fields shares between different IT to make AbstractIT more independent. [gravitino]
yuqi1129 commented on code in PR #4996:
URL: https://github.com/apache/gravitino/pull/4996#discussion_r1797525238
##
integration-test-common/build.gradle.kts:
##
@@ -43,6 +43,8 @@ dependencies {
testImplementation(libs.httpclient5)
testImplementation(libs.testcontainers)
testImplementation(libs.testcontainers.mysql)
+ testImplementation(libs.mysql.driver)
+ testImplementation(libs.postgresql.driver)
Review Comment:
I removed these two dependencies.
--
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]
Re: [PR] [#4951] improvement(test): Reduce fields shares between different IT to make AbstractIT more independent. [gravitino]
yuqi1129 commented on code in PR #4996:
URL: https://github.com/apache/gravitino/pull/4996#discussion_r1796601197
##
trino-connector/integration-test/src/test/java/org/apache/gravitino/trino/connector/integration/test/TrinoQueryITBase.java:
##
@@ -68,14 +68,17 @@ public class TrinoQueryITBase {
protected static final String metalakeName = "test";
protected static GravitinoMetalake metalake;
- private static void setEnv() throws Exception {
+ private static AbstractIT abstractIT;
+
+ private void setEnv() throws Exception {
+abstractIT = new AbstractIT();
Review Comment:
I have created a new class `BaseIT` that merely inherit `AbstractIT`.
--
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]
Re: [PR] [#4951] improvement(test): Reduce fields shares between different IT to make AbstractIT more independent. [gravitino]
yuqi1129 commented on code in PR #4996:
URL: https://github.com/apache/gravitino/pull/4996#discussion_r1796601197
##
trino-connector/integration-test/src/test/java/org/apache/gravitino/trino/connector/integration/test/TrinoQueryITBase.java:
##
@@ -68,14 +68,17 @@ public class TrinoQueryITBase {
protected static final String metalakeName = "test";
protected static GravitinoMetalake metalake;
- private static void setEnv() throws Exception {
+ private static AbstractIT abstractIT;
+
+ private void setEnv() throws Exception {
+abstractIT = new AbstractIT();
Review Comment:
Removed.
--
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]
Re: [PR] [#4951] improvement(test): Reduce fields shares between different IT to make AbstractIT more independent. [gravitino]
mchades commented on code in PR #4996:
URL: https://github.com/apache/gravitino/pull/4996#discussion_r1796557198
##
trino-connector/integration-test/src/test/java/org/apache/gravitino/trino/connector/integration/test/TrinoQueryITBase.java:
##
@@ -68,14 +68,17 @@ public class TrinoQueryITBase {
protected static final String metalakeName = "test";
protected static GravitinoMetalake metalake;
- private static void setEnv() throws Exception {
+ private static AbstractIT abstractIT;
+
+ private void setEnv() throws Exception {
+abstractIT = new AbstractIT();
Review Comment:
Is it possible for us to standardize the usage of `AbstractIT` class? Just
like its name, we set it as an abstract class that requires inheritance to use.
--
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]
Re: [PR] [#4951] improvement(test): Reduce fields shares between different IT to make AbstractIT more independent. [gravitino]
mchades commented on code in PR #4996:
URL: https://github.com/apache/gravitino/pull/4996#discussion_r1796396120
##
integration-test-common/build.gradle.kts:
##
@@ -43,6 +43,8 @@ dependencies {
testImplementation(libs.httpclient5)
testImplementation(libs.testcontainers)
testImplementation(libs.testcontainers.mysql)
+ testImplementation(libs.mysql.driver)
+ testImplementation(libs.postgresql.driver)
Review Comment:
Why does it work before then?
--
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]
Re: [PR] [#4951] improvement(test): Reduce fields shares between different IT to make AbstractIT more independent. [gravitino]
yuqi1129 commented on code in PR #4996:
URL: https://github.com/apache/gravitino/pull/4996#discussion_r1796394491
##
trino-connector/integration-test/src/test/java/org/apache/gravitino/trino/connector/integration/test/TrinoQueryTestTool.java:
##
@@ -217,8 +217,9 @@ public static void main(String[] args) throws Exception {
TrinoQueryITBase.autoStart = autoStart;
TrinoQueryITBase.autoStartGravitino = autoStartGravitino;
- TrinoQueryIT.setup();
+ // TrinoQueryIT.setup();
Review Comment:
done
--
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]
Re: [PR] [#4951] improvement(test): Reduce fields shares between different IT to make AbstractIT more independent. [gravitino]
yuqi1129 commented on code in PR #4996:
URL: https://github.com/apache/gravitino/pull/4996#discussion_r1796392677
##
integration-test-common/build.gradle.kts:
##
@@ -43,6 +43,8 @@ dependencies {
testImplementation(libs.httpclient5)
testImplementation(libs.testcontainers)
testImplementation(libs.testcontainers.mysql)
+ testImplementation(libs.mysql.driver)
+ testImplementation(libs.postgresql.driver)
Review Comment:
AbstractIT in `integration-test-common` needs `MySQL` and `PG` to start the
Gravitino server. If we want to use it in some test like `SparkQueryRunner`,
the dependency is missing.
--
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]
Re: [PR] [#4951] improvement(test): Reduce fields shares between different IT to make AbstractIT more independent. [gravitino]
yuqi1129 commented on code in PR #4996:
URL: https://github.com/apache/gravitino/pull/4996#discussion_r1796390829
##
trino-connector/integration-test/src/test/java/org/apache/gravitino/trino/connector/integration/test/TrinoQueryITBase.java:
##
@@ -68,14 +68,17 @@ public class TrinoQueryITBase {
protected static final String metalakeName = "test";
protected static GravitinoMetalake metalake;
- private static void setEnv() throws Exception {
+ private static AbstractIT abstractIT;
+
+ private void setEnv() throws Exception {
+abstractIT = new AbstractIT();
Review Comment:
`TrinoQueryITBase` needs to start Gravitino server but it's not the subclass
of `AbstractIT`, so we'd need to create a new instance.
Why does it work before then?
The method `startIntegrationTest` before hand is a static method, so we can
use `AbstractIT#startIntegrationTest`
directly.
##
spark-connector/spark-common/src/test/java/org/apache/gravitino/spark/connector/integration/test/sql/SparkQueryRunner.java:
##
@@ -83,6 +84,7 @@ public SparkQueryRunner(SparkTestConfig sparkTestConfig) {
}
initSparkEnv();
+abstractIT = new AbstractIT();
Review Comment:
see below.
--
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]
Re: [PR] [#4951] improvement(test): Reduce fields shares between different IT to make AbstractIT more independent. [gravitino]
mchades commented on code in PR #4996:
URL: https://github.com/apache/gravitino/pull/4996#discussion_r1795404339
##
trino-connector/integration-test/src/test/java/org/apache/gravitino/trino/connector/integration/test/TrinoQueryTestTool.java:
##
@@ -217,8 +217,9 @@ public static void main(String[] args) throws Exception {
TrinoQueryITBase.autoStart = autoStart;
TrinoQueryITBase.autoStartGravitino = autoStartGravitino;
- TrinoQueryIT.setup();
+ // TrinoQueryIT.setup();
Review Comment:
plz remove this
##
spark-connector/spark-common/src/test/java/org/apache/gravitino/spark/connector/integration/test/sql/SparkQueryRunner.java:
##
@@ -83,6 +84,7 @@ public SparkQueryRunner(SparkTestConfig sparkTestConfig) {
}
initSparkEnv();
+abstractIT = new AbstractIT();
Review Comment:
Why not use inheritance instead of a new instance?
##
integration-test-common/build.gradle.kts:
##
@@ -43,6 +43,8 @@ dependencies {
testImplementation(libs.httpclient5)
testImplementation(libs.testcontainers)
testImplementation(libs.testcontainers.mysql)
+ testImplementation(libs.mysql.driver)
+ testImplementation(libs.postgresql.driver)
Review Comment:
why add these dependencies?
##
trino-connector/integration-test/src/test/java/org/apache/gravitino/trino/connector/integration/test/TrinoQueryITBase.java:
##
@@ -68,14 +68,17 @@ public class TrinoQueryITBase {
protected static final String metalakeName = "test";
protected static GravitinoMetalake metalake;
- private static void setEnv() throws Exception {
+ private static AbstractIT abstractIT;
+
+ private void setEnv() throws Exception {
+abstractIT = new AbstractIT();
Review Comment:
Here as well, it is strange to create an instance named "abstractXX".
--
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]
Re: [PR] [#4951] improvement(test): Reduce fields shares between different IT to make AbstractIT more independent. [gravitino]
jerryshao commented on PR #4996: URL: https://github.com/apache/gravitino/pull/4996#issuecomment-240114 Ping @mchades . -- 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]
