jerryshao commented on code in PR #13517:
URL: https://github.com/apache/gravitino/pull/13517#discussion_r4267650074
##########
core/build.gradle.kts:
##########
@@ -105,11 +105,109 @@ artifacts {
add("testArtifacts", testJar)
}
-tasks.test {
- val policiesDocument = rootProject.file("docs/policies.md")
- inputs.file(policiesDocument)
- systemProperty("gravitino.repositoryRoot", rootProject.projectDir.path)
+// Core's tests run in four lanes: coreUnitTest, plus
coreH2Test/coreMySQLTest/corePostgreSQLTest.
+// A class joins a backend lane through its @CoreBackend.* annotation; see
CoreBackend.java.
+// Backend name -> the JUnit tag that admits a class to that lane. Keep in
sync with CoreBackend.
+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
+
+ reports.html.outputLocation.set(
+ rootProject.layout.buildDirectory.dir("reports/tests/core/$taskName")
+ )
+
+ 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)
+ // Read by the root setTestEnvironment inside doFirst, i.e. at execution
time, after every
+ // configuration action has run - so it doesn't depend on
register/configureEach ordering.
+ extensions.extraProperties["includeDockerTaggedTests"] = true
Review Comment:
[Question] This is now the only thing keeping the Docker-tagged classes in
the h2 lane, and after the cut nothing detects it if it stops working.
The channel is string-keyed across two build files: this line sets the task
extra property, and root `build.gradle.kts:519-525` reads it inside
`setTestEnvironment`'s `doFirst` to decide whether
`excludeTags("gravitino-docker-test")` applies. `core-test (17, h2)` runs with
`-PskipDockerTests=true`, so without the flag the lane loses the four
`@CoreBackend.All` classes under
`core/src/test/java/org/apache/gravitino/storage/` (all class-level
`@Tag("gravitino-docker-test")`: `TestEntityStorage.java:60`,
`TestEntityStorageChangeLog.java:47`, `TestEntityStorageRelationCache.java:73`,
`TestEntityStorageForLance.java:46`), plus
`TestJdbcPartitionStatisticStorageIT$H2Test` keeps running since it carries no
Docker tag.
It works today - I am not reporting a bug. What changed since my last review
is the safety net: `core_test_identity.py reconcile` used to require h2 ==
mysql == postgresql identity sets, so a silent h2 drop failed CI. That job is
gone, and the lanes are now compared by nothing, so the same drop would just
report fewer tests in a job that stays green.
I am explicitly not asking for the Python tool back. Two cheaper options:
- Run the h2 lane with `-PskipDockerTests=false` in `build.yml`. The runner
has Docker, `AbstractEntityStorageTest.storageProvider()` filters to h2 rows
and `BackendTestExtension` returns only the selected backend, so still no
container is started, and the Docker-tag decision stops depending on this flag
at all in the one job where it matters.
- Or assert lane membership from inside `coreUnitTest`, next to
`TestCoreLaneMembership`: that the four `TestEntityStorage*` classes carry
`CoreBackend.H2_TAG`. That catches a lost annotation but not a lost flag, so it
is the weaker of the two.
Verified by: read `core/build.gradle.kts:133-175` and root
`build.gradle.kts:514-530`; grepped `core/src/test` for `gravitino-docker-test`
(7 hits, all class-level); confirmed via the job list of run 38280654558 that
`core-test (17, h2)` passed with `-PskipDockerTests=true` and that
`core-test-contract` ran there but is absent from this head's `build.yml`.
##########
core/src/test/java/org/apache/gravitino/storage/relational/TestCoreLaneMembership.java:
##########
@@ -0,0 +1,107 @@
+/*
+ * 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 static org.junit.jupiter.api.Assertions.assertTrue;
+
+import java.lang.reflect.Modifier;
+import java.net.URL;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
+import java.util.stream.Collectors;
+import org.junit.jupiter.api.Tag;
+import org.junit.jupiter.api.Test;
+import org.junit.platform.commons.annotation.Testable;
+import org.junit.platform.commons.support.AnnotationSupport;
+import org.junit.platform.commons.support.HierarchyTraversalMode;
+import org.junit.platform.commons.support.ReflectionSupport;
+
+/**
+ * Fails when a core test class runs in no lane: it carries {@code
gravitino-docker-test}, which
+ * keeps it out of {@code coreUnitTest}, but no {@link CoreBackend} tag, which
keeps it out of every
+ * database lane. Nothing else notices a class that leaves every lane at once.
+ */
+class TestCoreLaneMembership {
+
+ private static final String DOCKER_TAG = "gravitino-docker-test";
+
+ private static final Set<String> BACKEND_TAGS =
+ Set.of(CoreBackend.H2_TAG, CoreBackend.MYSQL_TAG,
CoreBackend.POSTGRESQL_TAG);
+
+ @Test
+ void everyDockerTaggedTestClassHasABackendLane() {
+ URL coreTestClasses = codeLocation(TestCoreLaneMembership.class);
+ List<String> orphans =
+ ReflectionSupport.findAllClassesInPackage(
+ "org.apache.gravitino",
+ c -> coreTestClasses.equals(codeLocation(c)) &&
declaresTests(c),
+ name -> true)
+ .stream()
+ .filter(
+ c -> {
+ Set<String> tags = effectiveTags(c);
+ return tags.contains(DOCKER_TAG)
+ && tags.stream().noneMatch(BACKEND_TAGS::contains);
+ })
+ .map(Class::getName)
+ .sorted()
+ .collect(Collectors.toList());
+
+ assertTrue(
+ orphans.isEmpty(),
+ "These classes carry gravitino-docker-test but no @CoreBackend.* tag,
so they run in no "
+ + "core test lane. Add @CoreBackend.H2/MySQL/PostgreSQL or
@CoreBackend.All: "
+ + orphans);
+ }
+
+ /**
+ * Whether JUnit would run {@code c}: concrete, with at least one {@code
@Test}-style method (all
+ * meta-annotated with {@link Testable}), own or inherited.
+ */
+ private static boolean declaresTests(Class<?> c) {
+ return !Modifier.isAbstract(c.getModifiers())
+ && !ReflectionSupport.findMethods(
+ c,
+ m -> AnnotationSupport.isAnnotated(m, Testable.class),
+ HierarchyTraversalMode.TOP_DOWN)
+ .isEmpty();
+ }
+
+ /**
+ * Own and inherited tags, plus the enclosing classes' tags for non-static
{@code @Nested}
+ * classes, which JUnit applies to them. Static member classes are
discovered as top-level classes
+ * and don't inherit their enclosing class's tags.
+ */
+ private static Set<String> effectiveTags(Class<?> c) {
+ Set<String> tags = new HashSet<>();
+ for (Class<?> k = c;
+ k != null;
+ k = Modifier.isStatic(k.getModifiers()) ? null :
k.getEnclosingClass()) {
+ AnnotationSupport.findRepeatableAnnotations(k, Tag.class).forEach(t ->
tags.add(t.value()));
+ }
+ return tags;
+ }
Review Comment:
[Nit] The orphan guard reads class-level tags only, so a method-level Docker
tag still escapes every lane.
`effectiveTags` collects `@Tag` from the class, its superclasses and - for
non-static nested classes - its enclosing classes, which is the right model for
JUnit. But a `@Tag("gravitino-docker-test")` on a *method* of a class that
carries no `@CoreBackend.*` is invisible here, while
`core/build.gradle.kts:141` excludes that tag from `coreUnitTest`
unconditionally and the database lanes include only their own tag. Such a
method runs in no CI lane and this test stays green - the one case the guard
exists to prevent. Before this PR it still ran, because `build (17)` executed
`:core:test` with `-PskipDockerTests=false`.
Latent, not live: all seven `gravitino-docker-test` occurrences in
`core/src/test` are class-level, so nothing is dropped today. If you want to
close it, the filter could also consider the tags of the methods
`declaresTests` already walks, and treat a class with any Docker-tagged test
method the same as a Docker-tagged class.
Verified by: read `TestCoreLaneMembership.java:47-100` and
`core/build.gradle.kts:133-150`; grepped `core/src/test` for
`gravitino-docker-test` - `TestEntityStorage.java:60`,
`TestEntityStorageChangeLog.java:47`, `TestEntityStorageRelationCache.java:73`,
`TestEntityStorageForLance.java:46`,
`TestJdbcPartitionStatisticStorageIT.java:89/591/662`, all on classes. I did
not run the lanes (no JDK 17 Gradle run inside the review budget).
##########
build.gradle.kts:
##########
@@ -1019,7 +1024,17 @@ subprojects {
val extraArgs = project.property("extraJvmArgs") as List<String>
jvmArgs = listOf("-Xmx4G") + extraArgs
useJUnitPlatform()
- finalizedBy(tasks.getByName("jacocoTestReport"))
+ val isCoreSuiteTask =
+ project.path == ":core" &&
+ name in setOf(
+ "coreUnitTest",
+ "coreH2Test",
+ "coreMySQLTest",
+ "corePostgreSQLTest"
+ )
+ if (!isCoreSuiteTask) {
Review Comment:
[Nit] The four lane names now live in two files, and the root copy fails
silently if it drifts.
This block suppresses the `jacocoTestReport` finalizer by name, while
`core/build.gradle.kts:182-183` keeps its own `coreSuiteTaskNames` list for the
merged execution data. The follow-ups on #13515 are likely to add or rename a
lane; if only core's list is updated, the new lane re-acquires the finalizer
and runs `:core:jacocoTestReport` from whatever `core/build/jacoco/test.exec`
happens to be on disk - stale coverage locally, a no-source skip in CI - with
nothing failing either way.
Since the register-action-runs-last ordering is what makes the flag on
`core/build.gradle.kts:157` work, core can equally clear its own finalizer
there (`setFinalizedBy(emptyList())`) and this root block plus its hardcoded
list can go; a name-shape test (`project.path == ":core" &&
name.startsWith("core") && name.endsWith("Test")`) would also collapse the two
lists into one rule. Either keeps the lane definition in one file, which is
also what the Oct 8 simplification ask was about.
Verified by: read root `build.gradle.kts:1019-1038` and
`core/build.gradle.kts:176-191`; Gradle's `JacocoReport.executionData` is
`@SkipWhenEmpty`, hence the no-source skip when no `test.exec` exists rather
than a failure.
--
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]
