repository-s3 yamlRestTests to new cluster test framework #102353

breskeby · 2023-11-18T22:24:13Z

This refactors the repository-s3 yaml rest test suite to leverage our new testing framework.

As part of that migration s3 fixture has been reworked to ship with a Junit based test fixture that allows using different s3 fixtures directly without the need of any docker virtualisation or external processes.

As part of this refactoring a MinioTestContainer was introduced that leverages TestContainer api to build a docker based test fixture without the need to rely on specific docker or a docker compose file .

In general we want to move away from relying on the docker-compose-gradle plugin as its setup is overly complex to integrate with our testfixtures handling, has proven to be error prone and flaky and it makes compliance with gradle configuration cache compatibility (see ##57918)

Rework fixture-s3 fixture
Rework minio fixture
Support lazy evaluated system properties for local cluster definitions in our new test cluster framework.

From here we can move forward and replace other docker compose based usages of our s3-fixture and mini-fixture in our build before cleaning up s3-fixture and mini-fixture completely

Some comparison build scans:

old: https://gradle-enterprise.elastic.co/s/xz5xu3oqqxhdc
new: https://gradle-enterprise.elastic.co/s/soa65wjpm5rr2
new with configuration cache enabled: https://gradle-enterprise.elastic.co/s/4udekkgo3vfxq/timeline

elasticsearchmachine · 2023-11-18T22:24:43Z

Pinging @elastic/es-delivery (Team:Delivery)

- Rework docker-compose fixture usages to leverage testcontainers - Support lazy evaluated system properties for local cluster definitions

simplify fixture usage by relying on plain idiomatic gradle apis and best practices

Do not rely on docker compose for MinioFixture

test/fixtures/s3-fixture/src/main/java/fixture/s3/junit/S3HttpFixtureRule.java

...c/yamlRestTest/java/org/elasticsearch/repositories/s3/RepositoryS3ClientYamlTestSuiteIT.java

.../internalClusterTest/java/org/elasticsearch/repositories/s3/S3RepositoryThirdPartyTests.java

...c/yamlRestTest/java/org/elasticsearch/repositories/s3/RepositoryS3ClientYamlTestSuiteIT.java

test/fixtures/minio-fixture/build.gradle

test/fixtures/s3-fixture/src/main/java/fixture/s3/junit/S3HttpFixtureRule.java

...st-clusters/src/main/java/org/elasticsearch/test/cluster/local/AbstractLocalSpecBuilder.java

...o-fixture/src/main/java/org/elasticsearch/test/fixtures/minio/MinioFixtureTestContainer.java

...c/yamlRestTest/java/org/elasticsearch/repositories/s3/RepositoryS3ClientYamlTestSuiteIT.java

breskeby · 2023-11-21T15:13:09Z

@mark-vieira I rewrote this to split up the fixtures and simplify the build script for repository-s3 (reducing test tasks). This feels pretty nice now. Only the S3 fixtures still look a bit messy as we for now use them as TestRule and Standalone (in other s3 projects). Once we port all others we can clean that up even more imo.

...c/yamlRestTest/java/org/elasticsearch/repositories/s3/RepositoryS3ClientYamlTestSuiteIT.java

mark-vieira · 2023-11-21T19:36:46Z

modules/repository-s3/build.gradle

  systemProperty 'test.s3.account', s3PermanentAccessKey
  systemProperty 'test.s3.key', s3PermanentSecretKey
  systemProperty 'test.s3.bucket', s3PermanentBucket
  nonInputProperties.systemProperty 'test.s3.base', s3PermanentBasePath + "_third_party_tests_" + BuildParams.testSeed
-  if (useFixture) {


So I think one side-effect of this is we no longer have the ability to run the third party tests against a fixture. We can only run these with a real external service. I think we should wire the test case up to still support using fixtures. This is the case in the other repository modules and is helpful for local development.

I think this was never wired against the s3 fixtures. it just uses the minio fixture. Originally its configuration was not within that if statement: https://github.com/elastic/elasticsearch/blob/main/modules/repository-s3/build.gradle#L360C1-L375

Yes, because it probably only needed the minio fixture to run. Minio is just a mock S3 implementation. You can see below I can run these tests locally. I'm pretty sure that would fail with this PR.

https://gradle-enterprise.elastic.co/s/dfnnlb42ib4wc/tests/overview

mark-vieira · 2023-11-21T19:45:57Z

...stTest/java/org/elasticsearch/repositories/s3/AbstractRepositoryS3ClientYamlTestSuiteIT.java

+import org.elasticsearch.test.rest.yaml.ESClientYamlSuiteTestCase;
+
+public abstract class AbstractRepositoryS3ClientYamlTestSuiteIT extends ESClientYamlSuiteTestCase {
+    static final boolean USE_FIXTURE = Booleans.parseBoolean(System.getProperty("tests.use.fixture", "true"));


I actually don't think we need this. We never run any tests in yamlRestTest without a fixture. This should only be applicable to S3RepositoryThirdPartyTests which are designed to run against a real endpoint.

I kept it there to ensure all test behave like they did before. that is, if use fixture in the build script is declared, those tests are never executed.

Yeah, but I don't think that makes sense for these tests. These tests never talk to a real external service. That's what the third party tests are for. I think this should exist there and not here.

I think there's something fundamentally wrong in how these tests are designed in the first place already before this PR.
With this change they never run against third party really. only against the minio fixture. Actually I think the yamlRestTest task should run against thirdparty. I create a separate ticket to address this as this has been all wrong before this

...amlRestTest/java/org/elasticsearch/repositories/s3/RepositoryS3StsClientYamlTestSuiteIT.java

build-tools-internal/version.properties

mark-vieira · 2023-11-21T19:55:28Z

I left a few more minor comments but otherwise this looks great.

Once we port all others we can clean that up even more imo.

We should prioritize this work.

breskeby · 2023-11-22T16:09:35Z

I now understand what's going on here I think and what caused the confusion. The s3thirdpartytests are setup so they can run locally and against remote cluster. The build script setup has been done in a way that in theory you could a subset of yaml tests also against a remote service. I cleaned that up as you suggested in a follow up PR: #102454

One question is if we want to support this scenario for the yaml tests or not. Seems it worked at one point but those tests haven't been setup to run remotely at all. If we wanna ditch that support and think testing against our fixtures is enough for the yaml test suite, then we can cleanup that build script even more I think.

mark-vieira · 2023-11-30T01:09:01Z

If we wanna ditch that support and think testing against our fixtures is enough for the yaml test suite, then we can cleanup that build script even more I think.

We should reach out to @elastic/es-distributed on this. It's not clear to me that we ever ran those tests against a remote service. AFAIK we only ran the explicit "third party" test suite that way.

breskeby self-assigned this Nov 18, 2023

breskeby added >enhancement :Delivery/Build Build or test infrastructure Team:Delivery Meta label for Delivery team labels Nov 18, 2023

elasticsearchmachine added the v8.12.0 label Nov 18, 2023

Port repository-s3 to new cluster test framework

cd9eeec

- Rework docker-compose fixture usages to leverage testcontainers - Support lazy evaluated system properties for local cluster definitions

breskeby force-pushed the port-repository-s3-to-newtestframework branch from 4484857 to cd9eeec Compare November 18, 2023 22:26

breskeby changed the title ~~Port repository-s3 to new cluster test framework~~ WIP - Port repository-s3 yaml-rest-test to new cluster test framework Nov 18, 2023

This was referenced Nov 18, 2023

Elasticsearch build and Gradle plugins should support Gradle configuration cache #57918

Open

Make TestFixtures Plugin configuration cache compatible #102322

Closed

breskeby added 19 commits November 19, 2023 00:39

Make it work with configuration cache enabled

5f060cd

tweak EmptyDirTask as up-to-date

794020a

Introduce FixtureS3TestContainer

7b90bb6

Fix spotlight and simplify things

c6a5c12

Introduce FixtureApplication and FixtureApplicationConsumer plugin

bd2bc4b

simplify fixture usage by relying on plain idiomatic gradle apis and best practices

spotless

f2527c6

Handle useFixture == false

0ff8092

Handle registered fixture services dynamically

83f8329

Rework minio fixture usage

a03e2b3

Use local docker

c8e06cb

Use relative path for test fixture distro path

8a004ea

Port to plain s3 to plain java services

275de1d

Do not use local docker

2ee142e

Use plain java for fixture-s3 usage in repository-s3

bb6766f

Add debug output

4a2b53e

Cleanup s3-fixture project

29c84d5

Debug docker issue on ci

7ed3a2a

More simplifications

4530317

Do not rely on docker compose for MinioFixture

Fix spotless

4bce0e3

breskeby added 6 commits November 20, 2023 19:24

More clenaup

4baed3e

Some more cleanup

36127b7

Try fixing old s3 fixture usage

bb6bbfe

More cleanup

7b13078

Cleanup dependecnies

a350573

Some cleanup of dependencies

705c1a9

breskeby changed the title ~~repository-s3 yaml-rest-test to new cluster test framework~~ repository-s3 yamlRestRest to new cluster test framework Nov 20, 2023

breskeby changed the title ~~repository-s3 yamlRestRest to new cluster test framework~~ repository-s3 yamlRestTests to new cluster test framework Nov 20, 2023

breskeby requested a review from mark-vieira November 20, 2023 20:17

Failed test reporter should be on the classpath

b3e7d56

mark-vieira requested changes Nov 20, 2023

View reviewed changes

breskeby added 3 commits November 21, 2023 10:13

Need to disable security manager

baca5d2

Rework repository-s3 yaml tests run in one gradle task

c66c98e

Cleanup minio container

0f68251

breskeby requested a review from mark-vieira November 21, 2023 15:13

breskeby added 2 commits November 21, 2023 16:19

Minor reformatting / cleanup

8d219fc

Fix typo

2de621a

breskeby requested a review from rjernst November 21, 2023 17:17

mark-vieira approved these changes Nov 21, 2023

View reviewed changes

Inline cluster setup

a82bce2

breskeby added auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) and removed auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) labels Nov 21, 2023

Apply review feedback

f12f5c8

breskeby merged commit c11315d into elastic:main Nov 22, 2023
14 checks passed

breskeby deleted the port-repository-s3-to-newtestframework branch November 22, 2023 09:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

repository-s3 yamlRestTests to new cluster test framework #102353

repository-s3 yamlRestTests to new cluster test framework #102353

breskeby commented Nov 18, 2023 •

edited

Loading

elasticsearchmachine commented Nov 18, 2023

breskeby commented Nov 21, 2023

mark-vieira Nov 21, 2023

breskeby Nov 21, 2023

mark-vieira Nov 21, 2023

mark-vieira Nov 21, 2023

breskeby Nov 21, 2023

mark-vieira Nov 21, 2023

breskeby Nov 22, 2023

mark-vieira commented Nov 21, 2023

breskeby commented Nov 22, 2023

mark-vieira commented Nov 30, 2023

repository-s3 yamlRestTests to new cluster test framework #102353

repository-s3 yamlRestTests to new cluster test framework #102353

Conversation

breskeby commented Nov 18, 2023 • edited Loading

elasticsearchmachine commented Nov 18, 2023

breskeby commented Nov 21, 2023

mark-vieira Nov 21, 2023

Choose a reason for hiding this comment

breskeby Nov 21, 2023

Choose a reason for hiding this comment

mark-vieira Nov 21, 2023

Choose a reason for hiding this comment

mark-vieira Nov 21, 2023

Choose a reason for hiding this comment

breskeby Nov 21, 2023

Choose a reason for hiding this comment

mark-vieira Nov 21, 2023

Choose a reason for hiding this comment

breskeby Nov 22, 2023

Choose a reason for hiding this comment

mark-vieira commented Nov 21, 2023

breskeby commented Nov 22, 2023

mark-vieira commented Nov 30, 2023

breskeby commented Nov 18, 2023 •

edited

Loading