[GitHub] [beam] timrobertson100 commented on a change in pull request #14933: [BEAM-12421] Migrates ElasticsearchIOTest to use testContainers

[GitBox <gi...@apache.org>]

timrobertson100 commented on a change in pull request #14933:
URL: https://github.com/apache/beam/pull/14933#discussion_r646461212



##########
File path: sdks/java/io/elasticsearch-tests/elasticsearch-tests-7/src/test/java/org/apache/beam/sdk/io/elasticsearch/ElasticsearchIOTest.java
##########
@@ -46,63 +45,39 @@
 
 /** Tests for {@link ElasticsearchIO} version 7. */
 @ThreadLeakScope(ThreadLeakScope.Scope.NONE)
-// use cluster of 1 node that has data + master roles
-@ESIntegTestCase.ClusterScope(scope = SUITE, numDataNodes = 1, supportsDedicatedMasters = false)
-public class ElasticsearchIOTest extends ESIntegTestCase implements Serializable {
+public class ElasticsearchIOTest implements Serializable {

Review comment:
       Is it time to think about merging classes to avoid all the duplication perhaps?
   
   I know it's not created within this PR, but the similarities in the ES6 and ES7 tests look like they should be candidates to be a single class that takes a container tag to run against. I didn't check the ES5 one and I'm afraid I don't know the background as to why we have separate classes.
   
   We might consider taking that as a separate Jira.
   




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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org