Store coverage artifacts for branch comparison (#13)#16
Store coverage artifacts for branch comparison (#13)#16jordanpartridge merged 1 commit intomasterfrom
Conversation
|
Warning Rate limit exceeded@jordanpartridge has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 25 minutes and 1 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughThis PR adds coverage artifact storage and comparison infrastructure by uploading Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~35 minutes Areas requiring extra attention:
Possibly related issues
Poem
Pre-merge checks and finishing touches❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
app/Services/CloverParser.php (1)
24-30: Consider more explicit XML error handling.While the
@suppression with false-check works, usinglibxml_use_internal_errors(true)would provide better diagnostics if needed in the future.Alternative approach:
- $xml = @simplexml_load_file($this->cloverPath); - if ($xml === false) { + libxml_use_internal_errors(true); + $xml = simplexml_load_file($this->cloverPath); + libxml_use_internal_errors(false); + + if ($xml === false) {
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.github/workflows/gate.yml(1 hunks).gitignore(1 hunks)action.yml(2 hunks)app/Checks/TestRunner.php(1 hunks)app/Commands/CertifyCommand.php(4 hunks)app/Services/CloverParser.php(1 hunks)phpunit.xml(2 hunks)tests/Unit/Services/CloverParserTest.php(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
app/Checks/*.php
📄 CodeRabbit inference engine (CLAUDE.md)
All quality checks must implement the
CheckInterfaceinterface in the Checks directory
Files:
app/Checks/TestRunner.php
app/Commands/*.php
📄 CodeRabbit inference engine (CLAUDE.md)
GitHub Actions output should use
::errorannotations for file-specific issues in format::error file=<path>,line=<num>::<message>
Files:
app/Commands/CertifyCommand.php
🧠 Learnings (2)
📚 Learning: 2025-12-15T00:43:05.031Z
Learnt from: CR
Repo: synapse-sentinel/gate PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-15T00:43:05.031Z
Learning: Run `php gate run --coverage=100` to execute the gate locally with required 100% code coverage
Applied to files:
.github/workflows/gate.ymlaction.yml
📚 Learning: 2025-12-15T00:43:05.031Z
Learnt from: CR
Repo: synapse-sentinel/gate PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-15T00:43:05.031Z
Learning: Run tests using `vendor/bin/pest` with `--coverage --min=100` flags to enforce 100% code coverage
Applied to files:
tests/Unit/Services/CloverParserTest.php.gitignoreapp/Checks/TestRunner.php
🧬 Code graph analysis (2)
tests/Unit/Services/CloverParserTest.php (1)
app/Services/CloverParser.php (2)
CloverParser(9-97)parse(15-36)
app/Commands/CertifyCommand.php (9)
app/GitHub/ChecksClient.php (1)
ChecksClient(10-129)app/Checks/TestRunner.php (4)
__construct(11-13)TestRunner(9-104)name(15-18)run(20-40)app/Commands/SyntaxCommand.php (1)
handle(24-63)app/Commands/SecurityCommand.php (1)
handle(24-59)app/Commands/TestsCommand.php (1)
handle(25-61)app/Checks/SecurityScanner.php (1)
SecurityScanner(9-48)app/Checks/PestSyntaxValidator.php (1)
PestSyntaxValidator(9-47)app/Verdict.php (4)
failures(47-50)isApproved(52-55)Verdict(7-109)status(34-37)app/Checks/CheckResult.php (1)
CheckResult(7-24)
🔇 Additional comments (12)
.gitignore (1)
7-8: LGTM!The new ignore entries correctly match the coverage artifacts:
coverage.xmlgenerated by tests andbase-coverage/populated by the artifact download step.phpunit.xml (1)
28-32: LGTM!The Clover coverage output configuration is correct and aligns with the workflow's artifact upload step that expects
coverage.xml..github/workflows/gate.yml (2)
25-33: LGTM on the download step.The
continue-on-error: truecorrectly handles the case when no baseline coverage exists (e.g., first run on a new repo). The downloadedbase-coverage/directory is staged for future coverage delta comparison as noted in the PR objectives.
38-44: LGTM!The upload conditions correctly ensure artifacts are only stored on main branch pushes, and the 30-day retention meets the requirements from Issue #13.
action.yml (2)
57-71: LGTM on coverage directory detection.The auto-detection logic for
app/→src/→ workspace root is sensible. ThePCOV_DIRECTORYenv var is correctly exported viaGITHUB_ENVfor use in the subsequent step.
78-80: The conditional reference toenv.PCOV_DIRECTORYon line 80 is properly guarded and will not cause issues. Since the variable is only referenced wheninputs.coverage-driver == 'pcov'(the exact condition that sets it in the Configure Coverage step), andCOVERAGE_DIRis assigned with a fallback togithub.workspace,PCOV_DIRECTORYis guaranteed to have a valid value whenever it's referenced. No changes needed.Likely an incorrect or invalid review comment.
app/Commands/CertifyCommand.php (3)
31-49: LGTM on testing hooks.The
@internalannotation andwithMocks()pattern provide a clean way to inject test doubles without polluting the production API. The nullable properties with null-coalescing inhandle()keep the default behavior intact.
161-168: LGTM on compact mode execution.Skipping the spinner in compact mode is appropriate—spinners interfere with single-line summary output and CI log readability.
200-216: LGTM on compact output rendering.The single-line format with check results and overall status provides a clean CI-friendly summary.
app/Checks/TestRunner.php (1)
23-23: LGTM! Clean integration of Clover coverage generation.The addition of
--coverage-cloverflag correctly generates the coverage.xml file needed for artifact storage and branch comparison, aligning with the PR objectives.tests/Unit/Services/CloverParserTest.php (1)
7-156: Excellent test coverage of CloverParser functionality.The test suite comprehensively validates:
- Full and partial coverage scenarios
- Graceful error handling for missing and invalid files
- Edge cases like empty metrics
- Multi-file coverage calculation accuracy
app/Services/CloverParser.php (1)
72-78: Verify that 100% is the appropriate fallback for zero totals.When a file has zero statements or methods, the code returns 100% coverage. While this is consistent with "no uncovered code," it may be semantically different from "all code is covered."
This behavior is tested and appears intentional. Confirm this aligns with your coverage reporting expectations, particularly for:
- Generated files or interfaces with no implementations
- Files excluded from coverage but present in the report
| // Post PR comment with badge on success | ||
| if ($verdict->isApproved()) { | ||
| $checksClient->postCertificationComment($checkResults); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for postCertificationComment method definition
ast-grep --pattern 'function postCertificationComment($$$) { $$$ }'Repository: synapse-sentinel/gate
Length of output: 47
🏁 Script executed:
# Also search for the method name directly in case the pattern doesn't match
rg 'postCertificationComment' --type phpRepository: synapse-sentinel/gate
Length of output: 164
🏁 Script executed:
# Check the ChecksClient file to see all methods
fd 'ChecksClient' --type f --exec cat {} \;Repository: synapse-sentinel/gate
Length of output: 3657
Add missing postCertificationComment method to ChecksClient.
The code at line 112-115 calls $checksClient->postCertificationComment($checkResults), but this method does not exist in the ChecksClient class. The ChecksClient currently only contains these methods: isAvailable(), createCheck(), completeCheck(), and reportCheck(). This will cause a runtime error. Add the postCertificationComment() method to ChecksClient to handle posting certification comments.
| <?xml version="1.0" encoding="UTF-8"?> | ||
| <phpunit xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" | ||
| xsi:noNamespaceSchemaLocation="vendor/phpunit/phpunit/phpunit.xsd" | ||
| xsi:noNamespaceSchemaLocation="vendor/phpunit/phpunit.xsd" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd -t f 'phpunit.xsd' vendor/Repository: synapse-sentinel/gate
Length of output: 159
🏁 Script executed:
# Find and display composer.json to see PHPUnit version
find . -name "composer.json" -o -name "composer.lock" | head -5
# Display the phpunit.xml file to see full context
cat phpunit.xmlRepository: synapse-sentinel/gate
Length of output: 1094
🏁 Script executed:
# Check PHPUnit version in composer.json
cat composer.json | grep -A 5 -B 5 phpunitRepository: synapse-sentinel/gate
Length of output: 47
🏁 Script executed:
# Display full composer.json to see dependencies
head -100 composer.jsonRepository: synapse-sentinel/gate
Length of output: 1232
🏁 Script executed:
# Check composer.lock for PHPUnit version (Pest uses PHPUnit)
grep -A 5 '"name": "phpunit/phpunit"' composer.lock | head -20Repository: synapse-sentinel/gate
Length of output: 339
🏁 Script executed:
# Check if there's any documentation or comments in the repository about the schema
rg -i "phpunit.xsd" --no-heading
# Also check git history to see if this was changed recently
git log --oneline -n 20 -- phpunit.xmlRepository: synapse-sentinel/gate
Length of output: 204
Fix the PHPUnit schema path.
The schema path vendor/phpunit/phpunit.xsd is incorrect. PHPUnit 11 packages are installed at vendor/phpunit/phpunit/, so the schema should be at vendor/phpunit/phpunit/phpunit.xsd. Update the xsi:noNamespaceSchemaLocation attribute to include the correct path to avoid validation warnings.
🤖 Prompt for AI Agents
phpunit.xml around line 3: the xsi:noNamespaceSchemaLocation currently points to
vendor/phpunit/phpunit.xsd which is incorrect for PHPUnit 11; update the
attribute to vendor/phpunit/phpunit/phpunit.xsd so it points into the phpunit
directory (i.e., replace the existing path with
vendor/phpunit/phpunit/phpunit.xsd) to eliminate schema validation warnings.
Enable historical coverage tracking by storing coverage.xml as artifacts on main branch pushes and downloading base branch coverage for PR comparisons. Changes: - Add clover coverage.xml generation to phpunit.xml - Configure GitHub Actions to upload coverage artifacts on main pushes (30-day retention) - Download base branch coverage on PR runs for comparison - Update .gitignore to exclude coverage.xml and base-coverage/ Implements requirements from issue #13: - Artifact upload on main branch pushes - Artifact download for PR comparisons - 30-day retention policy - Graceful handling when baseline data is missing (continue-on-error)
jordanpartridge
force-pushed
the
feat/switch-pcov-coverage
branch
from
73ebe94 to
a969fe1
Compare
1 participant
Summary
Implements Issue #13 by adding coverage artifact storage to enable historical coverage tracking and branch-to-branch comparison.
Changes
Implementation Details
coverage-reportcontaining coverage.xmlTesting
Closes #13
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.