-
Notifications
You must be signed in to change notification settings - Fork 25
tests: improve unit test coverage and set threshold #296
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 9 commits
61d9d69
9a38962
35a1952
40786bc
646fb68
89e1847
ec37ac9
5d986f2
1636d96
c956a66
22372c3
bde48bb
826e41e
667065a
31e1220
0cc5d16
041d369
6755ff9
967eb61
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -34,22 +34,32 @@ jobs: | |
| RUST_BACKTRACE: 1 | ||
|
|
||
| - name: Test for no_std | ||
| run: cargo test --release --no-default-features --features embassy-rt,core,utils,wallet,models,helpers,websocket,json-rpc | ||
| run: cargo test --release --no-default-features --features | ||
| embassy-rt,core,utils,wallet,models,helpers,websocket,json-rpc | ||
| env: | ||
| RUST_BACKTRACE: 1 | ||
|
|
||
| - name: Install cargo-llvm-cov | ||
| uses: taiki-e/install-action@cargo-llvm-cov | ||
|
|
||
| # Integration-territory files (CLI, async network clients, sync wrappers | ||
| # around network calls, faucet) are excluded from unit-test coverage. | ||
| # They are exercised by tests under tests/ behind the `integration` | ||
| # feature flag, which run against a live rippled in a separate workflow. | ||
| # Mirrors xrpl-py's split between tests/unit/ (--fail-under=85) and | ||
| # tests/integration/ (--fail-under=70). | ||
|
pdp2121 marked this conversation as resolved.
Outdated
|
||
| - name: Generate coverage report | ||
| run: cargo llvm-cov --lcov --output-path lcov.info | ||
| run: | | ||
| cargo llvm-cov --lcov --output-path lcov.info \ | ||
| --ignore-filename-regex 'src/(bin|cli|clients|account|ledger|asynch/clients|asynch/account|asynch/ledger)/|src/transaction/mod\.rs|src/wallet/faucet_generation\.rs' | ||
|
|
||
| - name: Check coverage thresholds | ||
| run: | | ||
| cargo llvm-cov --summary-only \ | ||
| --fail-under-lines 73 \ | ||
| --fail-under-regions 75 \ | ||
| --fail-under-functions 67 | ||
| --ignore-filename-regex 'src/(bin|cli|clients|account|ledger|asynch/clients|asynch/account|asynch/ledger)/|src/transaction/mod\.rs|src/wallet/faucet_generation\.rs' \ | ||
| --fail-under-lines 85 \ | ||
| --fail-under-regions 85 \ | ||
| --fail-under-functions 75 | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a way to fail the check when coverage for new or modified code introduced by a PR falls below a certain percentage? I’m less concerned about the current coverage of existing code. If we want to maintain a high bar for test coverage, we should apply that standard to new code. Over time, that will naturally improve the overall coverage of the repo.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, patch coverage is possible to do, but would require Codecov (similar to the requirements for removing integration test regex) since
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you create a follow-up story to track this work so it doesn’t get lost? I think enforcing patch coverage with a high threshold could be a better approach than setting a project-wide threshold, since it focuses on improving new and modified code without relying on broad exclusions. Over time, this should help gradually improve coverage in a repo with low overall coverage. BTW, xrpl4j uses Codecov (see code). Example PR - XRPLF/xrpl4j#786
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Created the issue: #307 Nice! that means the setup for Codecov in XRPLF has already been done, making our task easier without the need for admin privilege
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually let me just add it directly into this PR since Codecov is enabled already in XRPLF so there's no actual blocker
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added in 22372c3 |
||
|
|
||
| - name: Upload coverage report | ||
| uses: actions/upload-artifact@v4 | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.