Skip to content

Conversation

@krystophny
Copy link
Contributor

@krystophny krystophny commented Aug 4, 2025

User description

Summary

  • Adds comprehensive code coverage analysis to the CI workflow
  • Implements coverage reporting similar to fortfront's setup
  • Provides visibility into test coverage for better quality assurance

Changes

  • New test-with-coverage job: Dedicated job for Ubuntu/GCC-12 with coverage instrumentation
  • Coverage generation: Uses lcov to capture coverage data with branch coverage enabled
  • GitHub integration: Converts coverage to Cobertura XML for GitHub Actions compatibility
  • Coverage reporting: Uses insightsengineering/coverage-action for detailed reports
  • PR checks: Creates coverage/project and coverage/patch checks with 50% thresholds
  • Artifact storage: Uploads coverage reports for review and historical tracking

Additional fixes

  • Fixed line truncation error in fluff_diagnostics XML formatting

Test plan

  • CI workflow runs successfully on this PR
  • Coverage report is generated and visible in PR comments
  • Coverage checks appear in the PR status
  • Artifacts are uploaded correctly

PR Type

Enhancement, Tests


Description

  • Add comprehensive code coverage analysis to CI workflow

  • Implement coverage reporting with GitHub integration

  • Fix XML formatting line truncation error

  • Create coverage checks with 50% thresholds


Diagram Walkthrough

flowchart LR
  A["Test with Coverage Job"] --> B["Generate LCOV Data"]
  B --> C["Convert to Cobertura XML"]
  C --> D["Coverage Action Report"]
  D --> E["GitHub Coverage Checks"]
  E --> F["Upload Coverage Artifacts"]
Loading

File Walkthrough

Relevant files
Enhancement
ci.yml
Add comprehensive coverage analysis workflow                         

.github/workflows/ci.yml

  • Add new test-with-coverage job with Ubuntu/GCC-12
  • Implement LCOV coverage generation with branch coverage
  • Add Cobertura XML conversion for GitHub Actions compatibility
  • Create coverage checks for project and patch thresholds (50%)
  • Upload coverage artifacts and integrate with existing workflow
    dependencies
+180/-3 
Bug fix
fluff_diagnostics.f90
Fix XML format string line truncation                                       

src/fluff_diagnostics/fluff_diagnostics.f90

  • Fix line truncation error in XML formatting
  • Split long format string across multiple lines with continuation
    characters
  • Maintain same functionality with improved readability
+7/-3     

- Add test-with-coverage job for Ubuntu with GCC 12
- Generate lcov coverage data with branch coverage
- Convert to Cobertura XML for GitHub Actions integration
- Add coverage reporting with insightsengineering/coverage-action
- Create coverage checks for project and patch thresholds (50%)
- Upload coverage artifacts for review
- Fix line truncation error in fluff_diagnostics XML formatting
- Maintain existing test matrix for other platforms/versions
@qodo-code-review
Copy link

qodo-code-review bot commented Aug 4, 2025

CI Feedback 🧐

(Feedback updated until commit 4a3fae4)

A test triggered by this PR failed. Here is an AI-generated analysis of the failure:

Action: Test Suite (windows-latest, 11)

Failed stage: Set up job [❌]

Failure summary:

The action failed because it uses a deprecated version of actions/upload-artifact: v3. GitHub has
deprecated v3 of the artifact actions and automatically fails requests that use this version.

Relevant error logs:
1:  ##[group]Runner Image Provisioner
2:  Hosted Compute Agent
...

15:  Image Release: https://github.com/actions/runner-images/releases/tag/win22%2F20250727.1
16:  ##[endgroup]
17:  ##[group]GITHUB_TOKEN Permissions
18:  Actions: read
19:  Checks: write
20:  Contents: write
21:  Issues: write
22:  Metadata: read
23:  Pages: write
24:  PullRequests: write
25:  ##[endgroup]
26:  Secret source: Actions
27:  Prepare workflow directory
28:  Prepare all required actions
29:  Getting action download info
30:  ##[error]This request has been automatically failed because it uses a deprecated version of `actions/upload-artifact: v3`. Learn more: https://github.blog/changelog/2024-04-16-deprecation-notice-v3-of-the-artifact-actions/

@qodo-code-review
Copy link

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Incorrect Logic

The patch coverage calculation simply reuses project coverage instead of calculating actual patch-specific coverage. This makes the patch coverage check meaningless as it will always match project coverage.

const patchCoverage = projectCoverage;
Resource Usage

The hardcoded OMP_NUM_THREADS=24 may not be appropriate for all GitHub Actions runners and could cause resource contention or inefficient resource usage.

OMP_NUM_THREADS=24 fpm test --profile debug --flag '-cpp -fprofile-arcs -ftest-coverage -g'
Error Handling

The coverage report step uses continue-on-error but subsequent steps depend on the cobertura.xml file it generates, which could lead to failures in coverage checks if the report generation fails.

continue-on-error: true
with:

@qodo-code-review
Copy link

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Fix incorrect patch coverage calculation

The patch coverage is incorrectly set to the same value as project coverage.
This defeats the purpose of having separate coverage checks for the entire
project versus just the changed lines in the PR.

.github/workflows/ci.yml [122]

-const patchCoverage = projectCoverage;
+// Calculate actual patch coverage from diff
+const patchCoverage = calculatePatchCoverage(xml, context);
  • Apply / Chat
Suggestion importance[1-10]: 9

__

Why: The suggestion correctly identifies a significant bug where patchCoverage is incorrectly assigned the value of projectCoverage, making the patch coverage check useless and misleading.

High
General
Use dynamic thread count detection

The hardcoded OMP_NUM_THREADS=24 may not be appropriate for all runner
environments and could cause resource contention or inefficient execution on
machines with fewer cores.

.github/workflows/ci.yml [65]

-OMP_NUM_THREADS=24 fpm test --profile debug --flag '-cpp -fprofile-arcs -ftest-coverage -g'
+OMP_NUM_THREADS=$(nproc) fpm test --profile debug --flag '-cpp -fprofile-arcs -ftest-coverage -g'
  • Apply / Chat
Suggestion importance[1-10]: 6

__

Why: The suggestion correctly points out that hardcoding OMP_NUM_THREADS is not ideal and proposes a more robust solution using nproc to dynamically set the thread count based on the runner's available cores.

Low
Add debugging output for failures

The script exits with code 1 on failure but the step doesn't have
continue-on-error: false explicitly set. Consider adding error handling or
making the failure behavior more explicit.

.github/workflows/ci.yml [89-92]

 if [ ! -f "cobertura.xml" ]; then
   echo "Failed to generate cobertura.xml"
+  ls -la
   exit 1
 fi
  • Apply / Chat
Suggestion importance[1-10]: 3

__

Why: The suggestion proposes adding a debugging command (ls -la) which can be helpful, but it's a minor improvement to an already functional error-checking block.

Low
  • More

@krystophny
Copy link
Contributor Author

Superseded by PR #4 which includes all coverage analysis work plus complete AST rule implementations

@krystophny krystophny closed this Aug 4, 2025
@krystophny krystophny deleted the add-coverage-analysis branch August 4, 2025 18:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants