jerryshao commented on code in PR #13517:
URL: https://github.com/apache/gravitino/pull/13517#discussion_r4119467008
##########
core/build.gradle.kts:
##########
@@ -103,6 +106,107 @@ artifacts {
add("testArtifacts", testJar)
}
+val coreDatabaseTestTag = "gravitino-core-database-test"
+val coreH2TestTag = "gravitino-core-h2-test"
+val coreMySQLTestTag = "gravitino-core-mysql-test"
+val corePostgreSQLTestTag = "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")
+ inputs.property("includeDockerTaggedTests", backend != null)
Review Comment:
[Nit] `inputs.property("includeDockerTaggedTests", backend != null)` only
registers a Gradle up-to-date-check input. It shares its name with the
`extensions.extraProperties["includeDockerTaggedTests"]` flag set 26 lines
below (line 158), but nothing reads it, and the two are unrelated mechanisms.
Since the extraProperties flag is itself never observed (see the comment on
`build.gradle.kts`), this pair reads as the wiring while neither half works.
Worth renaming one of them once the real mechanism is fixed.
Verified by: grep for `includeDockerTaggedTests` across the repository --
the only reader is `build.gradle.kts:518`.
##########
build.gradle.kts:
##########
@@ -514,9 +514,12 @@ allprojects {
val dockerTest = project.rootProject.extra["dockerTest"] as? Boolean ?:
false
param.environment("dockerTest", dockerTest.toString())
+ val includeDockerTaggedTests =
+
param.extensions.extraProperties.properties["includeDockerTaggedTests"] as?
Boolean
+ ?: dockerTest
Review Comment:
[Important] The `includeDockerTaggedTests` opt-in never takes effect, so the
`core-h2` lane silently loses every Docker-tagged test.
`core/build.gradle.kts:158` sets
`extensions.extraProperties["includeDockerTaggedTests"] = true` inside the
`tasks.register<Test>(...)` configuration action, but this line reads it from
`setTestEnvironment`, which is invoked from `subprojects {
tasks.configureEach<Test> { ... } }` at `build.gradle.kts:978-981`. Gradle
executes a collection's add-actions (`configureEach`/`withType`) *before* the
action passed to `register`, so the property is always `null` here and the
expression falls back to `dockerTest`.
Consequence: `.github/workflows/build.yml:206-208` runs the `core-h2` shard
with `-PskipDockerTests=true`, so `dockerTest` is false and
`excludeTags("gravitino-docker-test")` is applied to `:core:coreH2Test`. That
drops `TestEntityStorage`, `TestEntityStorageChangeLog`,
`TestEntityStorageRelationCache` and `TestEntityStorageForLance` (26
`@ParameterizedTest` methods x 2 provider rows = 52 cases) plus
`TestJdbcPartitionStatisticStorageIT$H2Test` -- all class-level
`@Tag("gravitino-docker-test")` yet all running against embedded H2. The MySQL
and PostgreSQL lanes run with `-PskipDockerTests=false` and keep them, so
`reconcile_manifests` (`dev/ci/core_test_identity.py:424-432`, which requires
`counters["mysql"] == counters["h2"]`) fails the `core-test-contract` job.
Because the `register` action is the *last* thing to run, the order-safe fix
is to clear the inherited tag there rather than to signal ahead of it -- e.g.
in `registerCoreTestTask`, `useJUnitPlatform { setExcludeTags(excludeTags -
"gravitino-docker-test") ... }` when `backend != null`. Alternatively move the
decision into `setTestEnvironment` itself, keyed on `project.path` plus task
name, and drop the extraProperties channel.
One note on the local evidence: the PR description reports the H2 lane at
519 tests matching PostgreSQL, which would contradict this. That is consistent
with `SKIP_DOCKER_TESTS=false` being set in your shell --
`build.gradle.kts:1650-1654` lets that environment variable override
`-PskipDockerTests=true`, making `dockerTest` true on Linux with Docker
running. CI does not set it.
Verified by: read `build.gradle.kts:431-535` and `:978-1035`,
`core/build.gradle.kts:109-209`, and the tag declarations in
`core/src/test/java/org/apache/gravitino/storage/` (grep for
`gravitino-docker-test`). I then reproduced the configuration ordering on
Gradle 8.2 (this repo's wrapper version) with a two-project build using the
same `subprojects { withType<Test>().configureEach { ... } }` plus subproject
`tasks.register<Test> { ... }` pattern: the root action ran first and printed
`extraProp=null`, while the register action's `reports.html.outputLocation`
won. The same probe confirmed that repeated `useJUnitPlatform` calls accumulate
tags rather than reset them, which is what makes the inherited `excludeTags`
stick.
##########
core/src/test/java/org/apache/gravitino/storage/relational/BackendTestExtension.java:
##########
@@ -121,19 +130,22 @@ public Stream<TestTemplateInvocationContext>
provideTestTemplateInvocationContex
"Running tests with H2 backend only. Set env var 'dockerTest=true'
to include all backends.");
}
- return backendsToTest.stream().map(BackendInvocationContext::new);
+ return backendsToTest.stream()
+ .map(backendType -> new BackendInvocationContext(testMethodName,
backendType));
}
private static class BackendInvocationContext implements
TestTemplateInvocationContext {
+ private final String testMethodName;
private final String backendType;
- public BackendInvocationContext(String backendType) {
+ public BackendInvocationContext(String testMethodName, String backendType)
{
+ this.testMethodName = testMethodName;
this.backendType = backendType;
}
@Override
public String getDisplayName(int invocationIndex) {
- return String.format("[%s Backend]", backendType.toUpperCase());
+ return String.format("%s()[%s Backend]", testMethodName,
backendType.toUpperCase());
Review Comment:
[Nit] `String.format("%s()[%s Backend]", ...)` hardcodes an empty parameter
list into the reported name. `@TestTemplate` methods may declare
`ParameterResolver`-supplied parameters, and for those the emitted JUnit XML
name would claim a no-arg signature -- and that name is exactly what
`dev/ci/core_test_identity.py` hashes as the test identity. Deriving the suffix
from `context.getRequiredTestMethod().getParameterTypes()`, or omitting `()`
entirely, would keep the name truthful.
Verified by: read `BackendTestExtension.java:112-152` and the expectations
in `TestBackendTestSelector.java:86-96`. Forward-looking only: I grepped all
461 `@TestTemplate` declarations under `core/src/test` and none currently
declares a parameter, so no name is wrong today.
##########
.github/workflows/build.yml:
##########
@@ -147,24 +151,26 @@ jobs:
spark-connector/**/*.log
build:
- # The type of runner that the job will run on
+ name: build (${{ matrix.java-version }}, ${{ matrix.shard }})
runs-on: ubuntu-latest
strategy:
+ fail-fast: false
matrix:
java-version: [ 17 ]
- timeout-minutes: 120
+ # Shards are defined in dev/ci/test-shards.sh.
+ shard: ${{ fromJSON(needs.changes.outputs.build_shards) }}
+ timeout-minutes: 90
Review Comment:
[Question] `timeout-minutes` drops from 120 to 90 at the same time the
matrix gains a dedicated MySQL lane. Your own appendix puts `coreMySQLTest` at
1h09m of JUnit time locally; with checkout, toolchain setup, `compileTestJava`
and Testcontainers startup on top, that shard could land close to the new
ceiling. The CI baseline you cite (`:core:test` at 45m13s for unit plus all
three backends) suggests the runner is considerably faster than your local
Docker, but since #13517's workflows have not run yet there is no measurement
to confirm it. Is 90 deliberate, or would keeping 120 until a real `core-mysql`
run lands be safer?
Verified by: read `.github/workflows/build.yml:151-165` and the timing
tables in the PR description; no CI run exists on this head to check against.
--
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]
