Skip to content

fix(ci): PR diff coverage uses base branch SHAs and aggregate 80% gate (port to 5.7-main) #2616

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
sherwinski merged 1 commit into from
Apr 16, 2026
Merged

fix(ci): PR diff coverage uses base branch SHAs and aggregate 80% gate (port to 5.7-main) #2616

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,8 +53,13 @@ jobs:
else
echo "bypass=false" >> $GITHUB_OUTPUT
fi
- name: "[Diff Coverage] Check coverage"
- name: "[PR] Coverage on changed lines (min ${{ env.DIFF_COVERAGE_THRESHOLD }}%)"
if: github.event_name == 'pull_request' || github.event_name == 'workflow_dispatch'
working-directory: OneSignalSDK
env:
# Exact PR diff (vs merge ref / wrong base branch)
DIFF_BASE_SHA: ${{ github.event_name == 'pull_request' && github.event.pull_request.base.sha || '' }}
DIFF_HEAD_SHA: ${{ github.event_name == 'pull_request' && github.event.pull_request.head.sha || '' }}
run: |
# Use the shared coverage check script for consistency
# Generate markdown report for PR comments
Expand Down
181 changes: 109 additions & 72 deletions OneSignalSDK/coverage/checkCoverage.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,9 @@
# Configuration
COVERAGE_THRESHOLD=${DIFF_COVERAGE_THRESHOLD:-80}
BASE_BRANCH=${BASE_BRANCH:-origin/main}
# When set (e.g. GitHub PR), diff against these SHAs so we measure exactly the PR patch, not the merge ref
DIFF_BASE_SHA=${DIFF_BASE_SHA:-}
DIFF_HEAD_SHA=${DIFF_HEAD_SHA:-}
GENERATE_MARKDOWN=${GENERATE_MARKDOWN:-false} # Set to 'true' for CI/CD to generate markdown report
SKIP_COVERAGE_CHECK=${SKIP_COVERAGE_CHECK:-false} # Set to 'true' to bypass coverage check (still runs but doesn't fail)

Expand Down Expand Up @@ -71,15 +74,27 @@
echo -e "${GREEN}✓ Coverage report generated${NC}\n"

# Step 2: Check diff coverage using manual method (reliable path matching)
echo -e "${YELLOW}[2/3] Checking diff coverage against $BASE_BRANCH...${NC}"
echo -e "${YELLOW}Threshold: ${COVERAGE_THRESHOLD}%${NC}\n"
if [ -n "$DIFF_BASE_SHA" ] && [ -n "$DIFF_HEAD_SHA" ]; then
DIFF_RANGE="${DIFF_BASE_SHA}...${DIFF_HEAD_SHA}"
echo -e "${YELLOW}[2/3] Checking diff coverage for PR range ${DIFF_BASE_SHA:0:7}...${DIFF_HEAD_SHA:0:7}${NC}"
else
DIFF_RANGE="${BASE_BRANCH}...HEAD"
echo -e "${YELLOW}[2/3] Checking diff coverage against ${BASE_BRANCH}...HEAD${NC}"
fi
echo -e "${YELLOW}Threshold: ${COVERAGE_THRESHOLD}% (aggregate on changed executable lines)${NC}\n"

# Get changed files (run from project root)
# Include committed changes, staged changes, and unstaged changes
cd "$REPO_ROOT"
COMMITTED_FILES=$(git diff --name-only "$BASE_BRANCH"...HEAD 2>/dev/null | grep -E '\.(kt|java)$' || true)
STAGED_FILES=$(git diff --cached --name-only 2>/dev/null | grep -E '\.(kt|java)$' || true)
UNSTAGED_FILES=$(git diff --name-only 2>/dev/null | grep -E '\.(kt|java)$' || true)
if [ -n "$DIFF_BASE_SHA" ] && [ -n "$DIFF_HEAD_SHA" ]; then
COMMITTED_FILES=$(git diff --name-only "$DIFF_RANGE" 2>/dev/null | grep -E '\.(kt|java)$' || true)
STAGED_FILES=""
UNSTAGED_FILES=""
else
# Local / workflow_dispatch: committed vs base, plus working tree
COMMITTED_FILES=$(git diff --name-only "$BASE_BRANCH"...HEAD 2>/dev/null | grep -E '\.(kt|java)$' || true)
STAGED_FILES=$(git diff --cached --name-only 2>/dev/null | grep -E '\.(kt|java)$' || true)
UNSTAGED_FILES=$(git diff --name-only 2>/dev/null | grep -E '\.(kt|java)$' || true)
fi
# Combine all, remove duplicates, and filter to OneSignalSDK files
CHANGED_FILES=$(echo -e "$COMMITTED_FILES\n$STAGED_FILES\n$UNSTAGED_FILES" | grep -E '^OneSignalSDK/' | sort -u || true)

