-
Notifications
You must be signed in to change notification settings - Fork 81
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[FLINK-35472] Improve tests for Elasticsearch 8 connector #105
base: main
Are you sure you want to change the base?
Conversation
cadc370
to
5e66fcd
Compare
@reta @snuyanzin Could you take a look? Thank you! |
* 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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @liuml07 , I think the usage of
@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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
*/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, @liuml07 !
@snuyanzin could you help review on this? Andriy and I don't have write permissions. Thank you! |
thanks for the contribution @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
I have rebased from the |
https://issues.apache.org/jira/browse/FLINK-35472