gnodet-bot commented on code in PR #26534:
URL: https://github.com/apache/camel/pull/26534#discussion_r4034449828
##########
.github/actions/incremental-build/incremental-build.sh:
##########
@@ -526,29 +547,24 @@ writeScalpelComparison() {
echo "" >> "$comment_file"
fi
- # Show Scalpel-detected change details
- if [ -n "$scalpel_props" ]; then
- echo "Changed properties: ${scalpel_props}" >> "$comment_file"
- echo "" >> "$comment_file"
- fi
- if [ -n "$scalpel_managed_deps" ]; then
- echo "Changed managed dependencies: ${scalpel_managed_deps}" >>
"$comment_file"
- echo "" >> "$comment_file"
- fi
- if [ -n "$scalpel_managed_plugins" ]; then
- echo "Changed managed plugins: ${scalpel_managed_plugins}" >>
"$comment_file"
- echo "" >> "$comment_file"
- fi
-
- echo "**Skip-tests mode would test ${scalpel_test_count} modules**
(${scalpel_direct_count} direct + ${scalpel_downstream_tested} downstream),
**skip tests for ${scalpel_skip_count}** (generated code, meta-modules)" >>
"$comment_file"
-
- # Show which modules Scalpel would test
+ # Show which modules Scalpel would test, with per-module evidence
(explain=true)
if [ -n "$scalpel_would_test" ]; then
echo "" >> "$comment_file"
- echo "<details><summary>Modules Scalpel would test
(${scalpel_test_count})</summary>" >> "$comment_file"
+ echo "<details><summary>Modules Scalpel would test
(${scalpel_tested_count})</summary>" >> "$comment_file"
echo "" >> "$comment_file"
+ local report="target/scalpel-report.json"
Review Comment:
⚠️ **Relative report path without CWD guarantee**
`local report="target/scalpel-report.json"` is a relative path. In
`runScalpelDetection` the same path works because Maven runs from the repo root
(CWD = workspace root in GHA). But here in `writeScalpelComparison`, this path
is re-declared without any `cd` — it relies on the inherited CWD from the
caller being the repo root.
In the current GHA setup that assumption holds, so it is not broken today.
But if anyone ever runs the script from a subdirectory, every evidence lookup
will silently fail (jq on a non-existent file exits non-zero, `|| true` masks
it, `evidence` stays empty). The `2>/dev/null` on the jq call compounds the
silence.
Consider using the same pattern as `runScalpelDetection` at line 289: either
pass the report path as a function argument, or resolve it relative to
`$SCRIPT_DIR` (already computed at line 33) so it is always absolute:
```suggestion
local report="${SCRIPT_DIR}/../../../../target/scalpel-report.json"
```
Or, simpler, promote it to a function parameter alongside `comment_file`.
##########
.github/actions/incremental-build/incremental-build.sh:
##########
@@ -490,20 +487,43 @@ writeScalpelComparison() {
only_current_count=$current_total
fi
- # One-line summary: what Scalpel would change
- local summary="Scalpel: ${scalpel_test_count} tested, ${scalpel_skip_count}
compile-only — current: ${current_total} all tested"
+ # N-of-M framing: use reactorModuleCount from report when available
+ local nm_suffix=""
+ if [ "$scalpel_reactor_count" -gt 0 ] 2>/dev/null; then
+ nm_suffix=" of ${scalpel_reactor_count}"
+ fi
+
+ # One-line summary with N-of-M framing
+ local summary="Scalpel: ${scalpel_tested_count}${nm_suffix} tested,
${scalpel_skip_count} compile-only — current: ${current_total} all tested"
echo "" >> "$comment_file"
echo "---" >> "$comment_file"
echo "" >> "$comment_file"
echo "<details><summary>:microscope: Scalpel shadow comparison —
${summary}</summary>" >> "$comment_file"
echo "" >> "$comment_file"
- echo "[Maveniverse Scalpel](https://github.com/maveniverse/scalpel) detected
**${scalpel_total} affected modules** (current approach: ${current_total})." >>
"$comment_file"
+ echo "[Maveniverse Scalpel](https://github.com/maveniverse/scalpel) detected
**${scalpel_total}${nm_suffix} affected modules** (current approach:
${current_total})." >> "$comment_file"
echo "" >> "$comment_file"
- # Show modules only Scalpel found (not in current reactor)
+ # Show Scalpel-detected change details
Review Comment:
💡 **N-of-M suffix reused across two different numerators**
`nm_suffix` (` of ${scalpel_reactor_count}`) is used for two different
sentences:
1. `${scalpel_tested_count}${nm_suffix} tested` — tested modules out of
reactor total ✓
2. `${scalpel_total}${nm_suffix} affected modules` — *all* affected (tested
+ compile-only) out of reactor total
Both are technically correct (same denominator applies to both), but a
reviewer reading the comment will see e.g. `85 of 1847 affected` and `47 of
1847 tested` and may wonder why the two numerators differ. The first sentence
is in the details `<summary>` tag, the second is in the body — they share the
`of 1847` without explaining that `scalpel_total` ≠ `scalpel_tested_count`.
Consider either:
- Adding a brief parenthetical: `(${scalpel_total} total,
${scalpel_tested_count}${nm_suffix} tested)`, or
- Reserving `nm_suffix` only for the tested count and using a plain number
for `scalpel_total`.
--
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]
