fix(ci): PR diff coverage uses base branch SHAs and aggregate 80% gate (port to 5.7-main)#2616
Conversation
#2608) Co-authored-by: AR Abdul Azeez <abdul@onesignal.com>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| print("\n ✓ All checked files have no executable lines (or fully covered)") | ||
| sys.exit(0) | ||
| else: | ||
| if eligible_paths: | ||
| msg = ( | ||
| "Changed Kotlin/Java sources under `src/main` could not be matched to the merged JaCoCo report " | ||
| "(or the diff has no executable touched lines). Check module wiring and that unit tests run for those sources." | ||
| ) | ||
| if generate_markdown: | ||
| markdown_output.append(f"\n### ❌ No usable coverage for changed sources\n\n{msg}\n") | ||
| with open(markdown_report, 'w') as f: | ||
| f.write('\n'.join(markdown_output)) | ||
| print('\n'.join(markdown_output)) | ||
| else: | ||
| print(f"\n ✗ {msg}") | ||
| sys.exit(1) | ||
| if generate_markdown: | ||
| markdown_output.append(f"\n### ⚠️ No Coverage Data\n") | ||
| markdown_output.append("No coverage data found for changed files\n") | ||
| markdown_output.append(f"\n### ✅ No main-source changes to gate\n") | ||
| markdown_output.append("No `src/main` Kotlin/Java changes in the diff.\n") | ||
| with open(markdown_report, 'w') as f: | ||
| f.write('\n'.join(markdown_output)) | ||
| else: | ||
| print("\n ⚠ No coverage data found for changed files") | ||
| print(" This may mean files aren't being compiled or tested") | ||
| print("\n ✓ No main-source Kotlin/Java changes in the diff") | ||
| sys.exit(0) | ||
| PYEOF | ||
|
|
||
| CHECK_RESULT=$? | ||
| if [ $CHECK_RESULT -eq 1 ]; then |
There was a problem hiding this comment.
🔴 The new eligible_paths module-wiring check (exit 1 when src/main sources can't be matched to JaCoCo) is bypassed whenever at least one changed src/main file is found in JaCoCo with zero executable lines. In a mixed PR where FileA is in JaCoCo with 0 executable lines and FileB is absent from JaCoCo entirely, the elif files_checked branch fires and exits 0 with 'All checked files have no executable lines', silently ignoring FileB's missing JaCoCo entry. The eligible_paths failure path introduced by this PR is therefore unreachable in this scenario, defeating the module-wiring detection it was designed to provide.
Extended reasoning...
What the bug is
The PR introduces a new failure mode: when src/main Kotlin/Java files are changed but none appear in the JaCoCo report, the script exits 1 with a clear error about module wiring. This is implemented via two lists: eligible_paths (all src/main changed files, populated before JaCoCo lookup) and files_checked (only files that ARE found in JaCoCo). The branching after the loop is: (1) if total_lines > 0 → aggregate gate; (2) elif files_checked: → exit 0 'All checked files have no executable lines'; (3) else: → check eligible_paths, possibly exit 1.
The specific code path that triggers it
In the scenario where FileA is found in JaCoCo with file_total == 0 (0 executable lines, e.g. a pure interface or annotation class), it is added to files_checked but does NOT increment total_lines. If FileB is a src/main Kotlin file NOT found in JaCoCo at all, it is added to eligible_paths but not files_checked. After the loop: total_lines = 0, files_checked = ['FileA'] (non-empty), eligible_paths = ['FileA', 'FileB']. The elif files_checked: branch fires and exits 0.
Why existing code doesn't prevent it
The elif files_checked branch does not inspect eligible_paths at all — it assumes that if any file was found in JaCoCo, the exit-0 message is correct. But eligible_paths may contain additional files that were never found. The branch message 'All checked files have no executable lines' is also misleading: FileB was never checked, it simply doesn't exist in the report.
What the impact would be
A module wiring issue — a src/main Kotlin source compiled into a module not included in the JaCoCo merge — will go undetected in CI. The build exits 0, the PR appears to pass coverage, and the structural problem (missing module from test run) is only visible as a per-file 'eligible_paths failure mode is defeated whenever at least one other changed src/main file happens to be in JaCoCo with 0 executable lines.
Addressing the refutation
The refutation argues that 'none map to the merged JaCoCo report' is intentional — the feature is for the all-or-nothing case, and partial mismatches still show a per-file warning. However: (1) the per-file warning does not cause a non-zero exit code, so CI does not fail; (2) the PR description does not state this all-or-nothing limitation — it says 'Failing with a clear message when src/main Kotlin/Java files change in the diff but none map to the merged JaCoCo report', which could reasonably apply to the case where only some are missing; (3) the design goal (catching module wiring failures) is not served by a warning that exits 0. The refutation's logic would justify removing the eligible_paths check entirely, since a missing file always produces a per-file warning regardless.
Step-by-step proof
- PR changes two src/main files:
com/onesignal/Foo.kt(a Kotlin interface with no body statements) andcom/onesignal/Bar.kt(new class in a mis-wired module). - Loop iteration for Foo.kt: found in JaCoCo with
file_total = 0→files_checked.append('Foo.kt'),total_linesstays 0. - Loop iteration for Bar.kt: not found in JaCoCo → prints '
⚠️ Bar.kt: Not in coverage report' → nothing added tofiles_checked. - After loop:
total_lines = 0,files_checked = ['Foo.kt'],eligible_paths = ['Foo.kt', 'Bar.kt']. - Branching:
total_lines > 0→ False;elif files_checked:→ True → exits 0 with 'All checked files have no executable lines'. - The
else:branch containing theeligible_pathscheck is never reached. Bar.kt's module wiring problem goes undetected.
How to fix it
In the elif files_checked: branch, also check whether eligible_paths contains files not in files_checked. If so, report those as missing and exit 1. For example: unmatched = [p for p in eligible_paths if os.path.basename(p) not in files_checked], and if unmatched is non-empty, emit the same module-wiring error message and exit 1.
| if diff_range: | ||
| result = subprocess.run( | ||
| ['git', 'diff', '--cached', '--unified=0', '--', file_path], | ||
| ['git', 'diff', '--unified=0', diff_range, '--', file_path], | ||
| capture_output=True, | ||
| text=True, | ||
| cwd=project_root | ||
| ) | ||
|
|
||
| # If no staged changes, check unstaged changes | ||
| if result.returncode != 0 or not result.stdout.strip(): | ||
| result = subprocess.run( | ||
| ['git', 'diff', '--unified=0', '--', file_path], | ||
| capture_output=True, | ||
| text=True, | ||
| cwd=project_root | ||
| ) | ||
|
|
||
| # If still nothing, try alternative base branch format | ||
| if result.returncode != 0 or not result.stdout.strip(): | ||
| if result.returncode != 0 or not result.stdout.strip(): | ||
| return None | ||
| else: | ||
| result = subprocess.run( | ||
| ['git', 'diff', '--unified=0', base_branch, 'HEAD', '--', file_path], | ||
| ['git', 'diff', '--unified=0', base_branch + '...HEAD', '--', file_path], | ||
| capture_output=True, | ||
| text=True, | ||
| cwd=project_root | ||
| ) | ||
|
|
||
| if result.returncode != 0 or not result.stdout.strip(): | ||
| return None | ||
| if result.returncode != 0 or not result.stdout.strip(): | ||
| result = subprocess.run( | ||
| ['git', 'diff', '--cached', '--unified=0', '--', file_path], | ||
| capture_output=True, | ||
| text=True, | ||
| cwd=project_root | ||
| ) | ||
| if result.returncode != 0 or not result.stdout.strip(): | ||
| result = subprocess.run( | ||
| ['git', 'diff', '--unified=0', '--', file_path], |
There was a problem hiding this comment.
🔴 The else branch in get_changed_lines() (lines 149–178) that handles staged/unstaged fallbacks is dead code and can never execute, causing a regression in the local developer workflow. Because the shell always exports DIFF_RANGE as a non-empty string (either the SHA-pair or ${BASE_BRANCH}...HEAD), Python's if diff_range: is always true — so a developer with staged-but-not-committed Kotlin/Java files will have those files in CHANGED_FILES but get_changed_lines() will call git diff --unified=0 origin/main...HEAD -- <staged_file>, return nothing, and fall back to checking all executable lines instead of only the changed ones, producing false failures or false passes.
Extended reasoning...
The bug: unreachable else branch
In get_changed_lines(), the new code uses if diff_range: … else: … where the else branch provides the staged/unstaged fallback chain that existed pre-PR. The problem is that diff_range is always non-empty: the shell always assigns DIFF_RANGE before exporting it — either ${DIFF_BASE_SHA}...${DIFF_HEAD_SHA} (lines 77–78, when both SHAs are set) or ${BASE_BRANCH}...HEAD (lines 80–81, the local/workflow_dispatch path, where BASE_BRANCH defaults to origin/main). There is no code path that leaves DIFF_RANGE empty before export DIFF_RANGE.
Why the guard doesn't help
Python reads it as diff_range = os.environ.get('DIFF_RANGE', '').strip() (around line 135). Since the environment variable is always a non-empty string like origin/main...HEAD, diff_range is always truthy. The else block — which tries git diff --cached (staged) and then git diff (unstaged) as progressive fallbacks — can never execute.
Concrete impact for local use (documented in the script header)
Step through the local case where a developer has staged-but-not-committed Kotlin/Java changes:
- Shell runs the non-SHA branch:
DIFF_RANGE="origin/main...HEAD";STAGED_FILESis populated viagit diff --cached --name-only;CHANGED_FILEStherefore includes the staged files. - Python receives
DIFF_RANGE="origin/main...HEAD"— truthy — so it always takes theif diff_range:path and callsgit diff --unified=0 origin/main...HEAD -- <staged_file>. - For a file that is only staged (not yet committed), that diff returns nothing. Python returns
Noneforchanged_lines. - The caller falls back to checking all executable lines in the file instead of only the changed ones — producing a false failure (low overall coverage even though the small changed portion is covered) or a false pass (high overall coverage even though the new lines are not).
Pre-PR, the code tried git diff --cached next, which would correctly return the staged diff and pinpoint exactly the changed lines.
Fix
Either (a) leave DIFF_RANGE unset/empty in the non-SHA shell path and continue to rely on the Python fallback chain, or (b) detect the local case in Python as diff_range == base_branch + '...HEAD' and add the staged/unstaged fallback after the primary git diff call returns empty, or (c) do the staged/unstaged fallbacks inside the if diff_range: block when the primary diff returns nothing.
2 participants
Description
One Line Summary
Port of #2608 to
5.7-mainso PR diff coverage measures exactly the PR patch against the chosen base branch (notmain) and enforces an aggregate 80% gate on touched executable lines.Details
Motivation
PRs targeting
5.7-main(e.g. #2614) are currently flagged by the[Diff Coverage] Check coverageCI step for files that were not changed in the PR. Root cause: on5.7-mainthe coverage script diffs againstorigin/mainby default, so every file that differs betweenmainand5.7-mainis treated as "changed" and run through the 80% gate.#2608 fixed this on
mainby:base.sha/head.shainto the script so the diff matches exactly what GitHub shows as the PR patch, regardless of the chosen base branch.src/mainKotlin/Java files change in the diff but none map to the merged JaCoCo report.This PR cherry-picks that commit (
5f7187bae) onto5.7-main.Scope
.github/workflows/ci.yml— step rename + pass PR base/head SHAs viaDIFF_BASE_SHA/DIFF_HEAD_SHA.OneSignalSDK/coverage/checkCoverage.sh— SHA-based diff range, aggregate gate, unmapped-source failure mode, updated report copy.Notes on the port
OneSignalSDK/coverage/checkCoverage.shon5.7-mainwas identical to pre-fix(ci): PR diff coverage uses base branch SHAs and aggregate 80% gate #2608main, so it applied cleanly..github/workflows/ci.ymlhad a trivial conflict on the step name (5.7-mainstill had the old[Diff Coverage] Check coveragelabel); took the incoming rename and newenv/ifblock from fix(ci): PR diff coverage uses base branch SHAs and aggregate 80% gate #2608.if: github.event_name == 'pull_request' || github.event_name == 'workflow_dispatch'condition is kept verbatim from fix(ci): PR diff coverage uses base branch SHAs and aggregate 80% gate #2608 even though5.7-mainCI only triggers onpull_requesttoday — it's a harmless no-op and keeps this port a clean 1:1 with the upstream commit.Testing
Unit testing
N/A — CI-only change.
Manual testing
bash -n OneSignalSDK/coverage/checkCoverage.shpasses.5.7-mainbase.Affected code checklist
Checklist
Overview
Testing
Final pass