tests: remove duplicate and dead test cases#8896
Merged
Merged
Conversation
Several parametrized test lists contained byte-identical entries that asserted the same thing twice, contributing no extra coverage: - losses/test_unified_focal_loss.py: both TEST_CASES entries identical - inferers/test_sliding_window_inference.py: duplicate 3D small-roi case - losses/test_dice_loss.py: sigmoid case repeated - losses/test_generalized_dice_loss.py: sigmoid case repeated - transforms/utility/test_apply_transform_to_pointsd.py: duplicate entry Signed-off-by: Soumya Snigdha Kundu <soumya_snigdha.kundu@kcl.ac.uk>
apps/test_download_url_yandex.py: remove test_verify, which depends on an unstable external Yandex URL and is permanently @unittest.skip-ed, so it never runs in CI. The error path remains covered by test_verify_error. Note: test_smartcachedataset::test_set_data was left skipped on purpose. Issue Project-MONAI#5660 (the smartcache thread hanging for hours on CI) was closed as stale with no fix, so the intermittent hang risk likely remains. Signed-off-by: Soumya Snigdha Kundu <soumya_snigdha.kundu@kcl.ac.uk>
Contributor
📝 WalkthroughWalkthroughThis PR removes test coverage across five test modules. The changes eliminate one unreliable Yandex download test and its constant; prune one 3D sliding-window inference scenario; remove single parameterized cases from GeneralizedDiceLoss, UnifiedFocalLoss, and point-transform coordinate tests. All changes are deletions from test data or test methods with no alterations to core logic. Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes 🚥 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}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/losses/test_unified_focal_loss.py (1)
22-30: ⚡ Quick winTest coverage now minimal after duplicate removal.
Only one test case remains, covering perfect prediction (loss = 0.0). Loss functions need diverse scenarios: imperfect predictions, non-zero loss values, parameter variations (weight, gamma, delta), edge cases.
Suggested additional test cases
TEST_CASES = [ [ # shape: (2, 1, 2, 2), (2, 1, 2, 2) - perfect prediction { "y_pred": torch.tensor([[[[1.0, 0], [0, 1.0]]], [[[1.0, 0], [0, 1.0]]]]), "y_true": torch.tensor([[[[1.0, 0], [0, 1.0]]], [[[1.0, 0], [0, 1.0]]]]), }, 0.0, ], [ # imperfect prediction - partial mismatch { "y_pred": torch.tensor([[[[0.8, 0.2], [0.3, 0.9]]], [[[0.7, 0.1], [0.2, 0.85]]]]), "y_true": torch.tensor([[[[1.0, 0], [0, 1.0]]], [[[1.0, 0], [0, 1.0]]]]), }, # expected_val would need to be calculated based on loss formula ], # Add cases for different weight, gamma, delta parameters ]🤖 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/losses/test_unified_focal_loss.py` around lines 22 - 30, Replace the single perfect-prediction entry in TEST_CASES with multiple diverse cases: keep the existing perfect case, add at least one imperfect-prediction case (modify y_pred vs y_true to produce non-zero loss), add edge cases (all-zeros/constant predictions, class-imbalance patterns), and add cases that exercise different loss parameters (pass explicit weight, gamma, delta variations to the UnifiedFocalLoss call). Ensure each new case in TEST_CASES references the same keys ("y_pred", "y_true") and provides either a precomputed expected value (for deterministic settings) or mark it for approximate/asserted range checks when exact numeric values are complex; use the symbols TEST_CASES, y_pred, y_true and the loss parameters weight/gamma/delta to locate where to modify tests.
🤖 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.
Nitpick comments:
In `@tests/losses/test_unified_focal_loss.py`:
- Around line 22-30: Replace the single perfect-prediction entry in TEST_CASES
with multiple diverse cases: keep the existing perfect case, add at least one
imperfect-prediction case (modify y_pred vs y_true to produce non-zero loss),
add edge cases (all-zeros/constant predictions, class-imbalance patterns), and
add cases that exercise different loss parameters (pass explicit weight, gamma,
delta variations to the UnifiedFocalLoss call). Ensure each new case in
TEST_CASES references the same keys ("y_pred", "y_true") and provides either a
precomputed expected value (for deterministic settings) or mark it for
approximate/asserted range checks when exact numeric values are complex; use the
symbols TEST_CASES, y_pred, y_true and the loss parameters weight/gamma/delta to
locate where to modify tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: caf5c6f5-2344-48f2-8ab8-c2030b22206b
📒 Files selected for processing (6)
tests/apps/test_download_url_yandex.pytests/inferers/test_sliding_window_inference.pytests/losses/test_dice_loss.pytests/losses/test_generalized_dice_loss.pytests/losses/test_unified_focal_loss.pytests/transforms/utility/test_apply_transform_to_pointsd.py
💤 Files with no reviewable changes (5)
- tests/inferers/test_sliding_window_inference.py
- tests/transforms/utility/test_apply_transform_to_pointsd.py
- tests/apps/test_download_url_yandex.py
- tests/losses/test_generalized_dice_loss.py
- tests/losses/test_dice_loss.py
Member
|
@coderabbitai where else in the unit tests are there duplicate test cases like those removed in this PR? |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
python3 - <<'EOF'
import ast
import os
import sys
from pathlib import Path
def find_duplicates_in_list(elements):
seen = []
dupes = []
for i, el in enumerate(elements):
key = ast.dump(el)
for j, (prev_key, prev_idx) in enumerate(seen):
if key == prev_key:
dupes.append((prev_idx, i))
break
else:
seen.append((key, i))
return dupes
results = []
for path in Path("tests").rglob("*.py"):
try:
src = path.read_text(errors="replace")
tree = ast.parse(src, filename=str(path))
except SyntaxError:
continue
for node in ast.walk(tree):
if not isinstance(node, ast.Assign):
continue
for target in node.targets:
if not isinstance(target, ast.Name):
continue
name = target.id
if "TEST" not in name.upper() and "CASES" not in name.upper() and "PARAMS" not in name.upper():
continue
val = node.value
if not isinstance(val, ast.List):
continue
elems = val.elts
if len(elems) < 2:
continue
dupes = find_duplicates_in_list(elems)
if dupes:
results.append((str(path), name, dupes))
for fpath, varname, dupes in sorted(results):
for (i, j) in dupes:
print(f"{fpath} {varname} indices {i} and {j} are identical")
if not results:
print("No duplicates found.")
EOFLength of output: 16409
apps/
bundle/
data/
handlers/
inferers/
metrics/
networks/
transforms/
The |
Contributor
Author
|
I'll make a follow up PR for this, Thank you for pointing that out! |
2 tasks
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
2 participants
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.
Description
Test-only cleanup. Removes byte-identical duplicate cases and one permanently-skipped test. No production code or test logic changes.
Changes
Duplicate within-file parametrized cases (identical entries that asserted the same thing twice, so removal changes nothing covered):
tests/losses/test_unified_focal_loss.py: bothTEST_CASESentries were identicaltests/inferers/test_sliding_window_inference.py: duplicate3D small roicasetests/losses/test_dice_loss.py:sigmoidcase repeatedtests/losses/test_generalized_dice_loss.py:sigmoidcase repeatedtests/transforms/utility/test_apply_transform_to_pointsd.py: duplicate entryDead test removed:
tests/apps/test_download_url_yandex.py: removedtest_verifyand its now-unusedYANDEX_MODEL_URL. It was permanently@unittest.skip-ed ("data source unstable") and hits an external Yandex URL, so it never runs in CI. The error path stays covered bytest_verify_error.Types of changes