MINOR: Build test artifacts once in CI - #20
Conversation
This comment has been minimized.
This comment has been minimized.
xborder
left a comment
There was a problem hiding this comment.
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
vectorcompiles with-Werrorand 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-cdatalegs ran the base suite under conda on all 3 JDKs;test-cdatanow runs only thecmodule (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.shrunsinstall -DskipTests, so test classes, test resources, and installed repo artifacts all exist forsurefire: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 tointegration-test, which the oldmvn testnever ran either. - Matrix
includesemantics are correct (macOS/Windows entries conflict ontarget, so they become standalone jobs), andmatrix.name || format(...)reproduces the old display names exactly. - The
findprune rewrite inbuild.shfixes the previously broken-not \( -prune \)idiom and is necessary so the.dockerartifact copies don't get picked up as source roots. - Windows/macOS restore steps (
tar,cp -a,~/.m2layout) are proven by the green validation run.
Remaining comments are minor polish — inline.
Re-review at ee3d78f ("Simplify build-once test workflow")Verified the new commit and updated the review threads accordingly. Resolved (4)
Also checked in the new commit — all fine
Still open (3)
CI statusTest workflow for ee3d78f is still in progress at time of writing. The |
Re-review at b432543 ("Restore custom Surefire test coverage" + "Make Vector test passes explicit")All review threads are now resolved. Verified fixes
CIFull Test workflow pass on b432543 is green (Ubuntu ×3 JDKs, macOS Intel/ARM, Windows, Conda JNI ×3 JDKs). The No remaining concerns from my side — LGTM. |
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-coreare run explicitly.Coverage trade-offs
Validation
bash -n ci/scripts/build.sh ci/scripts/test.shpre-commit run --files ci/scripts/test.sh .github/workflows/test.ymlpodman compose config --quietTestOpens