Fix/check diff coverage #518
Conversation
WalkthroughThe CI workflow splits test execution: standard tests on push, coverage on pull_request. The diff-coverage script is refactored to compute files to check from both lib changes and mapped tests, adds prerequisites and coverage file checks, updates logging, and replaces old change-detection logic. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as Developer
participant GH as GitHub Actions
participant Job as CI Job
Dev->>GH: Push
GH->>Job: Trigger workflow (event=push)
Job->>Job: Run Flutter tests
Note over Job: No coverage steps on push
Dev->>GH: Open/Update PR
GH->>Job: Trigger workflow (event=pull_request)
Job->>Job: Run Flutter tests with --coverage
Job->>Job: Install lcov
Job->>Job: Check coverage (lcov/info)
sequenceDiagram
autonumber
participant Script as check_diff_coverage.sh
participant Git as git
participant Lcov as lcov
Script->>Script: check_prerequisites()
Script->>Script: check_coverage_file_presence()
Script->>Git: Determine merge base
Script->>Script: get_files_to_check(merge_base, pattern)
Note over Script: Split changes: lib/*.dart and test/*.dart
Script->>Script: Map tests -> lib files
Script->>Script: Combine & de-duplicate file set
alt No files to check
Script-->>Script: Exit success early
else Files to check
Script->>Lcov: --extract lcov.info for files_to_check
Script-->>Script: calculate_coverage()
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
…e matches and when tests files are modified
josefinalliende
force-pushed
the
fix/check-diff-coverage
branch
from
ac51db9 to
c5abe67
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
scripts/check_diff_coverage.sh (4)
123-129: Scope locals to avoid leakage; also keep grep fallback robust.lib_diff_files, test_diff_files, and lib_files_from_test_diff_files are currently global in this function. Scope them as local to avoid side effects.
Apply this diff:
- local diff_files + local diff_files diff_files=$( - git diff --name-only "$merge_base" HEAD | grep "${file_pattern}" + git diff --name-only "$merge_base" HEAD | grep "${file_pattern}" || true ) - lib_diff_files=$(echo "$diff_files" | grep '^lib/.*\.dart$' || true) - test_diff_files=$(echo "$diff_files" | grep '^test/.*\.dart$' || true) - lib_files_from_test_diff_files=$(get_lib_files_from_test_files "$test_diff_files") - local files_to_check + local lib_diff_files test_diff_files lib_files_from_test_diff_files + lib_diff_files=$(echo "$diff_files" | grep '^lib/.*\.dart$' || true) + test_diff_files=$(echo "$diff_files" | grep '^test/.*\.dart$' || true) + lib_files_from_test_diff_files=$(get_lib_files_from_test_files "$test_diff_files") + local files_to_check
148-151: Guard against non-existent files before lcov extract.If the mapped lib file from a test doesn’t exist, lcov --extract may complain. Filter to existing files.
Apply this diff:
- while IFS= read -r file; do - files_array+=("$file") - done <<< "$files_to_check" + while IFS= read -r file; do + [[ -f "$file" ]] && files_array+=("$file") + done <<< "$files_to_check"
131-135: Avoid exiting from utility function; let the caller decide.get_files_to_check currently exits the whole script when empty. This makes the helper impure and duplicates exit handling in the caller. Prefer returning empty and keeping the early-exit in check_diff_coverage.
Apply this diff:
- if [ -z "$files_to_check" ]; then - print_success "No lib/ or test/ Dart files matching '${file_pattern}' changed. Skipping coverage check." - exit 0 - fi + if [ -z "$files_to_check" ]; then + print_success "No lib/ or test/ Dart files matching '${file_pattern}' changed. Skipping coverage check." + fiThe caller already handles the empty case (Lines 243–251).
8-10: Unused debug_print helper.debug_print is defined but not used. Either wire it into key branches (e.g., before/after git operations) or remove it to reduce noise.
If you intend to keep it, consider a VERBOSE flag to toggle debug verbosity.
.github/workflows/ci.yml (2)
78-78: Trim trailing whitespace to satisfy linters.YAMLlint flags trailing spaces on Line 78.
Apply this diff:
- +
79-82: Make apt-get more reliable on GitHub runners.Add update and auto-confirm to avoid intermittent failures.
Apply this diff:
- - name: Install lcov + - name: Install lcov if: github.event_name == 'pull_request' - run: sudo apt-get install lcov + run: sudo apt-get update && sudo apt-get install -y lcov
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
.github/workflows/ci.yml(1 hunks)scripts/check_diff_coverage.sh(3 hunks)
🧰 Additional context used
🪛 YAMLlint (1.37.1)
.github/workflows/ci.yml
[error] 78-78: trailing spaces
(trailing-spaces)
🪛 Shellcheck (0.10.0)
scripts/check_diff_coverage.sh
[warning] 247-247: Use single quotes, otherwise this expands now rather than when signalled.
(SC2064)
🔇 Additional comments (2)
scripts/check_diff_coverage.sh (1)
236-251: Happy path looks solid after bug fixes.Once the early grep failure and trap quoting are fixed, the flow (preflight → fetch → merge-base → file set → lcov extract → threshold check) is coherent and robust. Nice job handling “no relevant diffs” and mapping test-only changes to lib targets.
.github/workflows/ci.yml (1)
72-77: Good gating: coverage only on PRs, plain tests on pushes.This addresses the original failure mode on master and keeps coverage checks scoped to diffs in PRs.
Also applies to: 80-85
| get_lib_file_from_test_file() { | ||
| local test_file | ||
| test_file=$1 | ||
| local lib_file | ||
| lib_file=${test_file/test\//lib\/} | ||
| lib_file=${lib_file%_test.dart} | ||
| lib_file=${lib_file}.dart | ||
| echo "$lib_file" | ||
| } |
There was a problem hiding this comment.
Bug: Incorrect test→lib path mapping due to escaped slashes.
Using backslashes in parameter expansion results in literal backslashes and the substitution doesn’t occur, producing paths like test/... instead of lib/... . This breaks mapping for test-only changes and deletions.
Apply this diff:
get_lib_file_from_test_file() {
local test_file
test_file=$1
local lib_file
- lib_file=${test_file/test\//lib\/}
- lib_file=${lib_file%_test.dart}
- lib_file=${lib_file}.dart
+ # Map "test/…_test.dart" -> "lib/….dart"
+ lib_file="${test_file/#test\//lib/}"
+ lib_file="${lib_file%_test.dart}.dart"
echo "$lib_file"
}Optional hardening (if your tests aren’t always 1:1 with lib files): validate the file exists before including it downstream. I’ve proposed a guard below in calculate_coverage.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| get_lib_file_from_test_file() { | |
| local test_file | |
| test_file=$1 | |
| local lib_file | |
| lib_file=${test_file/test\//lib\/} | |
| lib_file=${lib_file%_test.dart} | |
| lib_file=${lib_file}.dart | |
| echo "$lib_file" | |
| } | |
| get_lib_file_from_test_file() { | |
| local test_file | |
| test_file=$1 | |
| local lib_file | |
| # Map "test/…_test.dart" -> "lib/….dart" | |
| lib_file="${test_file/#test\//lib/}" | |
| lib_file="${lib_file%_test.dart}.dart" | |
| echo "$lib_file" | |
| } |
🤖 Prompt for AI Agents
In scripts/check_diff_coverage.sh around lines 74 to 82, the test→lib path
mapping uses escaped slashes in the parameter expansion which leaves literal
backslashes and prevents substitution (producing paths like test/... instead of
lib/...), so change the substitution to use unescaped slashes (e.g.
lib_file=${test_file/test//lib/} or better lib_file=${test_file#test/};
lib_file=lib/${lib_file%_test.dart}.dart) so the replacement actually converts
the test path to the lib path; optionally add a guard after computing lib_file
to check [ -f "$lib_file" ] and skip or warn if the target file does not exist
before including it downstream.
| diff_files=$(git diff --name-only "$merge_base" HEAD | grep "${file_pattern}") | ||
| lib_diff_files=$(echo "$diff_files" | grep '^lib/.*\.dart$' || true) | ||
| test_diff_files=$(echo "$diff_files" | grep '^test/.*\.dart$' || true) | ||
| lib_files_from_test_diff_files=$(get_lib_files_from_test_files "$test_diff_files") | ||
| local files_to_check | ||
| files_to_check=$(combine_changed_files "$lib_diff_files" "$lib_files_from_test_diff_files") | ||
|
|
||
| if [ -z "$files_to_check" ]; then | ||
| print_success "No lib/ or test/ Dart files matching '${file_pattern}' changed. Skipping coverage check." | ||
| exit 0 | ||
| fi |
There was a problem hiding this comment.
Fix: grep will exit non-zero under set -e when no matches (breaking the “no diffs” path).
With set -euo pipefail, grep returning 1 aborts the script before your empty-set handling. Add a fallback to avoid premature exit.
Apply this diff:
- diff_files=$(git diff --name-only "$merge_base" HEAD | grep "${file_pattern}")
+ diff_files=$(git diff --name-only "$merge_base" HEAD | grep "${file_pattern}" || true)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| diff_files=$(git diff --name-only "$merge_base" HEAD | grep "${file_pattern}") | |
| lib_diff_files=$(echo "$diff_files" | grep '^lib/.*\.dart$' || true) | |
| test_diff_files=$(echo "$diff_files" | grep '^test/.*\.dart$' || true) | |
| lib_files_from_test_diff_files=$(get_lib_files_from_test_files "$test_diff_files") | |
| local files_to_check | |
| files_to_check=$(combine_changed_files "$lib_diff_files" "$lib_files_from_test_diff_files") | |
| if [ -z "$files_to_check" ]; then | |
| print_success "No lib/ or test/ Dart files matching '${file_pattern}' changed. Skipping coverage check." | |
| exit 0 | |
| fi | |
| diff_files=$(git diff --name-only "$merge_base" HEAD | grep "${file_pattern}" || true) | |
| lib_diff_files=$(echo "$diff_files" | grep '^lib/.*\.dart$' || true) | |
| test_diff_files=$(echo "$diff_files" | grep '^test/.*\.dart$' || true) | |
| lib_files_from_test_diff_files=$(get_lib_files_from_test_files "$test_diff_files") | |
| local files_to_check | |
| files_to_check=$(combine_changed_files "$lib_diff_files" "$lib_files_from_test_diff_files") | |
| if [ -z "$files_to_check" ]; then | |
| print_success "No lib/ or test/ Dart files matching '${file_pattern}' changed. Skipping coverage check." | |
| exit 0 | |
| fi |
🤖 Prompt for AI Agents
In scripts/check_diff_coverage.sh around lines 124 to 134, the pipeline
assigning diff_files uses git diff | grep "${file_pattern}" which will cause
grep to return exit status 1 under set -e when there are no matches and abort
the script; update that assignment to tolerate no matches by adding a fallback
(e.g., append "|| true" to the grep pipeline) so diff_files becomes empty
instead of causing the script to exit, keeping the subsequent empty-set handling
intact.
| exit 0 | ||
| else | ||
| temp_file=$(make_temp_file) | ||
| trap "rm -f $temp_file" EXIT |
There was a problem hiding this comment.
ShellCheck SC2064: Quote trap handler to avoid early expansion.
The trap string expands $temp_file immediately. Use single quotes and quote the variable inside the handler.
Apply this diff:
- trap "rm -f $temp_file" EXIT
+ trap 'rm -f "$temp_file"' EXIT📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| trap "rm -f $temp_file" EXIT | |
| trap 'rm -f "$temp_file"' EXIT |
🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 247-247: Use single quotes, otherwise this expands now rather than when signalled.
(SC2064)
🤖 Prompt for AI Agents
In scripts/check_diff_coverage.sh around line 247 the trap handler currently
uses double quotes causing $temp_file to expand immediately; change it to use
single quotes and quote the variable inside the handler so the expansion happens
at trap execution (e.g. trap 'rm -f "$temp_file"' EXIT) to avoid early expansion
per ShellCheck SC2064.
|
I'm addressing code rabbit's comments but in a separate PR, so that I'm no longer blocking other open PRs because of this |
|
Unless of course if you think that some of those comments should block the merge |
Description
In #464 I added a script to check diff coverage. This script checked the coverage of changed files that followed a pattern name (starting only for providers).
🐛 CI was failing in master due to new step that checked diff coverage.
🤔 This error helped me realize that there were multiple errors in this check and that it was only working OK when a provider file was changed. These are my conclusions
💡 So this PR:
Case 1: No files changed match pattern
Case 2: Provider test is removed
Case 3: Provider test without min coverage is modified
Case 4: Provider test with min coverage is modified
Case 5: Provider without min coverage is modified
Case 6: Provider with min coverage is modified
Type of Change
Checklist
just precommitto ensure that formatting and linting are correctjust check-flutter-coverageto ensure that flutter coverage rules are passingCHANGELOG.mdfile with your changes (if they affect the user experience)Summary by CodeRabbit