Convert main integration tests into reusable blackbox 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:32:19Z

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

+      this.realm = realm;
+      this.server = server;
+
+      ObjectMapper mapper = new ObjectMapper();


Couldn't we create this client outside of the extension? In Quarkus, it would be better to inject the ObjectMapper that's used server-side, but it won't be available for injection here.

Oh sorry, this is for blackbox testing only. My bad.

Unresolving this conversation because it turns out the Server impl will need access to an ObjectMapper. We can't access the one created by Quarkus because this is a black box test. But we may be able to share the object mapper created here with the server impl. This is minor though, for now my Quarkus server impl is simply creating another ObjectMapper identical to this one.

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.

I like the proposal for remote tests to implement their own PolarisServerManager. Otherwise, the concern for one use case ends up cluttering the code for the other use case.

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.

The intention of the SnowmanExtension was simply to have a non-root user easily available so that all the tests didn't just run everything as root, thus missing issues that would be faced by non-root users. If snowman is just as easy to access without the extension, that's fine with me.

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.

I think I'm going to make a separate refactoring PR for tests currently using @PolarisFeatureConfig in order to push the config behaviour concerns to the unit test level, but keep blackbox test concentrating on user-visible functionality with default config.

#607 converts the tests that relied on config overrides to proper unit tests (those will be outside the scope of this PR if 607 is merged)

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

adutra · 2025-01-02T11:11:29Z

Do you think it would be valuable as a blackbox test?

Indeed I haven't thought about interference when the server is shared. In this case let's leave RateLimiterFilterTest and TimedApplicationEventListenerTest out of the refactoring scope.

dimas-b · 2025-01-03T22:30:31Z

Another related unit test refactoring: #612

dimas-b · 2025-01-03T23:54:34Z

@adutra : I've rebased and resolved conflicts. Application log is now checked in DropwizardServerManager.

Config overrides are gone. I hope the backbox tests are reusable in Quarkus now.

integration-tests/src/main/java/org/apache/polaris/service/it/ext/PolarisServerManager.java

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

adutra

Making good progress! With the latest changes the test at least are executed with Quarkus. I'm seeing a few failures though.

...ests/src/main/java/org/apache/polaris/service/it/test/PolarisApplicationIntegrationTest.java

...rc/main/java/org/apache/polaris/service/it/test/PolarisManagementServiceIntegrationTest.java

integration-tests/build.gradle.kts

dropwizard/service/build.gradle.kts

...rc/main/java/org/apache/polaris/service/it/test/PolarisManagementServiceIntegrationTest.java

dimas-b · 2025-01-10T17:20:51Z

@andrew4699 : I propose we merge this PR and address your points in follow-up PRs if you do not mind :) Would this be ok?

andrew4699 · 2025-01-10T18:40:39Z

It's great that we're refactoring this and I think it's going in a good direction, my main concern is that this is a breaking change. Today, you can provide your own HTTP client. If this change is merged as-is, you won't be able to.

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

Use Awaitility to make sure the expected response code is received within a reasonable timeout (1 min).

dimas-b · 2025-01-10T23:17:22Z

@andrew4699 : does the commit entitled review: customizable client help with your use cases?

collado-mike

Please ignore outdated comments - there was refactoring after my initial review, which I forgot to publish :)

collado-mike · 2025-01-06T17:47:44Z

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

+                            "grant_type",
+                            "client_credentials",
+                            "scope",
+                            "PRINCIPAL_ROLE:ALL",


Can we make scope an optional argument (default to PRINCIPAL_ROLE:ALL) so tests can verify sub-scoped tokens?

collado-mike · 2025-01-06T18:00:05Z

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

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


The intention of the SnowmanExtension was simply to have a non-root user easily available so that all the tests didn't just run everything as root, thus missing issues that would be faced by non-root users. If snowman is just as easy to access without the extension, that's fine with me.

collado-mike · 2025-01-11T20:32:06Z

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

+
+import java.net.URI;
+
+public interface Server extends AutoCloseable {


Can we add javadocs to public classes an interfaces? It's really helpful to see, at a glance, what classes purpose some of these classes are intended to serve.

collado-mike · 2025-01-11T20:42:08Z

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

+          config.entrySet().stream()
+              .map((e) -> ConfigOverride.config(e.getKey(), e.getValue()))
+              .toList()
+              .toArray(new ConfigOverride[0]);


nit: I tend to prefer Stream.toArray(ConfigOverride[]::new) for this purpose

collado-mike · 2025-01-11T20:44:16Z

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

+
+    @Override
+    public String realmId() {
+      return TEST_REALM;


Most of the tests today create a distinct realm per test so that there's no overlap for roles or catalogs, etc. It also means that a test that fails to clean up doesn't leave clutter that effects other tests.

collado-mike · 2025-01-11T20:45:15Z

.../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()));


I like the proposal for remote tests to implement their own PolarisServerManager. Otherwise, the concern for one use case ends up cluttering the code for the other use case.

collado-mike · 2025-01-11T20:58:21Z

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

