Skip to content

MINOR: Build test artifacts once in CI - #20

Open
xborder wants to merge 4 commits into
mainfrom
ci-build-once-test-many
Open

MINOR: Build test artifacts once in CI#20
xborder wants to merge 4 commits into
mainfrom
ci-build-once-test-many

Conversation

@xborder

@xborder xborder commented Jul 28, 2026

Copy link
Copy Markdown
Owner

Summary

Build the Java and C Data artifacts once on JDK 17, then reuse them across the existing Test workflow matrix. Prebuilt test jobs invoke Surefire directly and fail if compilation occurs or no tests execute. Custom Surefire executions for the Vector allocators and memory-core are run explicitly.

Coverage trade-offs

  • Compilation now runs once on Linux with JDK 17; JDK 21 and 23 still execute the full test suite against the JDK 17 bytecode, but no longer compile the sources.
  • C Data jobs run only the C module tests in the conda environment. The base Java suite continues to run on Ubuntu for JDK 17, 21, and 23.

Validation

  • bash -n ci/scripts/build.sh ci/scripts/test.sh
  • pre-commit run --files ci/scripts/test.sh .github/workflows/test.yml
  • podman compose config --quiet
  • Local Podman build of all 22 modules
  • Local prebuilt test run with no compilation: generic reactor tests, 1,136 Vector tests under each allocator, and TestOpens

@xborder xborder added the chore label Jul 28, 2026
@github-actions

This comment has been minimized.

@xborder xborder left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: build-once / test-many

The design is well thought through: display names are preserved for required-check continuity, memory-core's opens-tests execution is explicitly re-run, the "Compiling …" / "Tests run:" guards protect against silent staleness or skips, and the linked validation run shows all 11 jobs green.

One blocking issue (inline on ci/scripts/test.sh): vector's custom surefire executions (default-test + run-unsafe) are silently skipped in prebuilt mode, so the dual-allocator coverage of the vector suite is lost on every prebuilt leg. Details and suggested fix inline.

Semantic changes worth a sentence in the PR description (likely intentional)

  • Compilation now happens only on JDK 17/Linux. The JDK 21/23 legs previously verified the sources compile under newer javac — relevant since vector compiles with -Werror and javac adds warnings across releases. Running 17-built bytecode on newer JREs is the more valuable half and is retained, but the trade-off deserves to be stated.
  • The old conda-jni-cdata legs ran the base suite under conda on all 3 JDKs; test-cdata now runs only the c module (ARROW_JAVA_TEST_BASE=OFF). Reasonable de-duplication (base suite still runs 3× on Ubuntu), but it's a coverage change, not just a speedup.

Verified fine

  • build.sh runs install -DskipTests, so test classes, test resources, and installed repo artifacts all exist for surefire:test — the tarball contents are exactly right.
  • All argLines come from static pom properties (no lifecycle-set properties), so plugin-level surefire config applies correctly under direct invocation; failsafe binds to integration-test, which the old mvn test never ran either.
  • Matrix include semantics are correct (macOS/Windows entries conflict on target, so they become standalone jobs), and matrix.name || format(...) reproduces the old display names exactly.
  • The find prune rewrite in build.sh fixes the previously broken -not \( -prune \) idiom and is necessary so the .docker artifact copies don't get picked up as source roots.
  • Windows/macOS restore steps (tar, cp -a, ~/.m2 layout) are proven by the green validation run.

Remaining comments are minor polish — inline.

Comment thread ci/scripts/test.sh
Comment thread ci/scripts/test.sh Outdated
Comment thread ci/scripts/test.sh Outdated
Comment thread .github/workflows/test.yml Outdated
Comment thread .github/workflows/test.yml
Comment thread .github/workflows/test.yml Outdated
Comment thread compose.yaml
@xborder

xborder commented Jul 29, 2026

Copy link
Copy Markdown
Owner Author

Re-review at ee3d78f ("Simplify build-once test workflow")

Verified the new commit and updated the review threads accordingly.

Resolved (4)

  • set -exo pipefail hoisted to the top of test.sh
  • Surefire logs moved to mktemp (checkout stays clean; console still has full output)
  • MAVEN single-sourced as workflow-level env in the cache keys and compose interpolation
  • retention-days note (informational)

Also checked in the new commit — all fine

  • Dropping the jni-dist volume for the pure-Java path is safe: ARROW_JAVA_JNI is unset in those legs, and test.sh handles a non-existent /tmp/jni / jni dist dir.
  • The simplified matrix (os axis + !matrix.compose_service truthiness checks) keeps correct include semantics and preserves all required-check display names.
  • x-cdata-prebuilt YAML anchor dedup merges cleanly (no environment key conflicts).
  • Removed mkdir -p .docker / rm -rf build jni steps are safe on fresh runners (tar creates paths, rm -rf tolerates absence).

Still open (3)

  1. Blocking: vector's default-test/run-unsafe surefire executions are still skipped in prebuilt mode (see thread on ci/scripts/test.sh).
  2. test-cdata still on actions/checkout@v6 / actions/cache@v5 vs v7/v6 elsewhere.
  3. PR description doesn't yet state the two coverage changes (JDK 17-only compilation; conda legs no longer run the base suite).

CI status

Test workflow for ee3d78f is still in progress at time of writing. The JNI ubuntu-latest x86_64 / JNI ubuntu-24.04-arm aarch_64 failures are in the RC workflow (CMake Error: Couldn't find xsimd >= 14.2.0 in the arrow-cpp toolchain) and also fail on main's latest RC run — pre-existing and unrelated to this PR.

@xborder

xborder commented Jul 31, 2026

Copy link
Copy Markdown
Owner Author

Re-review at b432543 ("Restore custom Surefire test coverage" + "Make Vector test passes explicit")

All review threads are now resolved.

Verified fixes

  • Vector allocator coverage (was blocking): vector is excluded from the bulk surefire:test pass and its default-test / run-unsafe executions are invoked explicitly via test@<id>, restoring the execution-scoped classpathDependencyExcludes so the suite runs once per allocator as intended. Each pass is individually protected by the Tests run: guard, so a silently-skipped pass now fails the job. Root-pom pluginManagement config (argLine etc.) still merges into the @id invocations correctly.
  • test-cdata bumped to checkout@v7 / cache@v6.
  • PR description now documents both coverage trade-offs.

CI

Full Test workflow pass on b432543 is green (Ubuntu ×3 JDKs, macOS Intel/ARM, Windows, Conda JNI ×3 JDKs). The JNI * failures on the commit are the RC workflow's pre-existing xsimd >= 14.2.0 toolchain issue, which fails identically on main — unrelated to this PR.

No remaining concerns from my side — LGTM.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant