Skip to content

fix(ci): PR diff coverage uses base branch SHAs and aggregate 80% gate #2608

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
abdulraqeeb33 merged 1 commit into from
Apr 13, 2026
Merged

fix(ci): PR diff coverage uses base branch SHAs and aggregate 80% gate #2608

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
6 changes: 5 additions & 1 deletion .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,9 +63,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 || '' }}
Comment on lines +66 to +72
Copy link

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

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

In PR mode, the script now relies on git diff $DIFF_BASE_SHA...$DIFF_HEAD_SHA to discover changed files. With the default actions/checkout settings (shallow clone), those SHAs may not exist locally; git diff will fail and COMMITTED_FILES becomes empty, causing the job to incorrectly report “No Kotlin/Java files changed” and skip the coverage gate. To avoid silent false-passes, either fetch sufficient history/objects in the workflow checkout (e.g., fetch-depth: 0 or otherwise fetch both SHAs), and/or make the script explicitly fail when git diff $DIFF_RANGE errors in the SHA-based mode.

Copilot uses AI. Check for mistakes.
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

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

View check run for this annotation

Claude / Claude Code Review

Staged/unstaged fallback dead code in get_changed_lines for local/workflow_dispatch runs 

The staged/unstaged fallback in get_changed_lines is dead code for local runs because DIFF_RANGE is always exported as a non-empty string before the Python heredoc. For local development where CHANGED_FILES correctly includes staged/unstaged files, the function runs git diff origin/main...HEAD -- file, gets empty output for uncommitted files, and immediately returns None—silently falling back to measuring the entire JaCoCo file rather than just changed lines. This is a regression from the old co
Comment on lines +140 to +148
Copy link
Copy Markdown

@claude claude Bot Apr 9, 2026

Choose a reason for hiding this comment

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

🔴 The staged/unstaged fallback in get_changed_lines is dead code for local runs because DIFF_RANGE is always exported as a non-empty string before the Python heredoc. For local development where CHANGED_FILES correctly includes staged/unstaged files, the function runs git diff origin/main...HEAD -- file, gets empty output for uncommitted files, and immediately returns None—silently falling back to measuring the entire JaCoCo file rather than just changed lines. This is a regression from the old code that explicitly tried git diff --cached and git diff as fallbacks.

Extended reasoning...

Root cause: In the bash script, DIFF_RANGE is unconditionally set to a non-empty string before it is exported: origin/main...HEAD (local/workflow_dispatch) or BASE_SHA...HEAD_SHA (PR). Python reads it via os.environ.get('DIFF_RANGE', '').strip(), which is therefore always truthy. The if diff_range: branch is always taken, and the else block with staged/cached fallbacks is unreachable dead code.

Code path that triggers it: In the local/workflow_dispatch case, the bash script correctly collects staged and unstaged Kotlin/Java files via git diff --cached --name-only and git diff --name-only, adds them to CHANGED_FILES, and exports DIFF_RANGE=origin/main...HEAD. Python then calls git diff --unified=0 origin/main...HEAD -- for each file. For a file with only staged or unstaged changes, this command returns empty output because the changes are not yet in git commit history. The function hits if result.returncode != 0 or not result.stdout.strip(): return None and exits immediately with no fallback.

Why existing code does not prevent it: The intent was that the else branch would handle the staged/unstaged case. But because DIFF_RANGE is exported unconditionally, Python never reaches the else branch. The regression was introduced when the PR changed the bash script to always export DIFF_RANGE while the Python logic still expects diff_range to be falsy for the local path.

Impact: When get_changed_lines returns None, the loop falls back to relevant_lines = lines (all JaCoCo lines for the file). Coverage is then measured against the entire files executable lines rather than just the changed lines. This silently inflates or deflates reported diff coverage for locally-modified or staged files. No error is emitted. Impact is limited to local development; GitHub PR runs are unaffected (clean checkout, SHA-range diff) and workflow_dispatch CI is also unaffected (clean checkout, no staged/unstaged files).

Step-by-step proof:

  1. Developer modifies OneSignalSDK/.../Foo.kt but only stages it (git add), no commit.
  2. Bash collects STAGED_FILES via git diff --cached --name-only, picks up Foo.kt.
  3. Bash sets DIFF_RANGE=origin/main...HEAD (non-empty) and exports it.
  4. Python reads diff_range = 'origin/main...HEAD' (truthy).
  5. Python calls git diff --unified=0 origin/main...HEAD -- .../Foo.kt — returns empty because Foo.kt has only staged changes, not committed ones.
  6. result.stdout.strip() is empty -> return None with no further fallback.
  7. Coverage check uses all JaCoCo lines for Foo.kt instead of just the staged-line numbers, producing wrong coverage numbers.

Fix: Either (a) do not export DIFF_RANGE for the local/workflow_dispatch path (leave it unset) so Python falls through to the else branch with its staged/cached fallbacks, or (b) within the if diff_range: block, add a second-chance fallback to git diff --cached and git diff when the three-dot range returns empty and the range is a branch name rather than a SHA pair.

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],
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 @@ -343,25 +368,37 @@
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
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