jerryshao commented on code in PR #13517: URL: https://github.com/apache/gravitino/pull/13517#discussion_r4216473940
########## dev/ci/core_test_identity.py: ########## @@ -0,0 +1,509 @@ +#!/usr/bin/env python3 +# 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. + +"""Build and reconcile normalized core-test identity manifests. + +Gradle writes one JUnit XML directory per Test task. This tool turns those +reports into stable, backend-neutral identity multisets so the H2, MySQL, and +PostgreSQL lanes can prove that they exercised the same test contract. It also +records status counts and elapsed test time for CI artifacts. +""" + +import argparse +from collections import Counter +from decimal import Decimal, InvalidOperation +import hashlib +import json +import math +from pathlib import Path +import re +import sys +import xml.etree.ElementTree as ET + + +SCHEMA_VERSION = 1 +LANES = ("unit", "h2", "mysql", "postgresql") +DATABASE_LANES = LANES[1:] +MANIFEST_LANES = LANES +STATUS_KEYS = ("passed", "skipped", "failures", "errors") + +BACKEND_NAME_PATTERN = r"h2|mysql|postgresql" +TEST_TEMPLATE_BACKEND_RE = re.compile( + rf"\[(?P<backend>{BACKEND_NAME_PATTERN})\s+Backend\]", re.IGNORECASE +) +STATS_BACKEND_CLASS_RE = re.compile( + rf"(?P<prefix>TestJdbcPartitionStatisticStorageIT)\$" + rf"(?P<backend>{BACKEND_NAME_PATTERN})Test(?=$|\$)", + re.IGNORECASE, +) +BACKEND_TOKEN_RE = re.compile( Review Comment: `BACKEND_TOKEN_RE`'s boundaries are `(?<![A-Za-z0-9])` / `(?![A-Za-z0-9])`, so `_`, spaces, commas and brackets all count as delimiters. I ran the regex against realistic JUnit names: ``` MATCH ['mysql'] testParseJdbcUrl_mysql_default() MATCH ['h2'] convert_h2_type() MATCH ['mysql'] [1] mysql, true MATCH ['mysql'] testBackendConfig[mysql] ok testMySQLDialect() <- camelCase is safe ok testH2() <- camelCase is safe ``` Because `_test_name_backend_markers` feeds this into `normalize_identity`, which raises `ManifestError` on *any* marker in the unit lane (line 120) and on foreign markers in a database lane (line 126), two ordinary changes break CI: 1. Someone adds a core test that has nothing to do with databases but names the method in snake_case with `_mysql_` in it, or uses `@ParameterizedTest` with the string `"mysql"` as an argument (the default display name is then `[1] mysql`). The `coreUnitTest` lane's "Generate core test manifest" step fails with `Unit test result contains an explicit backend marker ['mysql']` — an error with no relationship to what they changed, in the one lane that is explicitly *not* about databases. 2. Someone adds a parameterized test over the three dialect names to an `@CoreBackend.All` class; the h2 lane then fails with `foreign backend marker(s) ['mysql', 'postgresql']`. This is latent rather than live only by luck: the `TestBackendTestSelector` added in this PR happens to use plain `@Test` with camelCase names. Written as `@ParameterizedTest` with `@ValueSource(strings = {"h2", "mysql", "postgresql"})` — the natural shape for testing a backend selector — it would already be red. `_normalize_test_name` does need this regex to line up names like `[1] mysql, true` across lanes, so it can't just be deleted. But could the two `ManifestError` checks be based only on the structured markers (`TEST_TEMPLATE_BACKEND_RE` and `STATS_BACKEND_CLASS_RE`), leaving `BACKEND_TOKEN_RE` to normalization alone? That still catches real structured leakage between lanes without failing on incidental substrings. ########## core/src/test/java/org/apache/gravitino/storage/relational/BackendTestExtension.java: ########## @@ -121,19 +130,25 @@ 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()); + // No trailing "()" here: @TestTemplate methods can declare parameters (e.g. an injected + // DatabaseTestContext), and a hardcoded empty parameter list would misrepresent the + // method's actual signature in JUnit XML/HTML reports. + return String.format("%s[%s Backend]", testMethodName, backendType.toUpperCase()); Review Comment: Prefixing the method name is necessary for identity normalization, and the comment about not hardcoding `()` is a good catch. Worth noting the blast radius though: this rewrites the `<testcase name>` of every core `@TestTemplate` test in JUnit XML and HTML — by the PR's own numbers 519 tests × 3 lanes — and also under the legacy `:core:test` path, which this PR keeps working. Anything keyed on the old names loses continuity: test-history and flaky-test tracking will treat the renamed tests as brand new, and an exact-match `--tests` filter in a script or doc stops resolving (a trailing-wildcard one like `*[H2 Backend]` still works, since the suffix is unchanged). Not a defect, but it is an observable change that the "Does this PR introduce any user-facing change?" section doesn't mention — it currently only covers the Gradle tasks and the new system property. ########## .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 }}) Review Comment: This changes every check name the repository currently reports. `build` goes from a single `build (17)` to six `build (17, core-unit)` / `(17, core-h2)` / ... entries, and in `backend-integration-test.yml` the `BackendIT-on-push` / `BackendIT-on-pr` pair becomes nine `BackendIT (linux/amd64, 17, <backend>, <mode>, <shard>)` entries. Any required status check configured in `apache/gravitino`'s branch protection under the old names will never report again, so every open PR sits at "Expected — Waiting for status to be reported" and cannot merge until INFRA reconfigures them. That is a coordinated cutover rather than a defect, but it isn't mentioned in the description, and this is the second PR in a five-deep stack (#13495 → #13516 → #13518 → #13530 → #13553) where each step may rename jobs again. Could the final check-name list and the intended merge order be written down in one place — the epic or this description — so whoever merges knows what has to change alongside each step? (Consolidating `-on-push` / `-on-pr` into one job is a genuine improvement in its own right — the `if` on `source_changes` alone covers both events.) ########## .github/workflows/build.yml: ########## @@ -190,12 +200,20 @@ jobs: exit 0 fi + shard_args_text="$(dev/ci/test-shards.sh build "${{ matrix.shard }}")" + mapfile -t shard_args <<< "${shard_args_text}" + + skip_docker_tests=false + case "${{ matrix.shard }}" in + core-unit|core-h2) skip_docker_tests=true ;; Review Comment: `core-h2` runs with `-PskipDockerTests=true` (so `dockerTest=false`), but `core/build.gradle.kts:182` sets `includeDockerTaggedTests = true` for *every* backend lane including h2, while the Docker-presence guard on line 188 skips h2 via `if (backend != "h2")`. Today that combination is correct — H2 needs no container and only shares classes that are `gravitino-docker-test`-tagged because MySQL/PostgreSQL need one — but it leaves two edges: 1. If someone adds `@CoreBackend.H2` to a class that genuinely needs a container for something else, the h2 lane will run it with no Docker and no fail-fast, unlike the clear `doFirst` message mysql/postgresql get. 2. More subtly, h2's `dockerTest` environment variable is `false` while the other two lanes see `true`, and `core_test_identity.py reconcile` requires the three lanes' identity sets to be *exactly* equal. Method-level `assumeTrue` / `@Disabled` are fine because they still emit `<testcase><skipped/>`, but a container-level conditional annotation (e.g. `@EnabledIfEnvironmentVariable(named = "dockerTest", matches = "true")` on a class) emits no testcase at all, so h2 and mysql would diverge and surface as `Database identity mismatch between h2 and mysql` — an error that gives no hint the real cause is an environment difference between lanes. Core currently only has two method-level `@Disabled` in `TestLockManager`, so nothing trips this. Either running h2 with `-PskipDockerTests=false` too (keeping all three lane environments identical) or recording "no container-level conditional annotations on backend test classes" as an explicit constraint in `design-docs/testing/core-db-split.md` would close 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]
