[#4951] improvement(test): Reduce fields shares between different IT to make AbstractIT more independent. #4996
Conversation
yuqi1129
changed the title
yuqi1129
changed the title
|
@mchades |
|
Ping @mchades . |
...rc/test/java/org/apache/gravitino/spark/connector/integration/test/sql/SparkQueryRunner.java
Outdated
Show resolved
Hide resolved
...st/src/test/java/org/apache/gravitino/trino/connector/integration/test/TrinoQueryITBase.java
Outdated
Show resolved
Hide resolved
.../src/test/java/org/apache/gravitino/trino/connector/integration/test/TrinoQueryTestTool.java
Outdated
Show resolved
Hide resolved
...gration-test-common/src/test/java/org/apache/gravitino/integration/test/util/AbstractIT.java
Outdated
Show resolved
Hide resolved
| 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) |
There was a problem hiding this comment.
Can this annotation be inherited by subclasses?
There was a problem hiding this comment.
Yes. Please see SparkHiveCatalogIT33 and SparkHiveCatalogIT
There was a problem hiding this comment.
If that, we call remove all the annotation like @TestInstance(TestInstance.Lifecycle.PER_CLASS) in the subclasses.
There was a problem hiding this comment.
OK, I have removed the redundant annotations.
| "embedded, kvBackend", | ||
| "deploy, jdbcBackend", | ||
| "deploy, kvBackend" | ||
| }) |
There was a problem hiding this comment.
Would removing these have no effect?
There was a problem hiding this comment.
It's pointless and won't actually happen.
| private static BaseIT baseIT; | ||
|
|
||
| private void setEnv() throws Exception { | ||
| baseIT = new BaseIT(); |
There was a problem hiding this 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?
There was a problem hiding this 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.
There was a problem hiding this 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
There was a problem hiding this 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
There was a problem hiding this 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?
There was a problem hiding this comment.
I think so, refactoring this part will be another work for me.
There was a problem hiding this comment.
If that's the case, I believe adding comments to the class BaseIT is sufficient and there is no need to refactor it.
|
Please address the conflicts here. |
What changes were proposed in this pull request?
Remove static fields of
AbstractITto make it more robust.Why are the changes needed?
To make it more robut.
Fix: #4951
Does this PR introduce any user-facing change?
N/A
How was this patch tested?
The existing test.