[FLINK-35472] Improve tests for Elasticsearch 8 connector

[liuml07]

https://issues.apache.org/jira/browse/FLINK-35472

Per discussion in this PR (thanks to @reta), it makes the tests more reusable if we use parameterized tests. It requires some changes of the existing tests, which includes:

• Make base test class parameterized with secure parameter. As JUnit 5 has limited support for parameterized tests with inheritance, we can use the ParameterizedTestExtension introduced in Flink, see this doc
• Manage the test container lifecycle instead of using the managed annotation @Testcontainers and @Container so that the test containers can be used as a singleton for all tests in the suite
• Create and use common methods in the base class that concrete test classes can be mostly parameter-agnostic
  This JIRA intends to not change any logic or functionality. Instead it focuses on tests refactoring for more reusable tests and future proof.

[liuml07]

@reta @snuyanzin Could you take a look? Thank you!

[reta]

Thanks @liuml07 , I think the usage of

Suggested change

	@ExtendWith(ParameterizedTestExtension.class)
	@ExtendWith(ParameterizedTestExtension.class)

is not really bringing value here since the test cases depend on static containers (and parameterized tests do not support that). However, I think you have quite interesting idea to explore, I would suggest to try that:

• create 2 containers (as you do now but using the @Testcontainers extension):

  @Container
   public static final ElasticsearchContainer ES_CONTAINER = createElasticsearchContainer();
  
  @Container
  public static final ElasticsearchContainer ES_SECURE_CONTAINER = createElasticsearchSecureContainer();
  

• consequently, use 2 RestClient clients, secure and non-secure
• introduce abstract method, fe abstract boolean enableSecurity() so each test case would have to override and the base test case could use to make the decision which client + customization to use

WDYT? Thank you.

[liuml07]

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

[liuml07]

Let me attach a screenshot when I run it locally. Both containers are used by each test case, and also the two containers are used by all test cases of all test classes.

[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.

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.

UPD: seems to be covered, from the docs you referenced [1]:

At the end of the test suite the Ryuk container that is started by Testcontainers core will take care of stopping the singleton container.

[1] https://java.testcontainers.org/test_framework_integration/manual_lifecycle_control/#singleton-containers

[liuml07]

Thank you very much for checking again!

This manual lifecycle management for singleton test containers is new to me as well. I presume Ryuk's delay-based container cleanup is not straightforward to many people, so I also put a comment in the base test class.

 *
 * <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.
 */

[reta]

Thank you, @liuml07 !

[liuml07]

@snuyanzin could you help review on this? Andriy and I don't have write permissions. Thank you!

[snuyanzin]

thanks for the contribution @liuml07
the ci is still failing, can you please have a look ?

[liuml07]

Thank you @snuyanzin for taking another look.

I see a lot of errors like this, and not sure if this is causing the compile & test to timeout

Warning: Could not transfer metadata org.apache.flink:flink-test-utils-junit:1.18-SNAPSHOT/maven-metadata.xml from/to apache.snapshots (https://repository.apache.org/snapshots): transfer failed for
... (repeat multiple lines) ...
Error: The action 'Compile and test' has timed out after 50 minutes.

I have rebased from the main branch and will check again.

[liuml07]

Error:  Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.8.0:testCompile (default-testCompile) on project flink-connector-elasticsearch-base: Compilation failure
Error:  /home/runner/work/flink-connector-elasticsearch/flink-connector-elasticsearch/flink-connector-elasticsearch-base/src/test/java/org/apache/flink/connector/elasticsearch/sink/ElasticsearchWriterITCase.java:[485,20] org.apache.flink.connector.elasticsearch.sink.ElasticsearchWriterITCase.TestMailbox is not abstract and does not override abstract method shouldInterrupt() in org.apache.flink.api.common.operators.MailboxExecutor

We didn't change the code over there for 6/7, and they don't fail locally. Interesting why it's failing for the PR.

[liuml07]

Rebasing from main branch.

[reswqa]

LGTM.