LF-12212 Adapt git action (ensure test coverage) to run only when needed #991
Conversation
WalkthroughThe pull request introduces modifications to the GitHub Actions workflow in Changes
Suggested reviewers
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Test Coverage ReportLine Coverage: 78.66% (2238 / 2845 lines) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/enforceTestCoverage.yml (1)
10-11: Consider monitoring test file changes.While filtering for
src/**is correct for coverage checks, changes to test files might affect coverage without triggering this workflow. Consider adding a separate workflow or extending this one to notify when test files are modified without corresponding source changes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/enforceTestCoverage.yml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: run-unit-tests
🔇 Additional comments (1)
.github/workflows/enforceTestCoverage.yml (1)
10-11: LGTM! The path filter optimizes workflow execution.The addition of the paths filter aligns with the PR objective and improves efficiency by running only when source files are modified.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
.github/workflows/enforceTestCoverage.yml (3)
35-44: Improve the file change detection pattern.The current grep pattern
^src/.*\.sol|^test/.*\.solmight miss Solidity files in subdirectories. Consider using a more robust pattern or GitHub's built-in path filters.Apply this diff to improve the pattern:
- CHANGED_FILES=$(git diff --name-only origin/${{ github.base_ref }}...HEAD | grep -E '^src/.*\.sol|^test/.*\.sol' || true) + CHANGED_FILES=$(git diff --name-only origin/${{ github.base_ref }}...HEAD | grep -E '^(src|test)/.*\.sol$' || true)Alternatively, you could use GitHub's built-in path filters in the workflow trigger:
on: pull_request: types: [opened, synchronize, reopened, ready_for_review] paths: - 'src/**.sol' - 'test/**.sol'
50-50: Consider optimizing conditional execution.The repetitive
if: env.solidity_changed == 'true'conditions could be simplified by moving them to the job level or using a composite step.Consider this alternative structure:
jobs: enforce-min-test-coverage: runs-on: ubuntu-latest if: | github.event.pull_request.draft == false && contains(github.event.pull_request.changed_files, '.sol') steps: # ... steps without individual conditionsOr create a composite step to reduce repetition:
jobs: enforce-min-test-coverage: runs-on: ubuntu-latest if: github.event.pull_request.draft == false steps: - name: Coverage Check if: env.solidity_changed == 'true' uses: ./.github/actions/coverage-check # ... remaining steps moved to composite actionAlso applies to: 54-54, 58-58, 62-62, 65-65, 76-76, 155-155
167-169: Enhance skip notification for better visibility.The current notification could be more informative and visible to PR reviewers.
Consider adding a PR comment when tests are skipped:
- name: Skip Tests (No Solidity Changes) if: env.solidity_changed == 'false' - run: echo "No Solidity files changed. Skipping test coverage check." + uses: mshick/[email protected] + with: + repo-token: ${{ secrets.GIT_ACTIONS_BOT_PAT_CLASSIC }} + message: | + ## Test Coverage Check Skipped + + No Solidity files were changed in this PR. Test coverage check was skipped. + + Note: Changes to non-Solidity files do not trigger coverage checks.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/enforceTestCoverage.yml(4 hunks)src/Facets/CBridgeFacet.sol(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/Facets/CBridgeFacet.sol
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: run-unit-tests
- GitHub Check: enforce-min-test-coverage
🔇 Additional comments (2)
.github/workflows/enforceTestCoverage.yml (2)
32-34: LGTM! Settingfetch-depth: 0is necessary for git history.The configuration ensures that the complete git history is available for comparing changes with the base branch.
7-9: Verify the required check status in repository settings.Based on past discussions, this workflow's status as a required check might need adjustment. If it's marked as required in repository settings, PRs with no Solidity changes might be blocked unnecessarily.
Please check:
- Repository Settings > Branches > Branch protection rules
- Verify if "Enforce Min Test Coverage" is listed under required status checks
- Consider the trade-offs:
- Required: Ensures coverage checks always pass but might block PRs unnecessarily
- Optional: More flexible but might allow merging without coverage checks
|
So to solve it and to keep action reporting and also run min test coverage script only when there is a change for |
0xDEnYO
deleted the
lf-12212-run-ensure-test-coverage-only-when-needed
branch
Which Jira task belongs to this PR?
LF-12212
Why did I implement it this way?
Checklist before requesting a review
Checklist for reviewer (DO NOT DEPLOY and contracts BEFORE CHECKING THIS!!!)