Convert main integration tests into reusable backbox tests #590

dimas-b · 2024-12-26T02:19:42Z

Introduce new module: integration-tests to contain the test code for backbox integration tests
Refactors existing main integration tests to avoid relying on Dropwizard internals.
Add new test extension for managing the test environment with custom server managers discovered via ServiceLoader
Use Dropwizard test extentions for the reference implementation of PolarisServerManager. Custom server builds can provide their own environment-specific implementations.
Add helper classes for REST API (Management and Catalog)
Run reusable tests by extending them in the DW module. Using test suites is also possible, but it only uses one gradle test worker, so test parallelism is reduced. At the same time running tests in parallel withing the same JVM using JUnit5 threads currently leads to errors.

This PR is meant to be the first step towards #524. Subsequent PRs can certainly enhance the testing framework more.

gradle/libs.versions.toml

...n-tests/src/main/java/org/apache/polaris/service/it/ext/PolarisIntegrationTestExtension.java

adutra · 2024-12-26T10:47:37Z

.../service/src/test/java/org/apache/polaris/service/dropwizard/it/DropwizardServerManager.java

+
+    @Override
+    public URI baseUri() {
+      return URI.create(String.format("http://localhost:%d", ext.getLocalPort()));


The old TestEnv extension could modify this to hit a remote Polaris server afaict.

My thinking was that remote cases would have their own Server Manager impl.

The old TestEnvironmentExtension is too intertwined with existing whitebox DW extensions... I though we'd have a totally new interface and gradually migrate TestEnvironmentExtension use cases to it.

To clarify: if a remote server is available outside the JUnit5 lifecycle, one can simply make an Server Manager impl. that returned a pre-configured URI and Realm, and then use a JUnit5 "suite" to execute tests from the new test module.

adutra · 2024-12-26T10:48:18Z

.../service/src/test/java/org/apache/polaris/service/dropwizard/it/DropwizardServerManager.java

+
+    @Override
+    public ClientCredentials adminCredentials() {
+      return new ClientCredentials("test-admin", "test-secret");


The old TestEnv extension used to append a "test ID" to the generated credentials afaict.

I'm not sure that the purpose of that append was, TBH 😅 I believe having one admin user per realm (as in this case) is sufficient.

Currently, there's only one realm for Server Manager instance, so other implementations can chose to use unique admin user names, if they prefer.

adutra · 2024-12-26T10:49:09Z

integration-tests/src/main/java/org/apache/polaris/service/it/env/Server.java

+
+  AuthToken adminToken();
+
+  ClientCredentials adminCredentials();


Where has the Snowman user gone?

Into the Nothing. The Server Manager provides "root" (admin) credentials. Tests create extra principals when they need to.

I am fine with that, although my impression was that the snowman user was introduced precisely to avoid using the root user for all admin tasks. Let's wait and see what @andrew4699 says.

I moved the work done by the SnowmanCredentialsExtension into specific test classes... hopefully maintaining the same functionality as before (the "snowman" principals used to be created using "admin" tokens too). Those can be found by searching for "snowman" in Principal names in the new test code. I believe explicit principal manipulation by test code is easier to follow and maintain than if it is done by a test extension.

The principal and role manipulation logic moved to the methods in the new ManagementApi class.

* Introduce new module: `integration-tests` to contain the test code for backbox integration tests * Refactors existing main integration tests to avoid relying on Dropwizard internals. * Add new test extension for managing the test environment with custom server managers discovered via `ServiceLoader` * Use Dropwizard test extentions for the reference implementation of `PolarisServerManager`. Custom server builds can provide their own environment-specific implementations. * Add helper classes for REST API (Management and Catalog) * Run reusable tests by extending them in the DW module. Using test suites is also possible, but it only uses one gradle test worker, so test parallelism is reduced. At the same time running tests in parallel withing the same JVM using JUnit5 threads currently leads to errors.

adutra · 2024-12-31T15:23:27Z

I noticed that 3 other tests use the Dropwizard extension and could be considered as "integration" tests:

FileIOIntegrationTest
RateLimiterFilterTest
TimedApplicationEventListenerTest

We could create "black-box" versions of those while still keeping the "white-box" ones around, wdyt?

adutra · 2024-12-31T15:38:18Z

...n-tests/src/main/java/org/apache/polaris/service/it/ext/PolarisIntegrationTestExtension.java

+                  .stream()
+                  .collect(
+                      Collectors.toMap(PolarisFeatureConfig::name, PolarisFeatureConfig::value));
+          return new Env(manager.realm(), manager.startServer(featureConfig));


I'm struggling to adapt this to Quarkus. In a Quarkus test (integration or not), the application will be running already when the extension is executed, and it's not possible to change the feature configs at that moment. The only way to change the application config is through a QuarkusTestProfile afaik. I don't think we can use a QuarkusTestResourceLifecycleManager to launch the application itself on demand; that's intended for dependent resources like databases etc. I think the @PolarisFeatureConfig annotation would eventually go away completely with Quarkus.

Let me see if we this can be refactored to fit Quarkus better

On a closer look at the test that use @PolarisFeatureConfig I tend to think that those tests are an overkill to have at the integration test level. The test variations merely check how configuration keys are resolved between catalog properties, server config and default values. I believe that part can be done by a unit test.

I'll try and refactor the blackbox tests to rely only on default config settings.

dimas-b · 2024-12-31T15:53:35Z

FileIOIntegrationTest is not really a blackbox test. It tweaks the internals of CDI in DW. I'm not even sure why it needs a DropwizardAppExtension. I think it can be converted to a proper unit test.

RateLimiterFilterTest could indeed be converted to a blackbox test. I did not do this initially because I thought that the test would become non-deterministic in the presence of other tests if the server is shared. Do you think it would be valuable as a blackbox test?

TimedApplicationEventListenerTest makes assertions that assume exclusive access to the server, so I'm not sure it would be valuable as a blackbox text when the server is shared.

dimas-b · 2024-12-31T23:22:27Z

FileIOIntegrationTest refactoring: #598

dimas-b requested review from jbonofre, ashvina, RussellSpitzer, snazy, vvcephei, takidau, jackye1995, flyrain, eric-maynard, collado-mike, ebyhr and adutra as code owners December 26, 2024 02:19

dimas-b mentioned this pull request Dec 26, 2024

[FEATURE REQUEST] Reusable integration tests #524

Open

adutra reviewed Dec 26, 2024

View reviewed changes

dimas-b force-pushed the sharable-tests branch from b6a6012 to 8b7325e Compare December 27, 2024 20:32

dimas-b added 3 commits December 30, 2024 14:44

fix CI: add fake GCP creds to polaris-server-integrationtest.yml

6d97635

review: use jakarta-ws-rs-api

90ee9d0

dimas-b force-pushed the sharable-tests branch from 8b7325e to 90ee9d0 Compare December 30, 2024 19:44

adutra reviewed Dec 31, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Convert main integration tests into reusable backbox tests #590

Convert main integration tests into reusable backbox tests #590

dimas-b commented Dec 26, 2024

adutra Dec 26, 2024

dimas-b Dec 27, 2024

dimas-b Dec 27, 2024

adutra Dec 26, 2024

dimas-b Dec 27, 2024

adutra Dec 26, 2024

dimas-b Dec 27, 2024

adutra Dec 27, 2024

dimas-b Dec 30, 2024

adutra commented Dec 31, 2024

adutra Dec 31, 2024

dimas-b Dec 31, 2024

dimas-b Dec 31, 2024

dimas-b commented Dec 31, 2024

dimas-b commented Dec 31, 2024


		AuthToken adminToken();

		ClientCredentials adminCredentials();

Convert main integration tests into reusable backbox tests #590

Are you sure you want to change the base?

Convert main integration tests into reusable backbox tests #590

Conversation

dimas-b commented Dec 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adutra commented Dec 31, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dimas-b commented Dec 31, 2024

dimas-b commented Dec 31, 2024