Expand All @@ -102,6 +117,7 @@
export MARKDOWN_REPORT
export BASE_BRANCH
export REPO_ROOT
export DIFF_RANGE
python3 << PYEOF
import xml.etree.ElementTree as ET
import re
Expand All @@ -116,47 +132,50 @@
markdown_report = os.environ.get('MARKDOWN_REPORT', 'diff_coverage.md')
base_branch = os.environ.get('BASE_BRANCH', 'origin/main')
repo_root_env = os.environ.get('REPO_ROOT')
diff_range = os.environ.get('DIFF_RANGE', '').strip()

def get_changed_lines(file_path, project_root):
"""Get line numbers of added/modified lines from git diff"""
try:
# First try to get diff from committed changes
result = subprocess.run(
['git', 'diff', '--unified=0', base_branch + '...HEAD', '--', file_path],
capture_output=True,
text=True,
cwd=project_root
)

# If no committed changes, check staged changes
if result.returncode != 0 or not result.stdout.strip():
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],

Check failure on line 165 in OneSignalSDK/coverage/checkCoverage.sh

View check run for this annotation

Claude / Claude Code Review

Staged/unstaged fallback in get_changed_lines() is unreachable dead code 

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 -
Comment on lines +140 to +165
Copy link
Copy Markdown

@claude claude Bot Apr 16, 2026

Choose a reason for hiding this comment

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

🔴 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:

  1. Shell runs the non-SHA branch: DIFF_RANGE="origin/main...HEAD"; STAGED_FILES is populated via git diff --cached --name-only; CHANGED_FILES therefore includes the staged files.
  2. Python receives DIFF_RANGE="origin/main...HEAD" — truthy — so it always takes the if diff_range: path and calls git diff --unified=0 origin/main...HEAD -- <staged_file>.
  3. For a file that is only staged (not yet committed), that diff returns nothing. Python returns None for changed_lines.
  4. 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.

