reta commented on code in PR #105:
URL:
https://github.com/apache/flink-connector-elasticsearch/pull/105#discussion_r1621010106
##########
flink-connector-elasticsearch8/src/test/java/org/apache/flink/connector/elasticsearch/sink/ElasticsearchSinkBaseITCase.java:
##########
@@ -21,86 +21,144 @@
package org.apache.flink.connector.elasticsearch.sink;
+import org.apache.flink.connector.base.sink.writer.ElementConverter;
+import org.apache.flink.testutils.junit.extensions.parameterized.Parameter;
+import
org.apache.flink.testutils.junit.extensions.parameterized.ParameterizedTestExtension;
+import org.apache.flink.testutils.junit.extensions.parameterized.Parameters;
+
+import co.elastic.clients.elasticsearch.core.bulk.BulkOperationVariant;
+import co.elastic.clients.elasticsearch.core.bulk.IndexOperation;
import org.apache.http.HttpHost;
+import org.apache.http.auth.AuthScope;
+import org.apache.http.auth.UsernamePasswordCredentials;
+import org.apache.http.client.CredentialsProvider;
+import org.apache.http.impl.client.BasicCredentialsProvider;
import org.apache.http.util.EntityUtils;
import org.elasticsearch.client.Request;
import org.elasticsearch.client.Response;
import org.elasticsearch.client.RestClient;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.TestTemplate;
+import org.junit.jupiter.api.extension.ExtendWith;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import org.testcontainers.containers.output.Slf4jLogConsumer;
+import org.testcontainers.containers.wait.strategy.LogMessageWaitStrategy;
import org.testcontainers.containers.wait.strategy.Wait;
import org.testcontainers.elasticsearch.ElasticsearchContainer;
-import org.testcontainers.junit.jupiter.Container;
-import org.testcontainers.junit.jupiter.Testcontainers;
import org.testcontainers.utility.DockerImageName;
import java.io.IOException;
import java.time.Duration;
+import java.util.Arrays;
+import java.util.List;
import static org.assertj.core.api.Assertions.assertThat;
-/** Base Integration tests class. */
-@Testcontainers
-public class ElasticsearchSinkBaseITCase {
+/**
+ * {@link ElasticsearchSinkBaseITCase} is the base class for integration tests.
+ *
+ * <p>It is extended with the {@link ParameterizedTestExtension} for
parameterized testing against
+ * secure and non-secure Elasticsearch clusters. Tests must be annotated by
{@link TestTemplate} in
+ * order to be parameterized.
+ *
+ * <p>The cluster is running via test containers. In order to reuse the
singleton containers by all
+ * inheriting test classes, we manage their lifecycle. The two containers are
started only once when
+ * this class is loaded. At the end of the test suite the Ryuk container that
is started by
+ * Testcontainers core will take care of stopping the singleton container.
+ */
+@ExtendWith(ParameterizedTestExtension.class)
Review Comment:
> The idea of using ParameterizedTestExtension is to make all subclasses run
with both secure and non-secure tests by default. So tests do not actually
depend on one static container, but instead use both of them.
Oh, apologies, missed that, I think the approach makes perfect sense that.
> I have been reading the doc of testcontainers and didn't find a solution
that works best for base classes via @Container managed lifecycle and still
efficiently.
Got it, thank for reference but we still have a problem here since
containers are not shutdown properly (we do start them up), that the concerns I
have vs using `@Container`, let me try to look into that.
##########
flink-connector-elasticsearch8/src/test/java/org/apache/flink/connector/elasticsearch/sink/ElasticsearchSinkBaseITCase.java:
##########
@@ -21,86 +21,144 @@
package org.apache.flink.connector.elasticsearch.sink;
+import org.apache.flink.connector.base.sink.writer.ElementConverter;
+import org.apache.flink.testutils.junit.extensions.parameterized.Parameter;
+import
org.apache.flink.testutils.junit.extensions.parameterized.ParameterizedTestExtension;
+import org.apache.flink.testutils.junit.extensions.parameterized.Parameters;
+
+import co.elastic.clients.elasticsearch.core.bulk.BulkOperationVariant;
+import co.elastic.clients.elasticsearch.core.bulk.IndexOperation;
import org.apache.http.HttpHost;
+import org.apache.http.auth.AuthScope;
+import org.apache.http.auth.UsernamePasswordCredentials;
+import org.apache.http.client.CredentialsProvider;
+import org.apache.http.impl.client.BasicCredentialsProvider;
import org.apache.http.util.EntityUtils;
import org.elasticsearch.client.Request;
import org.elasticsearch.client.Response;
import org.elasticsearch.client.RestClient;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.TestTemplate;
+import org.junit.jupiter.api.extension.ExtendWith;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import org.testcontainers.containers.output.Slf4jLogConsumer;
+import org.testcontainers.containers.wait.strategy.LogMessageWaitStrategy;
import org.testcontainers.containers.wait.strategy.Wait;
import org.testcontainers.elasticsearch.ElasticsearchContainer;
-import org.testcontainers.junit.jupiter.Container;
-import org.testcontainers.junit.jupiter.Testcontainers;
import org.testcontainers.utility.DockerImageName;
import java.io.IOException;
import java.time.Duration;
+import java.util.Arrays;
+import java.util.List;
import static org.assertj.core.api.Assertions.assertThat;
-/** Base Integration tests class. */
-@Testcontainers
-public class ElasticsearchSinkBaseITCase {
+/**
+ * {@link ElasticsearchSinkBaseITCase} is the base class for integration tests.
+ *
+ * <p>It is extended with the {@link ParameterizedTestExtension} for
parameterized testing against
+ * secure and non-secure Elasticsearch clusters. Tests must be annotated by
{@link TestTemplate} in
+ * order to be parameterized.
+ *
+ * <p>The cluster is running via test containers. In order to reuse the
singleton containers by all
+ * inheriting test classes, we manage their lifecycle. The two containers are
started only once when
+ * this class is loaded. At the end of the test suite the Ryuk container that
is started by
+ * Testcontainers core will take care of stopping the singleton container.
+ */
+@ExtendWith(ParameterizedTestExtension.class)
Review Comment:
> The idea of using ParameterizedTestExtension is to make all subclasses run
with both secure and non-secure tests by default. So tests do not actually
depend on one static container, but instead use both of them.
Oh, apologies, missed that, I think the approach makes perfect sense that.
> I have been reading the doc of testcontainers and didn't find a solution
that works best for base classes via @Container managed lifecycle and still
efficiently.
Got it, thanks for reference but we still have a problem here since
containers are not shutdown properly (we do start them up), that the concerns I
have vs using `@Container`, let me try to look into that.
--
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-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
