Skip to content

test(ci): benchmark core unit-test artifact reuse (DO NOT MERGE) - #38327

Draft
wezell wants to merge 1 commit into
mainfrom
issue-38326-core-unit-reuse-experiment
Draft

wezell wants to merge 1 commit into
mainfrom
issue-38326-core-unit-reuse-experiment

Conversation

@wezell

@wezell wezell commented Oct 28, 2026

Copy link
Copy Markdown
Member

Summary

Throwaway benchmark — DO NOT MERGE. Test whether the JVM unit-test job is faster when it reuses the production and tests JARs already downloaded in maven-repo, rather than compiling core again. This is experimental evidence gathering, not a change to production CI.

Refs #38326 — reduce CI build and test preparation time.

Changes Made

  • Add a workflow gated to this experiment branch. Build once, then run baseline and reuse on the same Ubuntu runner type with the same initial-build artifacts.
  • Keep the existing Maven-job action for runner setup, dependency caches, artifact downloads, and report publication. Both paths use the normal native libvips setup.
  • Baseline runs the core test lifecycle. Reuse resolves the project version, validates and unpacks both core JARs (including resources), then runs initialize surefire:test without compilation.
  • Disable Maven build-result caching in both paths and fail on zero discovered tests. Missing or corrupt JARs fail before test execution; no fallback to compilation or replacement artifacts.
  • Publish logs, Surefire reports, setup-inclusive timings, and compiler-execution counts. Compare per-class test/failure/error/skip counts. The summary refuses a speedup claim unless both jobs pass with equivalent test results, the baseline compiles, and reuse does not.
  • Leave all shared actions, normal PR/merge-queue workflows, and the test matrix unchanged.

Testing

  • Eight approved fixture-based regression checks: baseline lifecycle; restoration of classes/resources and removal of stale outputs; no compilation in reuse; each missing/corrupt JAR; failure propagation in both paths.
  • Confirmed Red before adding the benchmark runner, then all checks passed.
  • Bash syntax, ShellCheck, actionlint, and git diff --check passed.
  • Offline Maven smoke checks confirmed non-recursive exec:exec can launch the helper and version evaluation returns 1.0.0-SNAPSHOT without compilation.
  • Full unit-test equivalence and real runner timings are pending this PR's experiment workflow.

Breaking Changes

None to production CI. This branch-only benchmark is deliberately not intended to merge.

Interpretation

The inner benchmark is launched through a common non-recursive Maven exec invocation after normal artifact restoration. Both paths pay that wrapper overhead. Timings include native setup, Java/cache setup, repository downloads, and candidate JAR restoration; job durations also remain available in Actions. One paired run is exploratory, not a reliable performance estimate. Existing test flakes or runner variance may invalidate the comparison.

@github-actions

Copy link
Copy Markdown
Contributor

❌ Linked Issue Needs Team Label

This PR is linked to issue #38326, but that issue has no Team : * label. Every linked issue must be owned by a team for tracking and triage.

How to fix this:

Apply a Team : * label to the linked issue (e.g., Team : Scout, Team : Platform, Team : Falcon, Team : Maintenance). Then push a new commit or edit the PR description to re-run this check.


This comment was automatically generated by the issue linking workflow

@github-actions github-actions Bot added the Area : CI/CD PR changes GitHub Actions/workflows label Oct 28, 2026
@claude

claude Bot commented Oct 28, 2026 •

Copy link
Copy Markdown
Contributor

Claude finished @wezell's task in 3m 46s —— View job


Code Review — core unit-test artifact reuse benchmark

I read all three added files and traced the moving parts: the maven-job action's local-repo layout (~/.m2/repository), the Maven version (3.9.2, from .mvn/wrapper/maven-wrapper.properties), and the dotCMS test-jar binding. Notes below. This is a throwaway DO NOT MERGE branch-gated workflow, so none of this touches production CI — but one bug defeats the experiment's own conclusion.