+    // Create a new CatalogRole that has CATALOG_MANAGE_CONTENT and CATALOG_MANAGE_ACCESS
+    String catalogRoleName = "custom-admin";
+    createCatalogRole(catalog.getName(), catalogRoleName);


Why create a custom admin role? The service_admin already has the default catalog admin role that was created when the Catalog was created.

collado-mike · 2025-01-11T21:04:24Z

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

+        new ClientCredentials(
+            principal.getCredentials().getClientId(),
+            principal.getCredentials().getClientSecret(),
+            "dummy-principal"));


use the principal name?

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 2 times, most recently from 8b7325e to 90ee9d0 Compare December 30, 2024 19:44

adutra reviewed Dec 31, 2024

View reviewed changes

dimas-b force-pushed the sharable-tests branch from 90ee9d0 to 854885d Compare January 3, 2025 23:52

adutra changed the title ~~Convert main integration tests into reusable backbox tests~~ Convert main integration tests into reusable blackbox tests Jan 4, 2025

adutra reviewed Jan 4, 2025

View reviewed changes

integration-tests/src/main/java/org/apache/polaris/service/it/ext/PolarisServerManager.java Outdated Show resolved Hide resolved

adutra reviewed Jan 4, 2025

View reviewed changes

...n-tests/src/main/java/org/apache/polaris/service/it/ext/PolarisIntegrationTestExtension.java Outdated Show resolved Hide resolved

dimas-b force-pushed the sharable-tests branch 3 times, most recently from 920a2fa to bb4b6fb Compare January 6, 2025 19:09

dimas-b force-pushed the sharable-tests branch from 03d3890 to 3ed1873 Compare January 7, 2025 22:52

adutra reviewed Jan 8, 2025

View reviewed changes

...ests/src/main/java/org/apache/polaris/service/it/test/PolarisApplicationIntegrationTest.java Outdated Show resolved Hide resolved

...ests/src/main/java/org/apache/polaris/service/it/test/PolarisApplicationIntegrationTest.java Outdated Show resolved Hide resolved

adutra reviewed Jan 8, 2025

View reviewed changes

...rc/main/java/org/apache/polaris/service/it/test/PolarisManagementServiceIntegrationTest.java Outdated Show resolved Hide resolved

adutra reviewed Jan 8, 2025

View reviewed changes

integration-tests/build.gradle.kts Show resolved Hide resolved

dimas-b force-pushed the sharable-tests branch 2 times, most recently from 07b6a08 to d3bc035 Compare January 9, 2025 00:26

dimas-b marked this pull request as draft January 9, 2025 00:27

dimas-b force-pushed the sharable-tests branch 2 times, most recently from e50c564 to f6d2a8c Compare January 9, 2025 14:35

dimas-b marked this pull request as ready for review January 9, 2025 14:36

adutra mentioned this pull request Jan 9, 2025

Remove testRequestHeaderTooLarge() #630

Closed

dimas-b force-pushed the sharable-tests branch from f6d2a8c to 354f4f3 Compare January 10, 2025 00:23

adutra reviewed Jan 10, 2025

View reviewed changes

dropwizard/service/build.gradle.kts Outdated Show resolved Hide resolved

adutra reviewed Jan 10, 2025

View reviewed changes

...rc/main/java/org/apache/polaris/service/it/test/PolarisManagementServiceIntegrationTest.java Show resolved Hide resolved

dimas-b force-pushed the sharable-tests branch 2 times, most recently from ad84b95 to 09dcd88 Compare January 10, 2025 15:43

adutra approved these changes Jan 10, 2025

View reviewed changes

dimas-b force-pushed the sharable-tests branch from 09dcd88 to 198def0 Compare January 10, 2025 17:47

adutra approved these changes Jan 10, 2025

View reviewed changes

dimas-b added 5 commits January 10, 2025 18:16

review: exclude jul-to-slf4j

42774e6

review: fix POLARIS_BOOTSTRAP_CREDENTIALS

1745aa2

Make tests for 413/431 responses more robust

61dbd62

Use Awaitility to make sure the expected response code is received within a reasonable timeout (1 min).

review: customizable client

7c44ea3

dimas-b force-pushed the sharable-tests branch from 198def0 to 7c44ea3 Compare January 10, 2025 23:17

clean up entities created by tests

312091c

collado-mike approved these changes Jan 11, 2025

View reviewed changes


		AuthToken adminToken();

		ClientCredentials adminCredentials();


		import java.net.URI;

		public interface Server extends AutoCloseable {

Convert main integration tests into reusable blackbox tests #590

Are you sure you want to change the base?

Convert main integration tests into reusable blackbox tests #590

Conversation

dimas-b commented Dec 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adutra Jan 4, 2025 • edited Loading

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

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

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

adutra commented Jan 2, 2025

dimas-b commented Jan 3, 2025

dimas-b commented Jan 3, 2025

adutra left a comment

Choose a reason for hiding this comment

dimas-b commented Jan 10, 2025

andrew4699 commented Jan 10, 2025

dimas-b commented Jan 10, 2025

collado-mike left a 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

Choose a reason for hiding this comment

adutra Jan 4, 2025 •

edited

Loading