Improve fungible token coverage#113
Improve fungible token coverage#113tinkerer-shubh wants to merge 4 commits intoOpenZeppelin:mainfrom
Conversation
* Add tests for allowance TTL handling and edge cases * Add tests for capped supply functionality * Create comprehensive coverage documentation * Set up GitHub Actions workflow for coverage checks
* Add new test files for storage and trait functionality * Update existing test files in extensions * Include all necessary test snapshots
✅ Deploy Preview for delightful-dieffenbachia-2097e0 canceled.
|
| # Extract branch coverage and verify threshold | ||
| if grep -q "TOTAL.*branches" coverage-report.txt; then | ||
| BRANCH_COVERAGE=$(grep "TOTAL.*branches" coverage-report.txt | awk '{print $NF}' | sed 's/%//') | ||
| echo "Branch coverage: $BRANCH_COVERAGE%" | ||
| if (( $(echo "$BRANCH_COVERAGE < 85" | bc -l) )); then | ||
| echo "::error::Branch coverage $BRANCH_COVERAGE% is below the target of 85%" | ||
| echo "Uncovered branches:" | ||
| cat coverage-report.txt | grep -B 1 "\^" || echo "No specific uncovered branches found!" | ||
| exit 1 | ||
| else | ||
| echo "Branch coverage meets target: $BRANCH_COVERAGE% ≥ 85%" | ||
| fi | ||
| else | ||
| echo "Branch coverage information not available in this version of cargo-llvm-cov" | ||
| BRANCH_COVERAGE="Not available" | ||
| fi |
There was a problem hiding this comment.
Heads up about how this implementation works with cargo-llvm-cov's text reports:
- We look for "TOTAL.*branches" to find branch coverage info
- Pull the percentage with awk
- Spot uncovered code by finding "^" markers
This works fine now, but if cargo-llvm-cov changes its output format, our parsing might break. We should probably:
- Document this dependency somewhere
- Maybe add a version check for cargo-llvm-cov
- Keep an eye on this when we upgrade tools
- Consider using LCOV format with a proper parser if we need something more stable
For now though, this approach keeps things simple while getting the job done.
(OR we can just keep the line coverage for now and add branch coverage when it is natively supported in cargo-llvm-cov)
| - Improved branch coverage in critical areas like allowance management and extensions | ||
| - Reduced uncovered code in error paths | ||
|
|
||
| Some areas remain uncovered (documented in the "Uncovered Areas" section), but these are primarily in generated code or extreme edge cases that would require specialized testing techniques. No newline at end of file |
There was a problem hiding this comment.
I added coverage.md mainly to help reviewers understand the reasoning behind the test coverage choices in this PR.
I think we can safely remove this file in the end before the merge - it's just helpful context for the review process.
Also, I'd love feedback on how I’ve set up the test placements in the project structure—just checking to see if they feel right with the rest of it.
| - name: Upload coverage report to Codecov | ||
| # Only run if the Codecov token is configured | ||
| if: ${{ secrets.CODECOV_TOKEN != '' }} | ||
| continue-on-error: true # Don't fail the workflow if Codecov upload fails | ||
| uses: codecov/codecov-action@v3 | ||
| with: | ||
| token: ${{ secrets.CODECOV_TOKEN }} | ||
| files: lcov.info | ||
| fail_ci_if_error: false # Don't fail even if the Codecov upload fails No newline at end of file |
There was a problem hiding this comment.
I've added an optional Codecov integration that only runs if you have the CODECOV_TOKEN secret set up. It gives nicer visualizations and keeps history longer than GitHub's 14-day artifact storage.
(Totally optional though - the core coverage validation works fine without it.)
There was a problem hiding this comment.
tsk tsk
the workflow infact did not work fine without it. still working on this one!
There was a problem hiding this comment.
hey @tinkerer-shubh
- Added
CODECOV_TOKENto repo's secrets. - Opened this PR just to test it gets detected and it seems fine (had to make some changes to coverage.yml to make it run).
- You can use the config from the Stylus repo
|
(I had forgotten to attach the screenshot!) |
I noticed in the screenshot that the coverage report isn't showing branch coverage (it's all '0'). Maybe the script for this is failing? I'll dig into it. Another option—if we decide to stick with the scripts—is to use region coverage as a proxy for branch coverage. Not exactly the same, but could do the trick! |
|
@tinkerer-shubh we had some urgency to add code coverage, so I added it in here: #147 Hope it helps you in case you still want to progress on this PR. It would definitely help us to improve our coverage :) |
3 participants
Fixes #96
I'm marking this PR as a draft for now because:
One of the goals of this issue was to increase test coverage for the functions, which this PR doesn't address yet. While there are new tests covering edge cases, I think my suggestion that these functions don't need explicit tests isn't the best approach.
I'm also considering that simplifying the YAML file might be a better option.
I'll make this ready for review once I've addressed these points. :) Open to any feedback in the meantime!
(apologies for being all over the place on this one!)
PR Checklist