New Issues

  • 🟠 High: .github/workflows/cicd_experiment_core-unit-reuse.yml:111 — the compiler-execution counter regex --- compiler:\S+:(?:compile|testCompile) will almost certainly match zero lines. Maven 3.9.2 logs a lifecycle mojo as the plugin artifactId, not its goal prefix (ExecutionEventLogger.mojoStarted → MojoExecution.getArtifactId()), i.e. the line is --- maven-compiler-plugin:<ver>:compile (default-compile) @ dotcms-core ---, which does not start with --- compiler:. Consequences: baseline['compiler_executions'] is 0, so the equivalence gate at line 174 (baseline['compiler_executions'] > 0) is never satisfied and every run — even a clean, genuinely-equivalent one — prints "Not equivalent or a job failed: no speedup claim is valid" and the assert equivalent at line 185 fails the results job. It also turns the reuse['compiler_executions'] == 0 guard (line 175) into a false negative: if reuse ever did recompile, this regex would not notice. Fix: broaden the prefix, e.g. --- (?:maven-compiler-plugin|compiler):\S+:(?:compile|testCompile)[ (]. [Fix this →](https://claude.ai/code?q=In%20.github%2Fworkflows%2Fcicd_experiment_core-unit-reuse.yml%20on%20branch%20issue-38326-core-unit-reuse-experiment%2C%20the%20compiler-execution%20regex%20at%20line%20111%20is%20%60--- compiler%3A%5CS%2B%3A%28%3F%3Acompile%7CtestCompile%29%20%60%20but%20Maven%203.9.2%20logs%20%60--- maven-compiler-plugin%3A%3Cver%3E%3Acompile%60%2C%20so%20it%20matches%20nothing%20and%20the%20equivalence%20gate%20always%20fails.%20Broaden%20it%20to%20match%20both%20%60maven-compiler-plugin%3A%60%20and%20%60compiler%3A%60.&repo=dotCMS/core)

  • 🟡 Medium: .github/scripts/benchmark-core-unit-tests.sh:6 and .github/workflows/cicd_experiment_core-unit-reuse.yml:76-82 — the comparison's validity rests on the restored JARs having been compiled with the same -Dprod=true profile that baseline compiles under (line 6 passes -Dprod=true). The reuse path never recompiles; it unpacks whatever cicd_comp_build-phase.yml (core-build: true) installed. Assumption: the build-phase core build compiles with -Dprod=true. What to verify: if that initial build did not use -Dprod, baseline and reuse run different bytecode/resources and any test divergence (or false equivalence) is an artifact of the profile mismatch, not of reuse. Worth asserting the profile explicitly on the build job to make the paired comparison sound.

  • 🟡 Medium: .github/scripts/benchmark-core-unit-tests.sh:14-15 — the version is parsed with help:evaluate ... -DforceStdout | tail -n 1 under -q but without --no-transfer-progress (unlike the args array, which includes it). If the maven-help-plugin or any transitive artifact still needs resolving on the runner, progress/transfer lines can land on stdout and tail -n 1 would capture a non-version line, which then fails the regex at line 16 and aborts reuse. Low likelihood given the restored cache, but adding --no-transfer-progress to this invocation removes the risk cheaply.

Observations (non-blocking, confirmed OK)

  • The hardcoded $HOME/.m2/repository/... path (script line 20) matches the maven-job action's repo location (~/.m2/repository, action.yml:276/397), so the unused settings.localRepository stub in the test harness (core-unit-reuse-benchmark.test.sh:28) is harmless dead code.
  • The -tests JAR the reuse path requires is produced: dotCMS/pom.xml:1840-1846 binds maven-jar-plugin:test-jar to package, and the maven-repo artifact is staged (action.yml:599) before the SNAPSHOT-cleanup step (action.yml:720), so the SNAPSHOT jars survive into the artifact even though they're purged from the cache.
  • The zip-entry path-traversal / zip-bomb-ish validation in the Python unpacker (script lines 36-40: absolute, .., backslash, and testzip() checks) is a good touch and is exercised by the corrupt/missing-JAR fixtures.

Overall the TDD fixtures are thorough and the "fail, never fall back to compilation" stance is correct for an experiment. The compiler-regex bug is the one that matters: fix it or the results job can never report the speedup it's designed to measure.

· issue-38326-core-unit-reuse-experiment

Comment on lines +50 to +51
# initialize supplies the Byte Buddy agent path without running compilation.
./mvnw "${args[@]}" initialize surefire:test 2>&1 | tee benchmark-maven.log

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Medium severity blocking issue identified in your code:
Relative path used for script invocation in hook. Use absolute paths or environment variables like $CLAUDE_PROJECT_DIR, $CURSOR_PROJECT_DIR, or $HOME to ensure the correct script is executed regardless of working directory.

Why this might be safe to ignore:

This is an intentional invocation of the repository’s Maven wrapper from a benchmark/build script, not a hook executing an attacker-controlled relative script. The hook-specific rule appears overly broad for this context, and the surrounding code indicates a controlled project working directory.

To resolve this comment:

🔧 No guidance has been designated for this issue. Fix according to your organization's approved methods.

💬 Ignore this finding

Reply with Semgrep commands to ignore this finding.

  • /fp <comment> for false positive
  • /ar <comment> for acceptable risk
  • /other <comment> for all other reasons

Alternatively, triage in Semgrep AppSec Platform to ignore the finding created by hooks-relative-script-path-bash.

If this is a critical or high severity finding, please also link this issue in the #security channel in Slack.

You can view more details about this finding in the Semgrep AppSec Platform.

This branch has not been deployed

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

Labels

Area : CI/CD PR changes GitHub Actions/workflows

1 participant