Cleanup test profile and make it converge with prod profile #786

adutra · 2025-01-15T17:20:59Z

This PR cleans up the test profile and makes it converge with the prod profile, which is better as by default, tests are testing the real-life application.

If tests need to customize the configuration, this is now done on a per-test basis.

adutra · 2025-01-15T17:22:44Z

quarkus/service/src/main/resources/application.properties

@@ -73,7 +73,7 @@ quarkus.otel.sdk.disabled=false
 # quarkus.otel.traces.sampler.arg=1.0d

 polaris.realm-context.type=default
-polaris.realm-context.realms=realm1,realm2,realm3
+polaris.realm-context.realms=POLARIS


This change imho makes sense as POLARIS is definitely a better default value than the rather lame realm1,realm2,realm3. Tests that need to use a different realm name should override this property. Tests that generate random realm names need to change the realm resolver to test: polaris.realm-context.type=test.

adutra · 2025-01-15T17:23:28Z

quarkus/service/src/main/resources/application.properties

@@ -119,29 +119,5 @@ polaris.authentication.token-broker.max-token-generation=PT1H
 # polaris.storage.gcp.lifespan=PT1H

 %test.quarkus.log.file.enable=false
-%test.quarkus.log.category."org.apache.polaris".level=INFO
-%test.quarkus.log.category."org.apache.iceberg.rest".level=INFO
-%test.quarkus.log.category."org.apache.iceberg.rest.RESTSessionCatalog".level=ERROR


Removing these loggers for now, let's see if users complain about logs being too verbose.

adutra · 2025-01-15T17:59:43Z

quarkus/service/src/main/resources/application.properties

-%test.polaris.storage.gcp.lifespan=PT1H
+
+# Profile for integration tests
+%it.quarkus.http.limits.max-body-size=1000000


Unfortunately, when running integration tests, the configuration needs to be built into the application jar. But at least, the changes are confined in a specific profile that will be used only for that.