capture_output=True,
text=True,
cwd=project_root
)
if result.returncode != 0 or not result.stdout.strip():
result = subprocess.run(
['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

changed_lines = set()
current_new_line = None
Expand Down Expand Up @@ -217,11 +236,15 @@
total_lines = 0
files_below_threshold = []
files_checked = []
eligible_paths = []
markdown_output = []

if generate_markdown:
markdown_output.append("## Diff Coverage Report (Changed Lines Only)\n")
markdown_output.append(f"**Threshold:** {threshold}%\n\n")
markdown_output.append(
f"**Gate:** aggregate coverage on **changed executable lines** must be ≥ **{threshold}%** "
"(JaCoCo line data for lines touched in the diff).\n\n"
)
markdown_output.append("### Changed Files Coverage\n\n")

for changed_file in changed_files:
Expand All @@ -237,7 +260,9 @@
match = re.search(r'src/main/(java|kotlin)/(.+)/([^/]+\.(kt|java))$', path_part)
if not match:
continue


eligible_paths.append(changed_file)

package_path = match.group(2)
filename = match.group(3)
package_name = package_path.replace('/', '/')
Expand Down Expand Up @@ -273,18 +298,14 @@
coverage_pct = (file_covered / file_total * 100) if file_total > 0 else 100

if generate_markdown:
status = "✅" if coverage_pct >= threshold else "❌"
changed_info = f" ({len(changed_lines)} changed lines)" if changed_lines else " (all lines - could not determine changed lines)"
markdown_output.append(f"- {status} **{filename}**: {file_covered}/{file_total} changed lines ({coverage_pct:.1f}%){changed_info}")
changed_info = f" ({len(changed_lines)} touched lines in diff)" if changed_lines else " (all lines — could not resolve diff)"
markdown_output.append(f"- **{filename}**: {file_covered}/{file_total} touched executable lines ({coverage_pct:.1f}%){changed_info}")
if coverage_pct < threshold:
files_below_threshold.append((filename, coverage_pct, file_uncovered))
markdown_output.append(f" - ⚠️ Below threshold: {file_uncovered} uncovered changed lines")
markdown_output.append(f" - {file_uncovered} uncovered touched lines in this file")
else:
status = "✓" if coverage_pct >= threshold else "✗"
color = "" if coverage_pct >= threshold else "\033[0;31m"
reset = "\033[0m" if color else ""
changed_info = f" ({len(changed_lines)} changed lines)" if changed_lines else " (all lines - could not determine changed lines)"
print(f" {color}{status}{reset} {filename}: {file_covered}/{file_total} changed lines ({coverage_pct:.1f}%){changed_info}")
changed_info = f" ({len(changed_lines)} touched lines)" if changed_lines else " (all lines — could not resolve diff)"
print(f" {filename}: {file_covered}/{file_total} touched executable lines ({coverage_pct:.1f}%){changed_info}")
if coverage_pct < threshold:
files_below_threshold.append((filename, coverage_pct, file_uncovered))
break
Expand All @@ -299,39 +320,43 @@

if total_lines > 0:
overall_coverage = ((total_lines - total_uncovered) / total_lines * 100)

overall_pass = overall_coverage >= threshold

if generate_markdown:
markdown_output.append(f"\n### Overall Coverage (Changed Lines Only)\n")
markdown_output.append(f"**{total_lines - total_uncovered}/{total_lines}** changed lines covered ({overall_coverage:.1f}%)\n")

markdown_output.append(f"\n### Overall (aggregate gate)\n")
markdown_output.append(
f"**{total_lines - total_uncovered}/{total_lines}** touched executable lines covered "
f"({overall_coverage:.1f}% — requires ≥ {threshold}%)\n"
)
if files_below_threshold:
markdown_output.append(f"\n### ❌ Coverage Check Failed\n")
markdown_output.append(f"Files below {threshold}% threshold:\n")
markdown_output.append(f"\n**Per-file detail** (informational; gate is aggregate above):\n")
for filename, pct, uncovered in files_below_threshold:
markdown_output.append(f"- **{filename}**: {pct:.1f}% ({uncovered} uncovered changed lines)\n")

markdown_output.append(f"- **{filename}**: {pct:.1f}% ({uncovered} uncovered touched lines)\n")

if not overall_pass:
markdown_output.append(f"\n### ❌ Coverage Check Failed\n")
markdown_output.append(
f"Aggregate coverage on touched lines is **{overall_coverage:.1f}%** (minimum **{threshold}%**).\n"
)

# Write markdown file
with open(markdown_report, 'w') as f:
f.write('\n'.join(markdown_output))

# Print to console
print('\n'.join(markdown_output))

if files_below_threshold:
sys.exit(1)
else:
sys.exit(0)

sys.exit(0 if overall_pass else 1)
else:
print(f"\n Overall: {(total_lines - total_uncovered)}/{total_lines} changed lines covered ({overall_coverage:.1f}%)")

print(f"\n Overall: {(total_lines - total_uncovered)}/{total_lines} touched executable lines covered ({overall_coverage:.1f}%)")
print(f" Gate: ≥ {threshold}% aggregate on touched lines — {'PASS' if overall_pass else 'FAIL'}")

if files_below_threshold:
print(f"\n Files below {threshold}% threshold:")
print(f"\n Per-file (informational):")
for filename, pct, uncovered in files_below_threshold:
print(f" • {filename}: {pct:.1f}% ({uncovered} uncovered changed lines)")
sys.exit(1)
else:
print(f"\n ✓ All files meet {threshold}% threshold for changed lines")
sys.exit(0)
print(f" • {filename}: {pct:.1f}% ({uncovered} uncovered)")

sys.exit(0 if overall_pass else 1)
elif files_checked:
# Files were found but had no executable lines
if generate_markdown:
Expand All @@ -340,28 +365,40 @@
with open(markdown_report, 'w') as f:
f.write('\n'.join(markdown_output))
else:
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

Check failure on line 395 in OneSignalSDK/coverage/checkCoverage.sh

View check run for this annotation

Claude / Claude Code Review

eligible_paths module-wiring check bypassed when any changed file has 0 executable lines 

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 introduc
Comment on lines 368 to 395
Copy link
Copy Markdown

@claude claude Bot Apr 16, 2026

Choose a reason for hiding this comment

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

🔴 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 '⚠️ Not in coverage report' warning that reviewers may overlook. The entire purpose of the new 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

  1. PR changes two src/main files: com/onesignal/Foo.kt (a Kotlin interface with no body statements) and com/onesignal/Bar.kt (new class in a mis-wired module).
  2. Loop iteration for Foo.kt: found in JaCoCo with file_total = 0 → files_checked.append('Foo.kt'), total_lines stays 0.
  3. Loop iteration for Bar.kt: not found in JaCoCo → prints '⚠️ Bar.kt: Not in coverage report' → nothing added to files_checked.
  4. After loop: total_lines = 0, files_checked = ['Foo.kt'], eligible_paths = ['Foo.kt', 'Bar.kt'].
  5. Branching: total_lines > 0 → False; elif files_checked: → True → exits 0 with 'All checked files have no executable lines'.
  6. The else: branch containing the eligible_paths check 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 [ "$GENERATE_MARKDOWN" != "true" ]; then
if [ -n "$BYPASS_REASON" ]; then
echo -e "\n${YELLOW}⚠ Coverage below threshold (files below ${COVERAGE_THRESHOLD}%)${NC}"
echo -e "\n${YELLOW}⚠ Aggregate touched-line coverage below ${COVERAGE_THRESHOLD}%${NC}"
echo -e "${YELLOW} Build will not fail due to bypass: $BYPASS_REASON${NC}\n"
else
echo -e "\n${RED}✗ Coverage check failed (files below ${COVERAGE_THRESHOLD}% threshold)${NC}\n"
echo -e "\n${RED}✗ Coverage check failed (aggregate on touched lines < ${COVERAGE_THRESHOLD}%)${NC}\n"
fi
else
# In markdown mode, update the report to indicate bypass if applicable
Expand Down
Loading