ci: integrate reusable workflows for code quality and coverage - #41
ci: integrate reusable workflows for code quality and coverage#41pkking wants to merge 1 commit into
Conversation
- Add go_coverage_check.sh script for incremental coverage analysis - Add ci-reusable.yml workflow calling organization reusable workflows
Welcome To opensourceways CommunityHey @pkking , thanks for your contribution to the community. Bot Usage ManualI'm the Bot here serving you. You can find the instructions on how to interact with me at Here . That means you can comment below every pull request or issue to trigger Bot Commands. Contact GuideIf you have any questions, please contact the SIG: infratructure , |
CLA Signature Passpkking, thanks for your pull request. All authors of the commits have signed the CLA. 👍 |
Linking Issue Notice@pkking , the pull request must be linked to at least one issue. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a unified Go coverage check script (scripts/go_coverage_check.sh) designed to calculate both full and incremental coverage. Feedback focuses on improving the script's robustness and accuracy. Key suggestions include adopting set -euo pipefail for better error handling, refining the parsing of go tool cover output, and relaxing the overly aggressive filtering of changed lines to ensure accurate incremental coverage. Additionally, concerns were raised regarding the script's performance due to O(N*M) complexity in the coverage block matching logic and its dependency on Bash 4.0+ features.
| if (content == "" || | ||
| content ~ /^}/ || | ||
| content ~ /^{/ || | ||
| content ~ /^\)/ || | ||
| content ~ /^package/ || | ||
| content ~ /^import/ || | ||
| content ~ /^\/\// || | ||
| content ~ /^\/\*/ || | ||
| content ~ /^type.*struct{/ || | ||
| content ~ /^type.*interface{/) { | ||
| line++; | ||
| next; | ||
| } | ||
|
|
There was a problem hiding this comment.
The heuristic used to filter 'non-executable' lines is too aggressive and can lead to incorrect incremental coverage results. For instance, lines starting with } (like } else {) are often executable or part of a control flow block but will be skipped by this regex. Since the script already checks if a line falls within a coverage block (line 192), it is safer to only skip truly empty lines in the awk script and let the block-matching logic handle the rest.
| if (content == "" || | |
| content ~ /^}/ || | |
| content ~ /^{/ || | |
| content ~ /^\)/ || | |
| content ~ /^package/ || | |
| content ~ /^import/ || | |
| content ~ /^\/\// || | |
| content ~ /^\/\*/ || | |
| content ~ /^type.*struct{/ || | |
| content ~ /^type.*interface{/) { | |
| line++; | |
| next; | |
| } | |
| # Only skip truly empty lines to maintain line count | |
| if (content == "") { | |
| line++; | |
| next; | |
| } |
| # Full coverage threshold: configurable (default 10%) | ||
| # Incremental coverage threshold: configurable (default 80%) | ||
|
|
||
| set -e |
There was a problem hiding this comment.
It is recommended to use set -euo pipefail for more robust error handling. set -u treats unset variables as an error, and set -o pipefail ensures that a pipeline returns the exit status of the last command to exit with a non-zero status.
| set -e | |
| set -euo pipefail |
| fi | ||
|
|
||
| # Extract total coverage percentage | ||
| coverage=$(go tool cover -func="$coverage_file" | grep total | awk '{print $3}' | sed 's/%//') |
There was a problem hiding this comment.
The parsing of go tool cover output is fragile. grep total might match function names containing the word 'total'. Additionally, using $3 assumes a specific number of columns which can vary between Go versions or output formats. Using ^total: and $NF (the last field) is more robust.
| coverage=$(go tool cover -func="$coverage_file" | grep total | awk '{print $3}' | sed 's/%//') | |
| coverage=$(go tool cover -func="$coverage_file" | grep "^total:" | awk '{print $NF}' | sed 's/%//') |
| echo "$changed_lines" | head -20 | ||
|
|
||
| # Use associative array for processed blocks (more reliable than string matching) | ||
| declare -A processed_blocks=() |
There was a problem hiding this comment.
The use of associative arrays (declare -A) requires Bash 4.0+. While this is standard in most CI environments, it will cause the script to fail on systems with older Bash versions (e.g., default macOS). Consider adding a check at the start of the script to verify the Bash version or documenting this requirement.
| while IFS= read -r block_data; do | ||
| [[ -z "$block_data" ]] && continue | ||
|
|
||
| # Parse: start_line.col,end_line.col stmts hits | ||
| local line_range stmts hits | ||
| read -r line_range stmts hits <<< "$block_data" | ||
|
|
||
| local start_line="${line_range%%.*}" | ||
| local tmp="${line_range#*,}" | ||
| local end_line="${tmp%%.*}" | ||
|
|
||
| # Check if changed line is within block range | ||
| if ((target_line >= start_line && target_line <= end_line)); then | ||
| local block_uid="${target_file}:${line_range}" | ||
|
|
||
| if [[ -z "${processed_blocks[$block_uid]}" ]]; then | ||
| total_inc_stmts=$((total_inc_stmts + stmts)) | ||
|
|
||
| if ((hits > 0)); then | ||
| covered_inc_stmts=$((covered_inc_stmts + stmts)) | ||
| echo " ✅ Covered: ${block_uid} (${stmts} stmts)" | ||
| else | ||
| echo " ❌ Uncovered: ${block_uid} (${stmts} stmts)" | ||
| fi | ||
|
|
||
| processed_blocks[$block_uid]=1 | ||
| fi | ||
| break | ||
| fi | ||
| done <<< "$(echo -e "$blocks_data")" |
There was a problem hiding this comment.
This nested loop performs a linear search through all coverage blocks for every changed line, resulting in O(N*M) complexity. For large files or PRs, this will be significantly slow in a shell script. Additionally, the use of echo -e and a subshell inside the loop adds unnecessary overhead. A more efficient approach would be to use awk to join the changed lines with the coverage blocks in a single pass.
2 participants
Summary
Integrate organization reusable workflows to improve code quality, documentation quality, and test coverage.
Changes
1. scripts/go_coverage_check.sh
Add unified Go coverage check script with:
2. .github/workflows/ci-reusable.yml
New CI pipeline calling organization reusable workflows:
Related