This change LGTM, but it also highlights my concerns with checking body size restrictions as part of our "normative" suite of integration tests (commented under #630). It is very specific to the server env. FWIW, even HTTP proxies may have their own (different) limits.

dimas-b · 2025-01-15T18:08:37Z

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

@@ -96,6 +96,12 @@
 /**
 * Import the full core Iceberg catalog tests by hitting the REST service via the RESTCatalog
 * client.
+ *
+ * @implSpec @implSpec This test expects the server to be configured with the following features
+ *     enabled:


nit: disabled?

dimas-b · 2025-01-15T18:11:50Z

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

@@ -93,6 +93,18 @@
 import org.junit.jupiter.api.TestInfo;
 import org.junit.jupiter.api.extension.ExtendWith;

+/**
+ * @implSpec This test expects the server to be configured with the following features enabled:


WDYT about putting these notes in a README under integration-test and mention the README in javadoc?

dimas-b · 2025-01-15T18:13:55Z

quarkus/service/src/main/resources/application.properties

+%it.polaris.features.defaults."ENFORCE_PRINCIPAL_CREDENTIAL_ROTATION_REQUIRED_CHECKING"=true
+%it.polaris.features.defaults."INITIALIZE_DEFAULT_CATALOG_FILEIO_FOR_it"=true
+%it.polaris.features.defaults."SKIP_CREDENTIAL_SUBSCOPING_INDIRECTION"=true
+%it.polaris.features.defaults."SUPPORTED_CATALOG_STORAGE_TYPES"=["FILE","S3","GCS","AZURE"]


Would it be possible to keep these settings in a java test profile? IIRC, application.properties is embedded in the final distribution jar, so it would be nice to avoid having test stuff there.

I can split in many files, if that's better, but the it file must stay under src/main/resources according to this doc:

While adding test-specific configuration properties using src/test/resources/application.properties (note there’s test, not main) is possible for unit tests, it’s not possible for integration tests.

And indeed I tested, it works only if the file is in main not when it's in intTest.

I mean using QuarkusTestProfile to define these properties... Does it not work with integration tests?

No, you can't use QuarkusTestProfile with @QuarkusIntegrationTest.

TBH I'm not a fan of splitting the files, I think having all the profiles in application.properties is actually simpler, wdyt?

fair enough! TIL about test profiles not working in integration tests 🤔

dimas-b · 2025-01-15T18:16:43Z

...e/src/test/java/org/apache/polaris/service/quarkus/it/QuarkusRestCatalogIntegrationTest.java

 import org.apache.polaris.service.it.test.PolarisRestCatalogIntegrationTest;

 @QuarkusTest
-public class QuarkusRestCatalogIntegrationTest extends PolarisRestCatalogIntegrationTest {}
+@TestProfile(QuarkusRestCatalogIntegrationTest.Profile.class)
+public class QuarkusRestCatalogIntegrationTest extends PolarisRestCatalogIntegrationTest {


Sorry, I'm a bit confused wrt #778 ... Was it not moved to QuarkusRestCatalogIT?

Tests were duplicated. The only one that was removed from test is QuarkusSparkIT.

but why duplicate them? why not just rely on the intTest suite?

Because it can be useful to also run those *IntegrationTest when running ./gradlew test – they usually run faster, because there is no need to build the artifact first.

In Maven, it's common to have the following idiom:

@QuarkusTest public class FooTest { // tests go here } @QuarkusIntegrationTest public class FooIT extends FooTest {}

(this doesn't work in Gradle though)

But if you prefer, I can remove the tests in test and only keep the ones in intTest.

Let's keep. I suppose it makes sense to test with in-memory store and intTest with EclipseLink + H2

dimas-b · 2025-01-15T20:21:07Z

build.gradle.kts

@@ -91,6 +91,8 @@ tasks.named<RatTask>("rat").configure {
  excludes.add("logs/**")
  excludes.add("service/common/src/**/banner.txt")
  excludes.add("quarkus/service/logs")
+  excludes.add("quarkus/server/logs")
+  excludes.add("quarkus/admin/logs")


WDYT about disabling log files in test tasks? Do people file test log files useful? As for me, I'd personally prefer STDOUT.

Well, it's about testing the "real thing". If the real thing produces log files, then let it be. At least you can inspect the log files manually to check e.g. if a change you made produced some expected output in the log files.

Do you think it's not worth it?

I'm fine deferring it til later. I guess my concern was mostly with log files ending up under the main source tree.

Noting for ITs: the approach from #785 runs the Quarkus application with the current directory being underneath the build/ directory.

This will improve a bit with #835.

adutra · 2025-01-16T17:01:02Z

quarkus/service/src/main/resources/application-it.properties

+# Note: this file MUST be placed in the src/main/resources directory, not src/intTest/resources!
+
+quarkus.http.limits.max-body-size=1000000
+quarkus.http.port=0


For posterity, it seems that with integration tests, it's this property that needs to be set to zero, and not quarkus.http.test-port.

Noting: The plugins from #785 do this automatically.

adutra · 2025-01-16T17:01:18Z

quarkus/service/src/main/resources/application-it.properties

+quarkus.http.limits.max-body-size=1000000
+quarkus.http.port=0
+
+quarkus.log.file.path=./build/logs/polaris.log


This property does not seem to take effect :-( will investigate later.

Fixed in #835.

adutra · 2025-01-16T17:09:27Z

quarkus/service/src/main/resources/application-it.properties

+
+# Need to allow a java security manager after Java 21, for Subject.getSubject to work
+# "getSubject is supported only if a security manager is allowed".
+quarkus.test.arg-line=-Djava.security.manager=allow


@snazy FYI there was a typo in this option, it's arg-line and not argLine. This PR fixes it.

Nice. I saw a log WARN about that too.

Weird though that it worked 🤷

eric-maynard · 2025-01-16T18:00:17Z

integration-tests/README.md

I would rather the code self-describe than try to keep a file like this up to date

Yeah good point. This will quickly go out of sync.

eric-maynard · 2025-01-16T18:00:48Z

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

@@ -93,6 +93,18 @@
 import org.junit.jupiter.api.TestInfo;
 import org.junit.jupiter.api.extension.ExtendWith;

+/**


Previously with dropwizard we could set overrides that were test-specific; is that not possible now?

Yes it still is.

eric-maynard · 2025-01-16T18:01:22Z

quarkus/service/src/main/resources/application-it.properties

+
+polaris.persistence.type=in-memory
+
+polaris.features.defaults."ALLOW_EXTERNAL_CATALOG_CREDENTIAL_VENDING"=false


Oh, I see, you set them here. Can we set these from within tests instead?

i.e. if a test relies on a certain feature flag being set let's just set it in that test rather than subtlety rely on the default.

This file is only for @QuarkusIntegrationTest annotated tests (these are in the intTest folder). These are "real" black box tests that run the actual final jar in a separate JVM process. Because of that, some tweaks are not possible for these tests, but at least, it is possible to provide a file like this that will be passed to the JVM process.

For @QuarkusTest annotated test (in the test folder), it is possible to define options that are global to all tests, and also possible to define options that are valid for just some tests, using QuarkusTestProfile.

i.e. if a test relies on a certain feature flag being set let's just set it in that test rather than subtlety rely on the default.

That's precisely the intent of this PR :-)

Sorry if I misunderstood, I was specifically looking at the comments like:

This test expects the server...

And thinking that the test shouldn't need that comment and should just always set those exact values

I think this comment belongs to another conversation? I'm confused now, what is it that you want me to remove? The javadocs, the README, everything?

eric-maynard · 2025-01-16T18:03:04Z

...kus/service/src/test/java/org/apache/polaris/service/quarkus/admin/PolarisAuthzTestBase.java

@@ -87,9 +87,9 @@ public static class Profile implements QuarkusTestProfile {
    @Override
    public Map<String, String> getConfigOverrides() {
      return Map.of(


IIUC, these need to map to entries in the Quarkus config file rather than the actual Polaris features?

Is there a way we can make that mapping programmatic rather than manual?

Yes this method allows test profiles to override properties define somewhere else. Unfortunately the mapping is not programmatic. I agree that this is not ideal, but that's how it is today.

But can we have it be programmatic? Is there a way to look up key:value pairs from the Quarkus config file?

No, they do not form a set like an enum or something like that. The amount of available configuration options depends on the extensions being used.

adutra · 2025-01-21T12:20:36Z

Rebased in order to adapt to the recently-introduced polaris-quarkus-defaults.

adutra · 2025-01-22T12:11:57Z

Is it OK to merge it today? 🙏

This PR cleans up the test profile and makes it converge with the prod profile, which is better as by default, tests are testing the real-life application. If tests need to customize the configuration, this is now done on a per-test basis with `QuarkusTestProfile`.

adutra · 2025-01-23T18:59:29Z

@dimas-b @eric-maynard
Rebased again, would it be possible to get another round of review please? Thanks 🙏

dimas-b

LGTM 👍

dimas-b · 2025-01-24T00:45:35Z

quarkus/defaults/src/main/resources/application.properties


 polaris.realm-context.type=default
-polaris.realm-context.realms=realm1,realm2,realm3
+polaris.realm-context.realms=POLARIS


nit: Was old default upper case?

Heh no: the default for prod was default-realm:

polaris/polaris-server.yml

Line 79 in aa240e9

defaultRealm: default-realm

But the default for tests was POLARIS (in uppercase):

polaris/dropwizard/service/src/test/resources/polaris-server-integrationtest.yml

Line 104 in aa240e9

defaultRealm: POLARIS

I thought POLARIS was more appropriate as a default than default-realm, but happy to change that back to default-realm if someone thinks otherwise.

adutra · 2025-01-24T09:29:45Z

There has been some confusion around regression tests lately (see #865) and I think this PR would help. So I'm going to merge it now. If there are leftovers, please speak up and I will fix in another PR. Thanks!

adutra requested review from ashvina, dimas-b, eric-maynard, jackye1995, jbonofre, vvcephei, collado-mike, snazy, RussellSpitzer, takidau, flyrain and ebyhr as code owners January 15, 2025 17:21

adutra commented Jan 15, 2025

View reviewed changes

adutra force-pushed the prune-test-profile branch from 42ecda1 to 11792d5 Compare January 15, 2025 17:58

adutra commented Jan 15, 2025

View reviewed changes

dimas-b reviewed Jan 15, 2025

View reviewed changes

adutra requested a review from dennishuo as a code owner January 16, 2025 09:04

adutra force-pushed the prune-test-profile branch 2 times, most recently from 9d73113 to 290b8d9 Compare January 16, 2025 13:57

dimas-b approved these changes Jan 16, 2025

View reviewed changes

adutra force-pushed the prune-test-profile branch from 290b8d9 to 6a6caa2 Compare January 16, 2025 15:53

adutra commented Jan 16, 2025

View reviewed changes

eric-maynard reviewed Jan 16, 2025

View reviewed changes

dimas-b approved these changes Jan 16, 2025

View reviewed changes

adutra force-pushed the prune-test-profile branch 4 times, most recently from a786be3 to d823e67 Compare January 21, 2025 12:19

adutra force-pushed the prune-test-profile branch from d823e67 to 3491e5f Compare January 22, 2025 12:11

This was referenced Jan 22, 2025

[WIP] Make dev profile compatible with tests and regtests #847

Closed

Reorder filters and let RealmIdResolver throw UnresolvableRealmException #848

Open

adutra force-pushed the prune-test-profile branch from 3491e5f to 82095ce Compare January 23, 2025 18:58

dimas-b approved these changes Jan 24, 2025

View reviewed changes

adutra mentioned this pull request Jan 24, 2025

Failed to run run_spark_sql.sh #865

Open

adutra merged commit ddb8025 into apache:main Jan 24, 2025
5 checks passed

adutra deleted the prune-test-profile branch January 24, 2025 09:30


		polaris.persistence.type=in-memory

		polaris.features.defaults."ALLOW_EXTERNAL_CATALOG_CREDENTIAL_VENDING"=false

Cleanup test profile and make it converge with prod profile #786

Cleanup test profile and make it converge with prod profile #786

Conversation

adutra commented Jan 15, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dimas-b Jan 15, 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

adutra Jan 15, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adutra Jan 16, 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

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

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 Jan 21, 2025 • edited Loading

adutra commented Jan 22, 2025

adutra commented Jan 23, 2025

dimas-b left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adutra Jan 24, 2025 • edited Loading

Choose a reason for hiding this comment

adutra commented Jan 24, 2025

dimas-b Jan 15, 2025 •

edited

Loading

adutra Jan 15, 2025 •

edited

Loading

adutra Jan 16, 2025 •

edited

Loading

adutra commented Jan 21, 2025 •

edited

Loading

adutra Jan 24, 2025 •

edited

Loading