jerryshao commented on code in PR #13517: URL: https://github.com/apache/gravitino/pull/13517#discussion_r4132699935
########## core/src/test/java/org/apache/gravitino/storage/relational/CoreTestLaneOf.java: ########## @@ -0,0 +1,93 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.gravitino.storage.relational; + +import java.util.ArrayList; +import java.util.List; +import java.util.Set; +import java.util.stream.Collectors; +import org.junit.jupiter.api.Tag; +import org.junit.platform.commons.support.AnnotationSupport; + +/** + * A local, discovery-only check: prints which core database test lane(s) a class runs in, from its + * tags, without running anything. Reads a class's tags with the same lookup the Jupiter engine uses + * at discovery time (see {@link CoreBackend}), so the answer matches what {@code + * core/build.gradle.kts}'s lane tasks would actually do. + * + * <p>Run with {@code ./gradlew :core:coreTestLaneOf -PclassName=<fully.qualified.ClassName>}. + */ +public final class CoreTestLaneOf { + + private CoreTestLaneOf() {} + + /** + * Entry point. + * + * @param args exactly one fully-qualified class name to inspect + */ + public static void main(String[] args) { + if (args.length != 1) { + System.err.println("Usage: CoreTestLaneOf <fully.qualified.ClassName>"); + System.exit(1); + return; + } + + Class<?> testClass; + try { + testClass = Class.forName(args[0]); + } catch (ClassNotFoundException e) { + System.err.println("Class not found on the test classpath: " + args[0]); + System.exit(1); + return; + } + + Set<String> tags = + AnnotationSupport.findRepeatableAnnotations(testClass, Tag.class).stream() + .map(Tag::value) + .collect(Collectors.toSet()); + + List<String> lanes = new ArrayList<>(); + if (tags.contains(CoreBackend.H2_TAG)) { + lanes.add("coreH2Test"); + } + if (tags.contains(CoreBackend.MYSQL_TAG)) { + lanes.add("coreMySQLTest"); + } + if (tags.contains(CoreBackend.POSTGRESQL_TAG)) { + lanes.add("corePostgreSQLTest"); + } + + System.out.println(args[0] + " tags: " + tags); + if (!lanes.isEmpty()) { + System.out.println(args[0] + " runs in: " + String.join(", ", lanes)); + return; + } + + if (tags.contains("gravitino-docker-test")) { + System.out.println( + args[0] + + " carries gravitino-docker-test but no backend tag - it will NOT run in ANY" + + " lane. Add @CoreBackend.H2/@CoreBackend.MySQL/@CoreBackend.PostgreSQL or" + + " @CoreBackend.All."); + } else { Review Comment: [Nit] This verdict is wrong for the one nested-per-backend class the docs point contributors at. `findRepeatableAnnotations(testClass, Tag.class)` sees a class's own and superclass-inherited tags, not the tags JUnit adds to a `@Nested` descriptor from its enclosing class. Run against `org.apache.gravitino.stats.storage.TestJdbcPartitionStatisticStorageIT` — the very pattern `CoreBackend`'s Javadoc says to copy — this branch prints "carries gravitino-docker-test but no backend tag - it will NOT run in ANY lane" plus advice to add an annotation, although its `H2Test`/`MySQLTest`/`PostgreSQLTest` nested classes each do run in a lane. The mirror case is a `@Nested` class whose backend tag comes only from its enclosing class: the tool would call it a `coreUnitTest` class. Reporting the nested classes' verdicts, or at least walking `getEnclosingClass()` and saying where the tags came from, would keep the tool honest. Verified by: read `CoreTestLaneOf.java:59-95` and the annotations in `TestJdbcPartitionStatisticStorageIT` (`:89-90` outer, Docker tag and no backend tag; `:590`, `:661`, `:735` nested, `@CoreBackend.MySQL`/`.PostgreSQL`/`.H2`). I could not execute the task here (no JDK 17 in this container), so the `@Nested` half rests on JUnit's documented rule that tags are inherited from enclosing classes, not on a run. ########## core/build.gradle.kts: ########## @@ -103,7 +106,164 @@ artifacts { add("testArtifacts", testJar) } +// Core's tests run in one of four Gradle lanes: coreUnitTest (default, no Docker) and +// coreH2Test/coreMySQLTest/corePostgreSQLTest (one per backend, coreMySQLTest and +// corePostgreSQLTest need Docker). Lane membership is decided purely by which of the three +// backend tags below a test class carries - see CoreBackend in +// core/src/test/java/org/apache/gravitino/storage/relational/CoreBackend.java for the typed +// annotations (@CoreBackend.H2/.MySQL/.PostgreSQL/.All) that set them, instead of writing raw +// @Tag("...") strings by hand: +// @CoreBackend.H2 -> runs only in coreH2Test +// @CoreBackend.H2 @CoreBackend.MySQL -> runs in coreH2Test and coreMySQLTest +// @CoreBackend.All -> runs in all three backend lanes +// (no CoreBackend annotation at all) -> a plain unit test, runs in coreUnitTest +// A class needing Docker but carrying no backend tag runs in no lane at all - check locally +// with `./gradlew :core:coreTestLaneOf -PclassName=<fully.qualified.ClassName>`. +// +// Backend name -> JUnit tag that admits a test class to that backend's lane. Adding a backend +// here is enough to teach the lane filtering below about it; also add it to CoreBackend.java. +val coreBackendTestTags = + linkedMapOf( + "h2" to "gravitino-core-h2-test", + "mysql" to "gravitino-core-mysql-test", + "postgresql" to "gravitino-core-postgresql-test" + ) +val coreTestBackendProperty = "gravitino.core.test.backend" + +fun registerCoreTestTask( + taskName: String, + backend: String? = null +) = tasks.register<Test>(taskName) { + group = "verification" + description = + if (backend == null) { + "Runs core unit tests." + } else { + "Runs core database tests against $backend." + } + + testClassesDirs = sourceSets["test"].output.classesDirs + classpath = sourceSets["test"].runtimeClasspath + + inputs.property("coreTestSuite", backend ?: "unit") + inputs.property("coreTestBackend", backend ?: "none") + // Distinct from the extensions.extraProperties["includeDockerTaggedTests"] flag set below, + // which is a different mechanism (read by root build.gradle.kts's shared test-environment + // setup to decide JUnit tag filtering) - this is only a Gradle up-to-date-check input. + inputs.property("coreTestIncludesDockerTaggedTests", backend != null) + reports.junitXml.outputLocation.set(layout.buildDirectory.dir("test-results/$taskName")) + reports.html.outputLocation.set( + rootProject.layout.buildDirectory.dir("reports/tests/core/$taskName") + ) + + extensions.configure<JacocoTaskExtension> { + destinationFile = layout.buildDirectory.file("jacoco/$taskName.exec").get().asFile + } + + useJUnitPlatform { + if (backend == null) { + // Whatever carries no backend tag (and no Docker tag) is the unit suite. + excludeTags(*coreBackendTestTags.values.toTypedArray(), "gravitino-docker-test") + } else { + val ownBackendTag = + coreBackendTestTags[backend] + ?: throw GradleException("Unsupported core test backend: $backend") + // Plain tag include, applied by JUnit at discovery time, so classes not tagged for this + // backend never show up in this lane's JUnit XML. A class tagged for several backends + // runs under each of them. + includeTags(ownBackendTag) Review Comment: [Important] Nothing fails when a core test class ends up in no lane, or leaves every lane at once. Lane membership is now purely tag-driven: the unit lane excludes the three backend tags plus `gravitino-docker-test` (line 166) and each database lane includes exactly one backend tag (this line). A class carrying `@Tag("gravitino-docker-test")` and no `@CoreBackend.*` therefore runs in **no** CI lane — `CoreBackend`'s own Javadoc says so (`CoreBackend.java:43-45`) — and nothing detects it: `reconcile` only requires the three database lanes to equal *each other* (`dev/ci/core_test_identity.py:418-424`), and `manifest` only rejects a wholly empty lane (`:253-255`). So a refactor that drops `@CoreBackend.All` from `AbstractEntityStorageTest` removes ~52 cases from all three lanes at once and CI stays green; the only detector in the tree is `coreTestLaneOf`, which a contributor has to think to run. Since the PR sells "fail-closed equivalence checks", this one seems worth closing too. Cheapest form: a unit-lane test that walks the compiled test classes and fails on any class whose tags contain `gravitino-docker-test` but none of `CoreBackend.H2_TAG/MYSQL_TAG/POSTGRESQL_TAG`. That covers the orphan case at build time and is the part of my earlier "cheap extra guard" note that is still open. Verified by: read `core/build.gradle.kts:133-196`, `CoreBackend.java:29-60`, `dev/ci/core_test_identity.py:212-260` and `:413-431`; grepped `core/src/test` and `.github/` for any other backend-tag coverage check — the only one is the manual `:core:coreTestLaneOf` task. ########## core/build.gradle.kts: ########## @@ -103,7 +106,164 @@ artifacts { add("testArtifacts", testJar) } +// Core's tests run in one of four Gradle lanes: coreUnitTest (default, no Docker) and +// coreH2Test/coreMySQLTest/corePostgreSQLTest (one per backend, coreMySQLTest and +// corePostgreSQLTest need Docker). Lane membership is decided purely by which of the three +// backend tags below a test class carries - see CoreBackend in +// core/src/test/java/org/apache/gravitino/storage/relational/CoreBackend.java for the typed +// annotations (@CoreBackend.H2/.MySQL/.PostgreSQL/.All) that set them, instead of writing raw +// @Tag("...") strings by hand: +// @CoreBackend.H2 -> runs only in coreH2Test +// @CoreBackend.H2 @CoreBackend.MySQL -> runs in coreH2Test and coreMySQLTest +// @CoreBackend.All -> runs in all three backend lanes +// (no CoreBackend annotation at all) -> a plain unit test, runs in coreUnitTest +// A class needing Docker but carrying no backend tag runs in no lane at all - check locally +// with `./gradlew :core:coreTestLaneOf -PclassName=<fully.qualified.ClassName>`. +// +// Backend name -> JUnit tag that admits a test class to that backend's lane. Adding a backend +// here is enough to teach the lane filtering below about it; also add it to CoreBackend.java. +val coreBackendTestTags = + linkedMapOf( + "h2" to "gravitino-core-h2-test", + "mysql" to "gravitino-core-mysql-test", + "postgresql" to "gravitino-core-postgresql-test" + ) +val coreTestBackendProperty = "gravitino.core.test.backend" + +fun registerCoreTestTask( + taskName: String, + backend: String? = null +) = tasks.register<Test>(taskName) { + group = "verification" + description = + if (backend == null) { + "Runs core unit tests." + } else { + "Runs core database tests against $backend." + } + + testClassesDirs = sourceSets["test"].output.classesDirs + classpath = sourceSets["test"].runtimeClasspath + + inputs.property("coreTestSuite", backend ?: "unit") + inputs.property("coreTestBackend", backend ?: "none") + // Distinct from the extensions.extraProperties["includeDockerTaggedTests"] flag set below, + // which is a different mechanism (read by root build.gradle.kts's shared test-environment + // setup to decide JUnit tag filtering) - this is only a Gradle up-to-date-check input. + inputs.property("coreTestIncludesDockerTaggedTests", backend != null) + reports.junitXml.outputLocation.set(layout.buildDirectory.dir("test-results/$taskName")) + reports.html.outputLocation.set( + rootProject.layout.buildDirectory.dir("reports/tests/core/$taskName") + ) + + extensions.configure<JacocoTaskExtension> { + destinationFile = layout.buildDirectory.file("jacoco/$taskName.exec").get().asFile + } + + useJUnitPlatform { + if (backend == null) { + // Whatever carries no backend tag (and no Docker tag) is the unit suite. + excludeTags(*coreBackendTestTags.values.toTypedArray(), "gravitino-docker-test") + } else { + val ownBackendTag = + coreBackendTestTags[backend] + ?: throw GradleException("Unsupported core test backend: $backend") + // Plain tag include, applied by JUnit at discovery time, so classes not tagged for this + // backend never show up in this lane's JUnit XML. A class tagged for several backends + // runs under each of them. + includeTags(ownBackendTag) + } + } + + if (backend != null) { + systemProperty(coreTestBackendProperty, backend) + extensions.extraProperties["includeDockerTaggedTests"] = true + + // Database tests mutate process-wide state and must remain sequential within each lane. + maxParallelForks = 1 + systemProperty("junit.jupiter.execution.parallel.enabled", "false") + + if (backend != "h2") { + doFirst { + if (rootProject.extra["dockerTest"] != true) { + throw GradleException( + "$path requires Docker; use -PskipDockerTests=false with Docker running." + ) + } + } + } + } +} + +registerCoreTestTask("coreUnitTest") +registerCoreTestTask("coreH2Test", "h2") +registerCoreTestTask("coreMySQLTest", "mysql") +registerCoreTestTask("corePostgreSQLTest", "postgresql") + +tasks.register<JavaExec>("coreTestLaneOf") { + group = "verification" + description = "Prints which core database test lane(s) a class runs in, from its tags, " + + "without running anything. Usage: -PclassName=<fully.qualified.ClassName>" + dependsOn(tasks.named("testClasses")) + classpath = sourceSets["test"].runtimeClasspath + mainClass.set("org.apache.gravitino.storage.relational.CoreTestLaneOf") + doFirst { + val className = project.findProperty("className") as? String + ?: throw GradleException( + "Usage: ./gradlew :core:coreTestLaneOf -PclassName=<fully.qualified.ClassName>" + ) + args(className) + } +} + +val coreSuiteCoverage = + providers.gradleProperty("coreSuiteCoverage").map(String::toBoolean).orElse(false) +val coreSuiteTaskNames = + listOf("coreUnitTest", "coreH2Test", "coreMySQLTest", "corePostgreSQLTest") +val coreSuiteExecutionData = + coreSuiteTaskNames.map { layout.buildDirectory.file("jacoco/$it.exec") } +val validateCoreSuiteCoverage by tasks.registering { + inputs.files(coreSuiteExecutionData) + + doLast { + val missingExecutionData = + coreSuiteExecutionData + .map { it.get().asFile } + .filterNot { it.isFile && it.length() > 0L } + if (missingExecutionData.isNotEmpty()) { + throw GradleException( + "Missing core JaCoCo execution data: ${missingExecutionData.joinToString()}" + ) + } + } +} + +tasks.named<JacocoReport>("jacocoTestReport") { + if (coreSuiteCoverage.get()) { + dependsOn(tasks.named("classes"), validateCoreSuiteCoverage) + executionData.setFrom(coreSuiteExecutionData) + } +} + +// :core:test is the java plugin's built-in `test` task, kept registered (and working) only for +// backward compatibility - IDEs and other tooling may still target it by convention. It is +// deprecated in place, not removed: +// - Dev CUJ: a contributor running tests locally should target one of the four lanes registered +// above (coreUnitTest / coreH2Test / coreMySQLTest / corePostgreSQLTest), never :core:test - +// it predates the lane split and does not correspond to any CI lane. Check where a class runs +// with `./gradlew :core:coreTestLaneOf -PclassName=...` instead of guessing. +// - CI CUJ: no change needed here. CI never invokes :core:test - dev/ci/test-shards.sh emits +// `-x :core:test` for the `others` shard, so the warning below only ever fires for a developer +// running it directly. Review Comment: [Nit] "CI never invokes `:core:test`" holds for `build.yml` only. `dev/ci/test-shards.sh` emits `-x :core:test` inside the `build`-suite branch alone (`:127-131`). The `backend-it` suite's catch-all is `print_others backend-it test` (`:163`), i.e. the root `test` task with no core exclusion, and `.github/workflows/backend-integration-test-action.yml:73-76` runs exactly that with `-PskipTests`. `:core:test` is therefore realized and executed (filtered to `**/integration/test/**`, which matches nothing in core) in every BackendIT `others` job, and this `doFirst` prints the deprecation warning there. Harmless in itself, but "only ever fires for a developer running it directly" is not accurate, and the sentence would mislead whoever next touches the shard table. Verified by: read `dev/ci/test-shards.sh:119-141` and `:156-170`, `.github/workflows/backend-integration-test.yml:60-85`, `.github/workflows/backend-integration-test-action.yml:73-90`, and root `build.gradle.kts:509-513` for the `-PskipTests` include filter. ########## core/build.gradle.kts: ########## @@ -103,7 +106,164 @@ artifacts { add("testArtifacts", testJar) } +// Core's tests run in one of four Gradle lanes: coreUnitTest (default, no Docker) and +// coreH2Test/coreMySQLTest/corePostgreSQLTest (one per backend, coreMySQLTest and +// corePostgreSQLTest need Docker). Lane membership is decided purely by which of the three +// backend tags below a test class carries - see CoreBackend in +// core/src/test/java/org/apache/gravitino/storage/relational/CoreBackend.java for the typed +// annotations (@CoreBackend.H2/.MySQL/.PostgreSQL/.All) that set them, instead of writing raw +// @Tag("...") strings by hand: +// @CoreBackend.H2 -> runs only in coreH2Test +// @CoreBackend.H2 @CoreBackend.MySQL -> runs in coreH2Test and coreMySQLTest +// @CoreBackend.All -> runs in all three backend lanes +// (no CoreBackend annotation at all) -> a plain unit test, runs in coreUnitTest +// A class needing Docker but carrying no backend tag runs in no lane at all - check locally +// with `./gradlew :core:coreTestLaneOf -PclassName=<fully.qualified.ClassName>`. +// +// Backend name -> JUnit tag that admits a test class to that backend's lane. Adding a backend +// here is enough to teach the lane filtering below about it; also add it to CoreBackend.java. +val coreBackendTestTags = + linkedMapOf( + "h2" to "gravitino-core-h2-test", + "mysql" to "gravitino-core-mysql-test", + "postgresql" to "gravitino-core-postgresql-test" + ) +val coreTestBackendProperty = "gravitino.core.test.backend" + +fun registerCoreTestTask( + taskName: String, + backend: String? = null +) = tasks.register<Test>(taskName) { + group = "verification" + description = + if (backend == null) { + "Runs core unit tests." + } else { + "Runs core database tests against $backend." + } + + testClassesDirs = sourceSets["test"].output.classesDirs + classpath = sourceSets["test"].runtimeClasspath + + inputs.property("coreTestSuite", backend ?: "unit") + inputs.property("coreTestBackend", backend ?: "none") + // Distinct from the extensions.extraProperties["includeDockerTaggedTests"] flag set below, + // which is a different mechanism (read by root build.gradle.kts's shared test-environment + // setup to decide JUnit tag filtering) - this is only a Gradle up-to-date-check input. + inputs.property("coreTestIncludesDockerTaggedTests", backend != null) + reports.junitXml.outputLocation.set(layout.buildDirectory.dir("test-results/$taskName")) + reports.html.outputLocation.set( + rootProject.layout.buildDirectory.dir("reports/tests/core/$taskName") + ) + + extensions.configure<JacocoTaskExtension> { + destinationFile = layout.buildDirectory.file("jacoco/$taskName.exec").get().asFile + } + + useJUnitPlatform { + if (backend == null) { + // Whatever carries no backend tag (and no Docker tag) is the unit suite. + excludeTags(*coreBackendTestTags.values.toTypedArray(), "gravitino-docker-test") + } else { + val ownBackendTag = + coreBackendTestTags[backend] + ?: throw GradleException("Unsupported core test backend: $backend") + // Plain tag include, applied by JUnit at discovery time, so classes not tagged for this + // backend never show up in this lane's JUnit XML. A class tagged for several backends + // runs under each of them. + includeTags(ownBackendTag) + } + } + + if (backend != null) { + systemProperty(coreTestBackendProperty, backend) + extensions.extraProperties["includeDockerTaggedTests"] = true Review Comment: [Question] What actually keeps the root `excludeTags("gravitino-docker-test")` off the H2 lane? This line is the only signal to `setTestEnvironment` (root `build.gradle.kts:517-524`) that a lane wants Docker-tagged tests, and it is set inside the action passed to `tasks.register<Test>`, while the reader runs from `subprojects { tasks.configureEach<Test> { ... } }` (root `build.gradle.kts:979-982`). CI says the mechanism works today: `core-h2` runs with `-PskipDockerTests=true` and no `SKIP_DOCKER_TESTS` exists anywhere in `.github/`, so `dockerTest` is false in that job, yet `core-test-contract` passed on this head under strict h2/mysql identity equality — only possible if the four `@CoreBackend.All` classes, all `@Tag("gravitino-docker-test")`, really ran in the H2 lane. What I could not confirm is *why*. Gradle 8.2's `Test.useTestFramework` returns early when the framework class is unchanged, so core's second `useJUnitPlatform { ... }` configures the *same* `JUnitPlatformOptions` instance and accumulates tags rather than resetting them — an `excludeTags("gravitino-docker-test")` added by the root action would survive into this lane. The lane's tag set therefore hangs on `register`-vs-`configureEach` ordering, which nothing documents and no test pins; if that ever flips (Gradle upgrade, or the root block moving into `projectsEvaluated`), the H2 lane silently loses those cases and only `core-test-contract` notices. Could you either record the ordering this relies on right here, or make it explicit — e.g. let `setTestEnvironment` skip the Docker exclusion for `:core`'s lane tasks by task name and drop the extraProperties channel entirely? Verified by: read root `build.gradle.kts:431-535` and `:978-1035`, `core/build.gradle.kts:133-196`; `javap -c` on `org/gradle/api/tasks/testing/Test.class` from the repo's own Gradle 8.2 wrapper distribution (`useJUnitPlatform(Action)` → `useJUnitPlatform()` → `useTestFramework`, which returns when the existing framework's class equals the new one's, then `applyOptions` on the existing options); grepped `.github/` and `dev/` for `SKIP_DOCKER_TESTS` (absent); read the `core-h2` (job 109094579683) and `core-test-contract` (job 109118944175) logs of run 36466794673, which is on this head. No local Gradle run was possible here — this container only has JDK 21 and the build rejects it. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
