JDBC: Automate schema version test discovery

[venkateshwaracholan]

Fixes #2675

Summary

Run JDBC metastore integration tests against every H2 schema version discovered on the test classpath without maintaining a separate test class per schema version.

Changes

• Add H2SchemaVersions to discover available h2/schema-vN.sql resources from the test classpath.
• Add AtomicMetastoreManagerWithJdbcBasePersistenceImplSchemaTest using JUnit @TestFactory, DynamicContainer, and DynamicTest to execute the JDBC metastore test suite against each discovered schema version.
• Remove the manually maintained schema-specific test subclasses (V0 through V4).

Validation

Ran:

./gradlew :polaris-relational-jdbc:test \
  --tests "org.apache.polaris.persistence.relational.jdbc.AtomicMetastoreManagerWithJdbcBasePersistenceImplV*SchemaTest"

Checklist

• 🛡️ Don't disclose security issues! (contact security@apache.org)
• 🔗 Clearly explained why the changes are needed, or linked related issues: Fixes #
• 🧪 Added/updated tests with good coverage, or manually tested (and explained how)
• 💡 Added comments for complex logic
• 🧾 Updated CHANGELOG.md (if needed)
• 📚 Updated documentation in site/content/in-dev/unreleased (if needed)

[dimas-b]

Thanks for your contribution, @venkateshwaracholan !

[dimas-b]

This is an interesting idea... but it feels a bit like an overkill to me 😅

Could we not do the same with JUnit test factories and without on-the-fly code generation?

[nandorKollar]

+1, using TestFactory and DynamicTests sounds like a better approach.

[venkateshwaracholan]

Thanks @dimas-b @nandorKollar, good call. I dropped the Gradle codegen and switched to @testfactory / dynamic tests instead.

[nandorKollar]

nit: I'd keep the the String.format which is used in the actual schema executor code in AtomicMetastoreManagerWithJdbcBasePersistenceImplTest base class:
String.format("%s/schema-v%d.sql", DatabaseType.H2.getDisplayName(), version)

[nandorKollar]

nit: this will skip schema files, when the numbering is not continuous, like for example: schema-0, schema-1, schema-3. Not sure that this we should prepare for this scenario too, or should we?

[dimas-b]

+1 to @nandorKollar 's comment and it's not a "nit" IMHO :)

I'd check every version number until DatabaseType.getLatestSchemaVersion() and fail if any version is missing the corresponding schema file resource.

[nandorKollar]

Although the current approach works, I don't really like the reflective invocation and the fact, that we need to manually implement BeforeEach. I think we can simplify that part using ParmetrizedTestClass. Instead of dynamics tests, how about something like this:

@ParameterizedClass
@MethodSource("schemaVersions")
public class AtomicMetastoreManagerWithJdbcBasePersistenceImplSchemaTest extends AtomicMetastoreManagerWithJdbcBasePersistenceImplTest {
    @Parameter
    int schemaVersion;

    static SortedSet<Integer> schemaVersions() {
        ClassLoader classLoader = H2SchemaVersions.class.getClassLoader();
        SortedSet<Integer> versions = new TreeSet<>();
        for (int version = 0; ; version++) {
            String resource = String.format("h2/schema-v%d.sql", version);
            if (classLoader.getResource(resource) == null) {
                break;
            }
            versions.add(version);
        }
        if (versions.isEmpty()) {
            throw new IllegalStateException("No H2 schema files found on classpath");
        }
        return versions;
    }

    @Override
    public int schemaVersion() {
        return schemaVersion;
    }
}

Would this work? Does it look better than dynamic tests?

[dimas-b]

for later: I'd rather we did this testing with PostgreSQL, which is the "main" database 🤔 ... H2 is not expected to be used in "prod", so full schema coverage on H2 does not seem to add assurance for end users 🤔

[nandorKollar]

Should we open a followup issue for that? I think Postgres already has a testcontainer, it seems that we already use it in Polaris integration tests.

[dimas-b]

Yes, let's keep this PR as "pure" refactoring and change from H2 to Posgres later.

[dimas-b]

This is flaky, IMHO, because we provide only a very narrow sub-set of functionality that @Test method may expect from the JUnit framework... but overall, it's probably ok for now.

[nandorKollar]

@dimas-b that's why I thought that maybe using ParameterizedClass could be a better choice here.

[dimas-b]

+1 to try ParameterizedClass... TIL that it came with JUnit 6 😅

[nandorKollar]

Actually it seems that it is there since JUnit 5.13: https://docs.junit.org/5.13.0/api/org.junit.jupiter.params/org/junit/jupiter/params/ParameterizedClass.html!

[dimas-b]

+1 to @nandorKollar 's comment and it's not a "nit" IMHO :)

I'd check every version number until DatabaseType.getLatestSchemaVersion() and fail if any version is missing the corresponding schema file resource.

[venkateshwaracholan]

Thanks @dimas-b and @nandorKollar for the feedback.

Switched from @TestFactory/reflection to @ParameterizedClass, so the inherited tests run with the normal JUnit lifecycle.
Updated schema discovery to validate every version from 0 through DatabaseType.H2.getLatestSchemaVersion() and fail if any expected schema resource is missing.
Reused the same schema resource path construction as the base test.

[nandorKollar]

nit: this looks good, but what if we make it simpler. We can simply use DatabaseType.getLatestSchemaVersion() to create the parameters from 0..DatabaseType.getLatestSchemaVersion()-1, and parametrise the test with that number. The test itself will try to load the file, and if let's say v2 will be missing, then it will eventually fail, as we expect. Maybe the failure report isn't that elegant as with explicitly checking for the continuous versioning. Basically we don't even need this class, but

  static Stream<Integer> schemaVersions() {
    return IntStream.rangeClosed(0, DatabaseType.getLatestSchemaVersion());
  }

What do you think?