Expand unit test suite to 90 tests with numerical correctness coverage#25
Open
wsnoble wants to merge 9 commits into
Open
Expand unit test suite to 90 tests with numerical correctness coverage#25wsnoble wants to merge 9 commits into
wsnoble wants to merge 9 commits into
Conversation
Add ~47 new tests across 6 files covering gaps identified in TESTING_PLAN.md: - test_confidence.py: Replace type-only stubs with exact q-value assertions, FDR threshold counts, monotonicity invariants, pep_fdr_type and prot_fdr_type parametrized coverage, and column-presence checks for threshold modes. - test_writers.py (new): File creation, column names, row counts, round-trip score fidelity, decoy flag, and threshold filtering. - test_edge_cases.py (new): Competition correctness, shared-peptide exclusion, empty/all-target/all-decoy construction errors, all-levels presence. - test_qvalues.py: Edge cases (all targets win, decoy wins, hand-computed 4T/2D case), monotonicity parametrize, _fdr2qvalue direct test. - test_dataset.py: No-decoys/no-targets ValueError, count correctness, peptide_pairing storage, missing column error, find_best_score edge case. - test_parsers.py: Multi-file concatenation, target-only raises, missing column error, copy_data=False, protein_delim storage. - conftest.py: Add clean_psm_df/clean_psms fixtures with hand-computed expected q-values (4 targets → q=0.25, 2 decoys → q=0.50/0.75). - SCALING_PLAN.md, TESTING_PLAN.md: Add planning documents. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Warning Review limit reached
More reviews will be available in 57 minutes and 4 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. 📝 WalkthroughWalkthroughAdds two planning documents ( ChangesPlanning Documents
Test Implementation
Code Quality and Refactoring
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/unit_tests/test_writers.py (1)
1-152:⚠️ Potential issue | 🟠 Major | ⚡ Quick winThis file currently fails CI formatting checks (Black).
Pipeline shows Black would reformat this file. Please run Black on this file before merge to unblock lint.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit_tests/test_writers.py` around lines 1 - 152, The test_writers.py file fails Black formatting checks and needs to be reformatted to comply with Black's code style standards. Run Black on this file to automatically fix all formatting issues. This will ensure the file passes the CI linting checks before merging.Source: Pipeline failures
tests/unit_tests/test_edge_cases.py (1)
1-205:⚠️ Potential issue | 🟠 Major | ⚡ Quick winThis file currently fails CI formatting checks (Black).
Pipeline shows Black would reformat this file. Please run Black on this file before merge to unblock lint.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit_tests/test_edge_cases.py` around lines 1 - 205, The test file test_edge_cases.py is failing Black code formatting checks. Run the Black formatter on this file to automatically fix all formatting issues. Use the command black tests/unit_tests/test_edge_cases.py to reformat the entire file according to Black's style guidelines, then commit the changes. This will resolve the CI lint failure blocking the merge.Source: Pipeline failures
🧹 Nitpick comments (1)
tests/unit_tests/test_confidence.py (1)
273-274: ⚡ Quick win
len(...) >= 0is tautological and does not validate behavior.This assertion always passes and adds no signal. Replace it with a non-empty or schema/value invariant assertion.
Proposed fix
- assert len(conf.confidence_estimates["peptides"]) >= 0 + assert len(conf.confidence_estimates["peptides"]) > 0🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit_tests/test_confidence.py` around lines 273 - 274, The assertion checking len(conf.confidence_estimates["peptides"]) >= 0 is tautological because len() always returns a non-negative value, making the assertion always pass without validating actual behavior. Replace this assertion with a meaningful check such as asserting the list is non-empty (len > 0) or validating a specific schema or value invariant that confirms the confidence_estimates["peptides"] contains the expected data structure.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/unit_tests/test_confidence.py`:
- Around line 359-368: The assertion in the
test_confidence_estimates_contains_only_targets function currently validates
only scan numbers as a subset of expected values, which doesn't directly verify
that rows are actually marked as targets. Replace the scan number subset
assertion with a direct check of the target column in psm_df to ensure all rows
have target set to True. Apply the same fix to the similar assertions around
lines 371-379 that also rely on indirect scan number validation instead of
checking the target flag directly.
In `@tests/unit_tests/test_dataset.py`:
- Around line 103-104: Remove the explicit `== True` comparison in the boolean
filter condition on simple_df and use the column directly (e.g.,
simple_df["target"] instead of simple_df["target"] == True), then convert the
regex pattern string in the pytest.raises match parameter to a raw string by
prefixing it with r (e.g., r"[Nn]o decoy" instead of "[Nn]o decoy"). Apply these
same fixes to the similar violations mentioned at lines 118-119.
In `@tests/unit_tests/test_parsers.py`:
- Line 161: The pytest.raises match parameter on the line with the ValueError
assertion is using a regular string instead of a raw string for the regex
pattern, which triggers Ruff RUF043. Change the match parameter from a regular
string to a raw string by adding the r prefix before the opening quote in the
match argument where it contains the pattern "[Nn]o decoy".
- Line 211: Replace the assertion that directly accesses the private attribute
_protein_delim with an assertion using the public protein_delim interface.
Change assert psms._protein_delim == ";" to assert psms.protein_delim == ";" to
test the public API contract instead of internal implementation details. This
change will expose the upstream bug in crema/dataset.py where the protein_delim
property is incorrectly implemented.
---
Outside diff comments:
In `@tests/unit_tests/test_edge_cases.py`:
- Around line 1-205: The test file test_edge_cases.py is failing Black code
formatting checks. Run the Black formatter on this file to automatically fix all
formatting issues. Use the command black tests/unit_tests/test_edge_cases.py to
reformat the entire file according to Black's style guidelines, then commit the
changes. This will resolve the CI lint failure blocking the merge.
In `@tests/unit_tests/test_writers.py`:
- Around line 1-152: The test_writers.py file fails Black formatting checks and
needs to be reformatted to comply with Black's code style standards. Run Black
on this file to automatically fix all formatting issues. This will ensure the
file passes the CI linting checks before merging.
---
Nitpick comments:
In `@tests/unit_tests/test_confidence.py`:
- Around line 273-274: The assertion checking
len(conf.confidence_estimates["peptides"]) >= 0 is tautological because len()
always returns a non-negative value, making the assertion always pass without
validating actual behavior. Replace this assertion with a meaningful check such
as asserting the list is non-empty (len > 0) or validating a specific schema or
value invariant that confirms the confidence_estimates["peptides"] contains the
expected data structure.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b48037b7-317f-4b71-ace0-69a5310af4c2
📒 Files selected for processing (9)
SCALING_PLAN.mdTESTING_PLAN.mdtests/conftest.pytests/unit_tests/test_confidence.pytests/unit_tests/test_dataset.pytests/unit_tests/test_edge_cases.pytests/unit_tests/test_parsers.pytests/unit_tests/test_qvalues.pytests/unit_tests/test_writers.py
- Run Black on test_writers.py and test_edge_cases.py (CI formatting failures) - Fix tautological assert (>= 0 → > 0) in test_pep_fdr_type_with_pairing - Use boolean indexing instead of == True/False in test_dataset.py (E712) - Use raw strings for pytest.raises match= regex (RUF043) in test_dataset.py and test_parsers.py - Improve target/decoy separation assertions: _prettify_tables strips the target column from output DataFrames, so use peptide names to distinguish targets (PEP1-6) from decoys (PEP1D-6D) in the clean_psms fixture Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Local Black was 21.10b0; CI runs the latest stable (26.x). Reformatted 5 files to resolve lint failures: - tests/conftest.py: Remove extra alignment spaces in list literals - tests/unit_tests/test_confidence.py: Fix blank line placement around sections - tests/unit_tests/test_edge_cases.py: Reformat dict literals in parametrize - tests/unit_tests/test_qvalues.py: Break long tuples in parametrize args - tests/unit_tests/test_writers.py: Remove extraneous blank line Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The protein_delim property was doing a column lookup (self[self._protein_delim]) instead of returning the delimiter string. Also update the corresponding test to assert against the public property rather than the private attribute. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Expands Crema’s unit test coverage with a focus on numerical correctness and edge cases across q-value calculation, dataset/parsers validation, confidence estimation, and TXT output writing; also fixes PsmDataset.protein_delim to return the delimiter string correctly.
Changes:
- Added substantial new unit tests for TDC/q-value correctness, monotonicity invariants, dataset/parser error handling, and TXT writer outputs.
- Introduced deterministic
clean_psm_df/clean_psmsfixtures to support hand-computed expected q-values. - Fixed
PsmDataset.protein_delimto returnself._protein_delim(instead of indexing the dataset).
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/unit_tests/test_writers.py | New writer tests validating output file creation, schema, and basic round-trip fidelity. |
| tests/unit_tests/test_qvalues.py | Adds TDC edge-case and monotonicity tests plus direct _fdr2qvalue coverage. |
| tests/unit_tests/test_parsers.py | Adds multi-file concatenation coverage and additional parser error-handling tests. |
| tests/unit_tests/test_edge_cases.py | New cross-module edge-case coverage (competition, shared peptides, empty/degenerate datasets). |
| tests/unit_tests/test_dataset.py | Adds dataset validation/error-handling tests and internal count/pairing checks. |
| tests/unit_tests/test_confidence.py | Replaces type-only stubs with numerical correctness/invariant/parameter validation tests. |
| tests/conftest.py | Adds deterministic fixtures used by multiple new correctness tests. |
| TESTING_PLAN.md | New test roadmap/planning document. |
| SCALING_PLAN.md | New scaling roadmap/planning document. |
| crema/dataset.py | Bug fix: protein_delim property now returns the delimiter string. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+65
to
+67
| def test_mixmax_confidence_desc(simple_psms: PsmDataset): | ||
| """Test that we can compute confidence with `TdcConfidence`""" | ||
| simple_psms.data["x"] = -1.0 * simple_psms.data["x"] | ||
| # Use internal data to avoid the copy() on the .data property | ||
| simple_psms._data["x"] = -1.0 * simple_psms._data["x"] |
- Update tests.yml: trigger on all branches (not just master), test Python 3.9/3.10/3.11, replace flake8 with ruff, upgrade action versions to v4/v5 - Add ruff config to setup.cfg (E/F/W/RUF rules, line-length=88, exclude docs/) - Add __all__ to crema/__init__.py for explicit public API - Fix E711/E712: replace == None / != None / == True / == False with is None / is not None / truthy checks throughout confidence.py and parsers - Fix E722: bare except → except Exception in crema.py - Fix E741: ambiguous variable l → line in params.py - Fix F401: remove unused imports (re, itertools, pandas, numpy, logging, pytest) across parsers, utils, and test files - Fix F841: remove unused local variable assignments in qvalues.py and parsers - Fix RUF005/RUF017: list concatenation → unpacking in confidence.py, dataset.py Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Construct a fresh PsmDataset with negated x scores instead of writing to _data directly, so the test doesn't depend on implementation details. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Comment on lines
69
to
75
| for read_fn in readers: | ||
| try: | ||
| psms = read_fn(args.psm_files) | ||
| break | ||
| except: | ||
| except Exception: | ||
| raise ValueError("Unrecognized file type.") | ||
|
|
|
|
||
| LOGGER.info("Building protein groups...") | ||
| protein_group, pep_to_prot = _group_proteins( | ||
| _protein_group, pep_to_prot = _group_proteins( |
Comment on lines
+133
to
+134
| def test_to_txt_roundtrip_accept(clean_psms, tmp_path): | ||
| """Scores written to file and read back must match in-memory values.""" |
- crema.py: parser loop now continues on failure and raises only after all readers are exhausted, restoring the fallback-to-next-reader behavior - confidence.py: fix typo "infomation" → "information" in error message - test_writers.py: rename test_to_txt_roundtrip_accept to test_to_txt_roundtrip_scores (it tests score values, not accept column) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
.github/workflows/tests.yml (1)
18-18: ⚡ Quick winDisable credential persistence to prevent leakage.
The checkout action defaults to persisting GitHub credentials in the workspace, which could leak through artifacts or logs if accidentally referenced by subsequent steps.
🛡️ Proposed fix to disable credential persistence
steps: - uses: actions/checkout@v4 + with: + persist-credentials: false - name: Set up Python ${{ matrix.python-version }}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/tests.yml at line 18, In the actions/checkout@v4 step, add the persist-credentials parameter set to false to prevent GitHub credentials from being persisted in the workspace. This mitigates the risk of credential leakage through artifacts or logs if subsequent workflow steps accidentally reference sensitive information. Add persist-credentials: false as a configuration parameter to the checkout action step.Source: Linters/SAST tools
crema/crema.py (1)
69-78: ⚡ Quick winAdd debug logging when readers fail.
The updated control flow correctly tries all readers before failing, fixing the previous bug where the first reader failure would prevent subsequent readers from being attempted. However, when a reader fails, the exception is silently caught and the loop continues. This can make debugging difficult when the expected reader fails for an unexpected reason (e.g., corrupt file, missing required column).
Consider adding debug logging to track which readers were attempted and why they failed:
♻️ Suggested enhancement
psms = None for read_fn in readers: try: psms = read_fn(args.psm_files) + logging.info("Successfully parsed file using %s", read_fn.__name__) break - except Exception: + except Exception as e: + logging.debug("Reader %s failed: %s", read_fn.__name__, str(e)) continue🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crema/crema.py` around lines 69 - 78, In the exception handling block within the for loop that iterates through readers, capture the exception being caught in the except Exception clause and add debug logging statements that track which reader was attempted (reference the read_fn to identify which reader is being tried) and log the actual exception details and message. This will help with debugging when a reader fails unexpectedly, such as due to a corrupt file or missing required column.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/tests.yml:
- Line 18: Replace the version tag reference in the actions/checkout action
(currently using `@v4`) with a specific commit hash to prevent supply chain
attacks. Find the exact commit SHA that corresponds to v4 from the
actions/checkout repository, then update the uses statement to reference that
commit hash instead of the version tag, and add a comment on the same line
indicating which version the hash corresponds to (e.g., # v4). Apply the same
fix to any other action uses statements that reference version tags instead of
commit hashes, as indicated by the "Also applies to" note.
---
Nitpick comments:
In @.github/workflows/tests.yml:
- Line 18: In the actions/checkout@v4 step, add the persist-credentials
parameter set to false to prevent GitHub credentials from being persisted in the
workspace. This mitigates the risk of credential leakage through artifacts or
logs if subsequent workflow steps accidentally reference sensitive information.
Add persist-credentials: false as a configuration parameter to the checkout
action step.
In `@crema/crema.py`:
- Around line 69-78: In the exception handling block within the for loop that
iterates through readers, capture the exception being caught in the except
Exception clause and add debug logging statements that track which reader was
attempted (reference the read_fn to identify which reader is being tried) and
log the actual exception details and message. This will help with debugging when
a reader fails unexpectedly, such as due to a corrupt file or missing required
column.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 529b5d23-eb0c-4724-86be-5a752f00add0
📒 Files selected for processing (24)
.github/workflows/tests.ymlPHASE1_SUMMARY.mdcrema/__init__.pycrema/confidence.pycrema/crema.pycrema/dataset.pycrema/params.pycrema/parsers/comet.pycrema/parsers/msamanda.pycrema/parsers/msfragger.pycrema/parsers/msgf.pycrema/parsers/mztab.pycrema/parsers/tide.pycrema/parsers/txt.pycrema/qvalues.pycrema/utils.pypyproject.tomlsetup.cfgtests/system_tests/test_cli.pytests/unit_tests/test_confidence.pytests/unit_tests/test_edge_cases.pytests/unit_tests/test_parsers.pytests/unit_tests/test_qvalues.pytests/unit_tests/test_writers.py
💤 Files with no reviewable changes (4)
- crema/parsers/msgf.py
- crema/utils.py
- crema/qvalues.py
- tests/unit_tests/test_qvalues.py
✅ Files skipped from review due to trivial changes (8)
- crema/parsers/txt.py
- crema/init.py
- PHASE1_SUMMARY.md
- pyproject.toml
- crema/parsers/tide.py
- crema/parsers/mztab.py
- crema/params.py
- crema/parsers/msfragger.py
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/unit_tests/test_writers.py
- tests/unit_tests/test_parsers.py
- tests/unit_tests/test_edge_cases.py
- tests/unit_tests/test_confidence.py
| - name: Test with pytest | ||
| run: | | ||
| pytest | ||
| - uses: actions/checkout@v4 |
There was a problem hiding this comment.
Pin actions to commit hashes to prevent supply chain attacks.
Using version tags (v4, v5) instead of commit hashes allows an attacker who compromises the upstream action repository to push malicious code under the same tag. Pin to specific commit SHAs and include a comment with the version for maintainability.
🔒 Proposed fix to pin actions
steps:
- - uses: actions/checkout@v4
+ - uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2
- name: Set up Python ${{ matrix.python-version }}
- uses: actions/setup-python@v5
+ uses: actions/setup-python@0b93645e9fea7318ecaed2b359559ac225c90a2b # v5.3.0
with:Also applies to: 20-20
🧰 Tools
🪛 zizmor (1.25.2)
[warning] 18-18: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[error] 18-18: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/tests.yml at line 18, Replace the version tag reference in
the actions/checkout action (currently using `@v4`) with a specific commit hash to
prevent supply chain attacks. Find the exact commit SHA that corresponds to v4
from the actions/checkout repository, then update the uses statement to
reference that commit hash instead of the version tag, and add a comment on the
same line indicating which version the hash corresponds to (e.g., # v4). Apply
the same fix to any other action uses statements that reference version tags
instead of commit hashes, as indicated by the "Also applies to" note.
Source: Linters/SAST tools
Captures the exception from each failed reader so that users running with --debug can see which parsers were tried and why they failed, without changing behavior for normal use. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add ~47 new tests across 6 files covering gaps identified in TESTING_PLAN.md:
Summary by CodeRabbit
Documentation
Bug Fixes
PsmDataset.protein_delimto correctly return the configured delimiter.New Features
__all__) for thecremapackage.Tests