Conversation
There was a problem hiding this comment.
Pull request overview
Adds a large “gap-filling” test module intended to drive branch coverage to 100% (Issue #660), plus a few targeted # pragma: no cover annotations for paths considered unreachable or impractical to hit.
Changes:
- Added
test_missing_coverage.pywith many targeted tests for previously-uncovered branches across managers, query/translation, formsets, showfields, and contrib integrations. - Marked a few specific lines as excluded from coverage via
# pragma: no cover. - Added/expanded tests for edge cases (e.g., stale content types, deferred loading translation errors, optional contrib modules).
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
src/polymorphic/utils.py |
Adds # pragma: no cover to the (effectively unreachable) final return in _compare_mro. |
src/polymorphic/tests/test_missing_coverage.py |
New comprehensive “coverage completion” test suite targeting specific uncovered branches, including contrib integrations. |
src/polymorphic/showfields.py |
Adds # pragma: no cover to the diamond-inheritance deduplication guard branch. |
src/polymorphic/formsets/models.py |
Adds # pragma: no cover to a queryset-data model selection line in _construct_form. |
Comments suppressed due to low confidence (1)
src/polymorphic/tests/test_missing_coverage.py:1872
- This test currently ends right after calling
s.is_valid()and doesn’t assert the expected outcome or any observable effect of thehasattr(self, "_validated_data")false branch. Add assertions (e.g., thatresultisTrue/Falseas intended, and that_validated_datais absent/not updated while errors are still propagated) so the branch is actually verified rather than just executed opportunistically.
with mock.patch.object(drf_serializers.Serializer, "is_valid", patched_is_valid):
result = s.is_valid()
# Result depends on child validation (child is valid, so True)
# But _validated_data was deleted before the check, so branch 98->101 executes
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @pytest.mark.skipif( | ||
| not (lambda: __import__("extra_views", fromlist=["ModelFormSetView"]))(), | ||
| reason="extra_views not installed", | ||
| ) |
There was a problem hiding this comment.
Same issue as the DRF skip markers above: using __import__(...) inside the skipif condition will raise ImportError during collection when extra_views isn’t installed, instead of skipping. Switch the condition to a non-throwing check (e.g., importlib.util.find_spec("extra_views") is None).
| @pytest.mark.skipif( | ||
| not (lambda: __import__("rest_framework", fromlist=["serializers"]))(), | ||
| reason="djangorestframework not installed", | ||
| ) |
There was a problem hiding this comment.
The pytest.mark.skipif condition uses a direct __import__(...) call; when the optional dependency is not installed, this will raise ImportError during test collection instead of skipping. Use a safe availability check (e.g., importlib.util.find_spec(...) is None, or pytest.importorskip(...) inside the tests) and set the skip condition based on that.
| # Patch the connection features to return 0 for max_query_params | ||
| with patch("django.db.connections") as mock_connections: | ||
| mock_features = type("MockFeatures", (), {"max_query_params": 0})() | ||
| mock_connection = type("MockConn", (), {"features": mock_features})() | ||
| mock_connections.__getitem__ = lambda self, key: mock_connection | ||
|
|
||
| # Can't easily use this - instead verify the existing behavior | ||
| # The default is 2000 - just verify it works | ||
| results = list(qs) | ||
| assert len(results) >= 2 |
There was a problem hiding this comment.
This test patches django.db.connections, but _polymorphic_iterator reads connections from the already-imported polymorphic.query module (from django.db import connections). Patching django.db.connections won’t affect polymorphic.query.connections, so the test doesn’t reliably exercise the max_query_params == 0 branch it describes. Patch polymorphic.query.connections (or patch the specific connections[alias].features.max_query_params used by polymorphic.query) so the branch is deterministically covered.
| with patch.object(ContentType.objects, "get_for_id", side_effect=patched_get_for_id): | ||
| result = qs._get_real_instances([b_obj]) | ||
| # With None model class, object ends up as None in resultlist and gets filtered out | ||
|
|
There was a problem hiding this comment.
test_real_concrete_class_is_none doesn’t assert anything about the result or side effects (it exits the patch context without checks), so it won’t fail even if the targeted branch regresses or isn’t executed. Add at least one assertion that verifies the expected behavior (e.g., returned list is empty and/or no exception is raised) to make this a meaningful regression test.
| forms = formset.forms | ||
| # The form types come from the queryset_data items | ||
| assert len(forms) == 2 | ||
| # At least one form for each type | ||
| field1s = {form.instance.__class__.__name__ for form in forms if form.instance.pk} | ||
| # queryset_data drives the model for each form | ||
| assert len(forms) == 2 |
There was a problem hiding this comment.
This test doesn’t validate the behavior it claims to cover: it calculates field1s but never asserts anything about which child form/model was chosen, and it repeats assert len(forms) == 2. As written it would still pass if the queryset-derived model selection is wrong. Add assertions that at least one constructed form corresponds to Model2B (e.g., has field2 and/or form._meta.model is Model2B) and one corresponds to Model2A.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #876 +/- ##
==========================================
+ Coverage 95.78% 99.10% +3.32%
==========================================
Files 28 28
Lines 1896 1892 -4
Branches 273 270 -3
==========================================
+ Hits 1816 1875 +59
+ Misses 48 12 -36
+ Partials 32 5 -27 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
closes #660