liuml07 commented on code in PR #105:
URL:
https://github.com/apache/flink-connector-elasticsearch/pull/105#discussion_r1619214117
##########
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:
Thanks for taking a look, @reta!
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. That way, we can
gain higher confidence when making specific changes to secure or non-secure
case. For example, adding a new NetworkConfig option, be it SSL related or
authentication provider.
Those secure and non-secure containers are set up automatically and are
singletons of the same JVM, which are reused by all test classes. Using
`@Container` would make each test class create it's own containers, as we
introduce more test classes, this seems sub-optimal.
Secondly, having two `@Container` in the base class meaning both containers
will be set up (for each class). If the subclass only chooses one to use via
`enableSecurity()`, the other one is just slowing down the tests without
providing any value.
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. The suggestion was to manually manage the lifecycle which is
simple enough to start it statically, and it will get closed automatically.[1]
[1]
https://java.testcontainers.org/test_framework_integration/manual_lifecycle_control/#singleton-containers
--
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]
