Skip to content

ci: add Scalpel shadow comparison and upgrade to 0.3.7#22524

Open
gnodet wants to merge 1 commit into
apache:mainfrom
gnodet:worktree-scalpel-skip-tests
Open

ci: add Scalpel shadow comparison and upgrade to 0.3.7#22524
gnodet wants to merge 1 commit into
apache:mainfrom
gnodet:worktree-scalpel-skip-tests

Conversation

@gnodet

@gnodet gnodet commented Apr 9, 2026

Copy link
Copy Markdown
Contributor

Summary

Add a Scalpel shadow comparison section to PR CI comments, showing what Maveniverse Scalpel's skip-tests mode would have tested — without affecting actual test execution.

This lets the team validate Scalpel's module detection across many PRs before switching from grep-based to Scalpel-driven test selection.

Changes

  • Shadow comparison in CI comment: collapsible section showing Scalpel's skip-tests module list (direct + downstream), with breakdown of tested vs skipped modules
  • Scalpel failure reporting: when Scalpel fails (shallow clone, build error), the reason is shown in the comment instead of silent fallback
  • skipTestsForDownstreamModules: derived from EXCLUSION_LIST via sed (no duplication) — tells Scalpel which downstream modules to skip in shadow mode
  • Scalpel upgraded to 0.3.7: fixes inflated affectedModules count for parent POM property changes (scalpel#39) and skipTestsForDownstreamModules not taking effect

How Scalpel skip-tests mode works

Scalpel distinguishes between modules that need recompilation and modules that need retesting:

  • All affected modules are recompiled in the Maven reactor to verify compilation doesn't break
  • Modules in the EXCLUSION_LIST (generated code, meta-modules like camel-allcomponents, camel-catalog, camel-endpointdsl…) are recompiled with -DskipTests — no test execution
  • All other affected modules are both recompiled and tested

For example, a junit-jupiter-version change affects 42 modules: 13 are recompiled and tested, 29 are recompiled only (tests skipped).

Regression safety

  • Grep-based detection unchanged: fetchDiff() still uses the GitHub REST API (no local git dependency for grep)
  • Scalpel uses local git: the CI workflow pre-fetches the base branch (git fetch --deepen=200) for Scalpel's JGit merge-base — guarded by !inputs.skip_full_build (same as Scalpel itself)
  • No behavioral change: Scalpel runs in shadow/report mode only — actual test execution is unchanged
  • Checkout actions preserved: checkout@v7.0.0 and persist-credentials: false kept in both pr-build-main.yml and sonar-build.yml

Validation

Tested with 3 clean PRs on Scalpel 0.3.7 — each bumps a different managed dependency property:

PR Property Grep found Scalpel affected Scalpel would test Scalpel skipped (recompile only) CI
#24272 kafka-version 0 4 2 2
#24273 junit-jupiter-version 1 42 13 29
#24274 jackson2-version 6 157 130 27
  • kafka: Scalpel finds 2 modules grep completely misses (kafka is consumed via <dependencyManagement>, invisible to grep)
  • junit: Scalpel finds 42 affected modules vs grep's 1. 29 downstream meta-modules are recompiled but not tested
  • jackson: Scalpel finds 157 affected modules vs grep's 6. Jackson is widely used via BOM import

CI architecture docs

Updated CI-ARCHITECTURE.md to document the dual detection strategy, shadow comparison behavior, and Scalpel configuration.

Claude Code on behalf of @gnodet

@github-actions

github-actions Bot commented Apr 9, 2026

Copy link
Copy Markdown
Contributor

🌟 Thank you for your contribution to the Apache Camel project! 🌟
🤖 CI automation will test this PR automatically.

🐫 Apache Camel Committers, please review the following items:

  • First-time contributors require MANUAL approval for the GitHub Actions to run
  • You can use the command /component-test (camel-)component-name1 (camel-)component-name2.. to request a test from the test bot although they are normally detected and executed by CI.
  • You can label PRs using skip-tests and test-dependents to fine-tune the checks executed by this PR.
  • Build and test logs are available in the summary page. Only Apache Camel committers have access to the summary.

⚠️ Be careful when sharing logs. Review their contents before sharing them publicly.

@github-actions

github-actions Bot commented Apr 9, 2026

Copy link
Copy Markdown
Contributor

ℹ️ CI did not run targeted module tests.


⚙️ View full build and test results

@gnodet gnodet marked this pull request as draft April 9, 2026 23:37
@gnodet gnodet force-pushed the worktree-scalpel-skip-tests branch from 77a5579 to 1834e34 Compare April 10, 2026 10:16
@gnodet gnodet changed the title ci: use Scalpel skip-tests mode for single-invocation CI builds ci: add Scalpel 0.3.0 shadow comparison alongside grep-based detection Apr 10, 2026

@apupier apupier left a comment

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.

requires to resolve conflict.

Does this PR mean that we should not update the maven extension without other changes? #22572

@gnodet gnodet changed the title ci: add Scalpel 0.3.0 shadow comparison alongside grep-based detection ci: add Scalpel shadow comparison for skip-tests mode validation May 5, 2026
@gnodet gnodet requested review from apupier and davsclaus May 5, 2026 09:57
@gnodet gnodet marked this pull request as ready for review May 5, 2026 09:58
@gnodet

gnodet commented May 5, 2026

Copy link
Copy Markdown
Contributor Author

Claude Code on behalf of Guillaume Nodet

@apupier Thanks for the review!

requires to resolve conflict.

Done — just merged latest main and resolved the conflicts.

Does this PR mean that we should not update the maven extension without other changes? #22572

No, the Scalpel extension can be updated independently — that PR (#22572) was already merged and is included in the merge we just did. The CI script is version-agnostic: it uses mode=report and reads the JSON output with jq fallbacks, so new Scalpel versions adding fields won't break anything. This PR just adds a shadow comparison section to the PR comment showing what Scalpel's skip-tests mode would have tested — purely observational, no change to which tests actually run.

@davsclaus davsclaus requested review from Croway and orpiske May 5, 2026 10:32

@apupier apupier left a comment

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.

Please launch it on a branch so that we ensure that it doesn't break the curren tflow and have a better idea of what will be the output

Comment thread .github/CI-ARCHITECTURE.md Outdated

The script overrides `fullBuildTriggers` to empty (`-Dscalpel.fullBuildTriggers=`) because Scalpel's default (`.mvn/**`) would trigger a full build whenever `.mvn/extensions.xml` itself changes (e.g., Dependabot bumping Scalpel).

The base branch is pre-fetched by the CI workflow (`git fetch --deepen=200` + fetch of `origin/main`) rather than by Scalpel's built-in JGit fetch (`-Dscalpel.fetchBaseBranch=false`). This avoids JGit issues in shallow CI clones.

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.

why do we need 200 hundreds commits from git history to detect dependencies?

What is the performance impact?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The 200 commits are not for detecting dependencies — they're for finding the git merge-base between the PR branch and main.

GitHub Actions checks out with depth=1 (a single commit). Scalpel needs the merge-base to compute which files changed, then maps those changed files to Maven modules and walks Maven's dependency graph to find affected downstream modules.

Why local git instead of the GitHub API? The existing grep-based script fetches the diff via the GitHub REST API (application/vnd.github.v3.diff), so it doesn't need local history at all. Scalpel uses local git instead because:

  • It's a Maven extension (JVM-based), so using local git via JGit is the natural approach
  • CI-provider-agnostic — works on GitLab, Jenkins, etc., not just GitHub
  • No diff truncation risk (GitHub API truncates large diffs)

Performance impact: git fetch --deepen=200 fetches only commit metadata (not file blobs). It adds ~2-3 seconds to the job — negligible compared to the Maven build.

I've updated CI-ARCHITECTURE.md to clarify this.

Claude Code on behalf of Guillaume Nodet

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Also note that the end goal is to switch to Scalpel completely and remove the grep-based custom mechanism, which will then remove the dependency on the GitHub API for diff fetching entirely.

Claude Code on behalf of Guillaume Nodet

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I've pushed a follow-up commit that switches the grep-based script itself to use local git (git merge-base + git diff) instead of the GitHub API for diff fetching. Both mechanisms now share the same --deepen=200 fetch step, which makes the git fetch justified by both and removes the GitHub API dependency for diffs entirely.

Claude Code on behalf of Guillaume Nodet

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Updated test PRs — now with POM changes to trigger Scalpel's shadow comparison (the previous ones only had Java file changes, which don't invoke Scalpel's POM analysis):

  • PR #23062kafka-version (narrow managed dep). Grep finds 0 modules, Scalpel should find camel-kafka.
  • PR #23063junit-jupiter-version (test-scoped managed dep). Grep finds 0, Scalpel should detect test scope.
  • PR #23064jackson2-version (widely-used managed dep). Grep finds 0, Scalpel should find many modules.

All three demonstrate grep's blind spot for <dependencyManagement>-inherited versions. Throwaway PRs — will be closed after validation.

Claude Code on behalf of Guillaume Nodet

@gnodet

gnodet commented May 7, 2026

Copy link
Copy Markdown
Contributor Author

Please launch it on a branch so that we ensure that it doesn't break the current flow and have a better idea of what will be the output

New validation PRs created with Scalpel 0.3.7 (clean CI history):

  • PR #24272kafka-version 4.3.1 → 4.3.0 (narrow managed dependency)
  • PR #24273junit-jupiter-version 5.14.4 → 5.14.3 (test-scoped managed dependency)
  • PR #24274jackson2-version 2.22.0 → 2.21.2 (widely-used managed dependency)

Previous test PRs (#23070#23072, #24250) used Scalpel 0.3.0–0.3.6 which had inflated module counts (scalpel#39). That's fixed in 0.3.7.

Preliminary result (from #24250 with 0.3.7):

PR Property Grep found Scalpel would test Scalpel skipped
#24250 kafka kafka-version 0 modules 2 (camel-kafka, camel-kafka-azure-rebalance-listener) 2 (camel-allcomponents, camel-endpointdsl)

CI runs for the new PRs are in progress — results will appear in the Scalpel shadow comparison section of each PR's CI comment.

Claude Code on behalf of @gnodet

@gnodet

gnodet commented Jun 26, 2026

Copy link
Copy Markdown
Contributor Author

Scalpel 0.3.7 validation results

Created 3 clean test PRs to validate the shadow comparison with different types of managed dependency changes:

PR Property Grep found Scalpel affected Scalpel would test Scalpel skipped (recompile only) CI
#24272 kafka-version 4.3.1→4.3.0 0 4 2 2
#24273 junit-jupiter-version 5.14.4→5.14.3 1 42 13 29
#24274 jackson2-version 2.22.0→2.21.2 6 157 130 27

Scalpel distinguishes between modules that need recompilation and those that need retesting:

  • All affected modules are recompiled in the reactor to verify compilation doesn't break
  • Modules in the EXCLUSION_LIST (generated code, meta-modules) are recompiled with -DskipTests
  • All other affected modules are both recompiled and tested

Key observations

  • Kafka — Scalpel finds 2 modules that grep completely misses. Kafka is consumed via <dependencyManagement>, so ${kafka-version} never appears in child POMs.
  • JUnit — Scalpel finds 42 affected modules vs grep's 1. 29 downstream meta-modules are recompiled but not tested.
  • Jackson — Scalpel finds 157 affected modules vs grep's 6. Jackson is widely used via BOM import — grep only catches the modules that explicitly write ${jackson2-version} in their POM.

All 3 test PRs passed CI ✅ with no regression to the existing grep-based test execution.

Claude Code on behalf of @gnodet

@gnodet gnodet changed the title ci: add Scalpel shadow comparison for skip-tests mode validation ci: add Scalpel shadow comparison and upgrade to 0.3.7 Jun 26, 2026
@gnodet gnodet marked this pull request as ready for review June 26, 2026 13:09
@gnodet gnodet requested a review from apupier June 26, 2026 13:09
@gnodet

gnodet commented Jun 26, 2026

Copy link
Copy Markdown
Contributor Author

Validation results (Scalpel 0.3.7)

Three test PRs were created to validate the shadow comparison across different dependency change scenarios:

Test PR Property changed Grep found Scalpel would test Scalpel would skip
#24250 kafka-version 45 modules 16 modules (3 direct + 13 downstream) 29 (generated code, meta-modules)
#24273 junit-jupiter-version 42 modules 13 modules (3 direct + 10 downstream) 29 (generated code, meta-modules)
#24272 kafka-version (narrow) CI pending CI pending CI pending
#24274 jackson2-version (wide) CI pending CI pending CI pending

Key observations

  • Scalpel correctly identifies direct consumers — for kafka-version, it finds camel-kafka + debezium modules + camel-ibm-secrets-manager (which depend on kafka-clients transitively)
  • Skip-tests mode filters out meta-modulescamel-allcomponents, camel-endpointdsl, camel-componentdsl, catalogs, docs, coverage are correctly skipped (generated/aggregation code, no real tests to run)
  • No behavioral change — this PR only adds shadow/reporting mode. Grep-based test selection is unchanged.
  • CI is green on the main PR ✅

Results for #24272 and #24274 will appear in their CI comments once builds complete.

See also: the last comment above with links to all validation PRs.

Add a shadow comparison section to CI PR comments showing what
Maveniverse Scalpel's skip-tests mode would have tested — without
affecting actual test execution.

Changes:
- incremental-build.sh: configure Scalpel with skipTestsForDownstreamModules
  and fetchBaseBranch=false, add writeScalpelComparison() for collapsible
  PR comment section with failure reporting
- pr-build-main.yml / sonar-build.yml: add base branch fetch step for
  Scalpel's merge-base detection in shallow CI clones, restore checkout v7
- CI-ARCHITECTURE.md: document shadow comparison approach and configuration
- Scalpel upgraded to 0.3.7: fixes inflated affectedModules count for
  parent POM property changes (scalpel#39) and skipTestsForDownstreamModules
@gnodet gnodet force-pushed the worktree-scalpel-skip-tests branch from d106d61 to 4585eeb Compare June 26, 2026 13:10
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.

2 participants