Jackie-Jiang commented on code in PR #8757:
URL: https://github.com/apache/pinot/pull/8757#discussion_r882019041
##########
pinot-connectors/pinot-flink-connector/src/test/java/org/apache/pinot/connector/flink/sink/PinotSinkIntegrationTest.java:
##########
@@ -64,7 +64,8 @@ public class PinotSinkIntegrationTest extends
BaseClusterIntegrationTest {
@BeforeClass
public void setUp()
throws Exception {
- TestUtils.ensureDirectoriesExistAndEmpty(_tempDir, _segmentDir, _tarDir);
+ setUpTestDirectories(this.getClass().getSimpleName());
+ TestUtils.ensureDirectoriesExistAndEmpty(getTempDir(), getSegmentDir(),
getTarDir());
Review Comment:
This can be merged into `setUpTestDirectories()`
##########
pinot-integration-test-base/src/test/java/org/apache/pinot/integration/tests/ClusterTest.java:
##########
@@ -84,27 +74,158 @@
/**
* Base class for integration tests that involve a complete Pinot cluster.
+ *
+ * Similar to {@link ControllerTest}, one should entirely encapsulate the
{@code ClusterTest} object as a
+ * singleton within a test group/suite; or instantiate one instance per test
class that requires setup.
*/
public abstract class ClusterTest extends ControllerTest {
Review Comment:
I think we can rename it to `DefaultTestCluster` and make it a regular class
instead of abstract class
##########
pinot-integration-test-base/src/test/java/org/apache/pinot/integration/tests/ClusterIntegrationTestUtils.java:
##########
@@ -527,17 +527,18 @@ private static Object generateRandomValue(Schema.Type
fieldType) {
/**
* Run equivalent Pinot and H2 query and compare the results.
*/
- static void testQuery(String pinotQuery, String brokerUrl,
org.apache.pinot.client.Connection pinotConnection,
- String h2Query, Connection h2Connection)
+ static void testQuery(ClusterTest testCluster, String pinotQuery, String
brokerUrl,
Review Comment:
Do we need this change? I don't see usage of `testCluster`. `postQuery()` is
a static method
##########
pinot-connectors/pinot-flink-connector/src/test/java/org/apache/pinot/connector/flink/sink/PinotSinkIntegrationTest.java:
##########
@@ -52,7 +52,7 @@
import org.testng.annotations.Test;
-public class PinotSinkIntegrationTest extends BaseClusterIntegrationTest {
+public class PinotSinkIntegrationTest extends ClusterTest {
Review Comment:
Why does it extend the ClusterTest? Should it create a `testCluster` instead?
##########
pinot-integration-test-base/src/test/java/org/apache/pinot/integration/tests/ClusterIntegrationTestDataAndQuerySet.java:
##########
@@ -52,14 +52,18 @@
* Shared set of common tests for cluster integration tests.
* <p>To enable the test, override it and add @Test annotation.
*/
-public abstract class BaseClusterIntegrationTestSet extends
BaseClusterIntegrationTest {
- private static final Logger LOGGER =
LoggerFactory.getLogger(BaseClusterIntegrationTestSet.class);
+public abstract class ClusterIntegrationTestDataAndQuerySet extends
ClusterIntegrationTestDataSet {
Review Comment:
Should we make it similar to `ClusterIntegrationTestDataSet` and move the
default settings into a separate 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
