Feature/improve api test coverage #2527
Conversation
- Add comprehensive tests for sponsor endpoint list and get operations - Fix pagination_test.py: add missing items assertion in test_paginate_exact_multiple_pages - Fix chapter_test.py: remove duplicate test_get_chapter_uppercase_prefix method Achieves 100% coverage for sponsor endpoint
Summary by CodeRabbitRelease Notes
WalkthroughAdds extensive unit tests across API decorators, internal mutations, and v0 REST endpoints; adjusts several v0 endpoint implementations to add deterministic tie-breakers and case-insensitive prefix handling; exports multiple get_/list_ helpers used by tests; and refines cache decorator behavior tests (HEAD/PATCH/TTL/prefix). Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
backend/tests/apps/api/rest/v0/chapter_test.py (1)
40-45: Fix datetime parsing for UTC timestamps.
datetime.fromisoformatrejects values that end with'Z', so these assertions blow up withValueErrorbefore the test even runs. Normalize the suffix to an explicit UTC offset (or use a helper) before parsing. One option:- assert chapter.created_at == datetime.fromisoformat(chapter_data["created_at"]) + assert chapter.created_at == datetime.fromisoformat( + chapter_data["created_at"].replace("Z", "+00:00") + ) … - assert chapter.updated_at == datetime.fromisoformat(chapter_data["updated_at"]) + assert chapter.updated_at == datetime.fromisoformat( + chapter_data["updated_at"].replace("Z", "+00:00") + )backend/tests/apps/api/rest/v0/issue_test.py (1)
35-40: Handle the'Z'suffix before callingfromisoformat.These assertions call
datetime.fromisoformatwith ISO strings ending in'Z', which raisesValueErrorin CPython. Normalize the string (e.g., replace'Z'with'+00:00') or parse with a helper so the tests run.- assert issue.created_at == datetime.fromisoformat(issue_data["created_at"]) + assert issue.created_at == datetime.fromisoformat( + issue_data["created_at"].replace("Z", "+00:00") + ) … - assert issue.updated_at == datetime.fromisoformat(issue_data["updated_at"]) + assert issue.updated_at == datetime.fromisoformat( + issue_data["updated_at"].replace("Z", "+00:00") + )backend/tests/apps/api/rest/v0/release_test.py (1)
34-38: Normalize ISO strings before parsing.
datetime.fromisoformatdoes not accept a trailing'Z', so these assertions will raise immediately. Convert'Z'to an explicit offset (e.g.,'+00:00') or use a helper before callingfromisoformat.- assert release.created_at == datetime.fromisoformat(release_data["created_at"]) + assert release.created_at == datetime.fromisoformat( + release_data["created_at"].replace("Z", "+00:00") + ) … - assert release.published_at == datetime.fromisoformat(release_data["published_at"]) + assert release.published_at == datetime.fromisoformat( + release_data["published_at"].replace("Z", "+00:00") + )backend/tests/apps/api/rest/v0/milestone_test.py (1)
56-68: Replace directfromisoformatcalls on'Z'timestamps.Every call here feeds
datetime.fromisoformata string ending with'Z', which CPython refuses. The test suite will crash before assertions. Strip or translate the suffix first (e.g.,value.replace("Z", "+00:00")) or add a small helper so each parse goes through a valid ISO string.- assert milestone.created_at == datetime.fromisoformat(milestone_data["created_at"]) + assert milestone.created_at == datetime.fromisoformat( + milestone_data["created_at"].replace("Z", "+00:00") + ) … - assert milestone.due_on == datetime.fromisoformat(milestone_data["due_on"]) + assert milestone.due_on == datetime.fromisoformat( + milestone_data["due_on"].replace("Z", "+00:00") + ) … - assert milestone.updated_at == datetime.fromisoformat(milestone_data["updated_at"]) + assert milestone.updated_at == datetime.fromisoformat( + milestone_data["updated_at"].replace("Z", "+00:00") + )Apply the same fix to every other
fromisoformatcall in this file to keep the tests runnable.
🧹 Nitpick comments (4)
backend/tests/apps/api/rest/v0/pagination_test.py (1)
1-293: Excellent test coverage with comprehensive edge case handling.This test suite provides thorough coverage of the
CustomPaginationclass, including:
- Standard pagination scenarios (first, middle, last pages)
- Edge cases (empty queryset, single page, out-of-range)
- Boundary conditions (partial last page, exact multiples)
- Validation of Http404 behavior with proper error messages
- Verification of offset and total_pages calculations
The tests correctly mock Django queryset behavior and verify both the returned metadata and the queryset slicing calls.
Optional: Reduce setup duplication with pytest fixtures.
Consider extracting common setup code into pytest fixtures to improve maintainability:
import pytest from unittest.mock import MagicMock import pytest from django.http import Http404 from apps.api.rest.v0.pagination import CustomPagination @pytest.fixture def paginator(): """Fixture for CustomPagination instance.""" return CustomPagination() @pytest.fixture def mock_queryset(): """Fixture for mock queryset.""" queryset = MagicMock() queryset.__getitem__ = MagicMock(return_value=[]) return queryset @pytest.fixture def mock_pagination(): """Fixture for mock pagination input.""" pagination = MagicMock() pagination.page = 1 pagination.page_size = 10 return pagination class TestCustomPagination: """Test cases for CustomPagination class.""" def test_paginate_first_page(self, paginator, mock_queryset, mock_pagination): """Test pagination on the first page.""" mock_queryset.count.return_value = 25 mock_queryset.__getitem__.return_value = ["item1", "item2", "item3"] result = paginator.paginate_queryset(mock_queryset, mock_pagination) assert result["current_page"] == 1 # ... rest of assertionsThis reduces boilerplate while keeping tests readable. However, the current approach of explicit setup in each test is also perfectly acceptable and may be preferred for its explicitness.
backend/tests/apps/api/rest/v0/project_test.py (2)
48-87: Excellent test coverage for ordering behavior!The tests correctly verify both custom and default ordering logic, including the multi-field ordering chain (
-stars_count,-forks_count). The mock setup accurately reflects the ORM call chain, and assertions validate both the ordering parameters and filter application.Consider adding an edge case test for empty results to ensure the endpoint handles empty querysets gracefully:
@patch("apps.api.rest.v0.project.ProjectModel.active_projects") def test_list_projects_empty_results(self, mock_active_projects): """Test listing projects returns empty list when no projects exist.""" mock_request = MagicMock() mock_filters = MagicMock() mock_ordered = MagicMock() mock_active_projects.order_by.return_value = mock_ordered mock_filters.filter.return_value = [] result = list_projects(mock_request, filters=mock_filters, ordering=None) assert result == []
90-139: Well-structured tests for project retrieval logic!The tests thoroughly validate the prefix-handling logic and error cases. The mock setup correctly simulates the ORM filter chain, and the 404 test properly verifies both the status code and response message.
Consider adding an edge case test to verify the case sensitivity behavior of the prefix check:
@patch("apps.api.rest.v0.project.ProjectModel.active_projects") def test_get_project_mixed_case_prefix(self, mock_active_projects): """Test that prefix check is case-sensitive.""" mock_request = MagicMock() mock_filter = MagicMock() mock_project = MagicMock() mock_active_projects.filter.return_value = mock_filter mock_filter.first.return_value = mock_project # "WWW-PROJECT-" should not be recognized as prefix due to case sensitivity result = get_project(mock_request, project_id="WWW-PROJECT-Nest") # Should add prefix since startswith is case-sensitive mock_active_projects.filter.assert_called_once_with( key__iexact="www-project-WWW-PROJECT-Nest" )This would document the expected behavior when users provide different casing for the prefix.
backend/tests/apps/api/rest/v0/committee_test.py (1)
133-147: Consider making the prefix check case-insensitive for consistency and better UX.Stack Overflow APIs are canonically lowercase and case-insensitive, which makes life easiest for clients while having a clear default. The current implementation has inconsistent case handling: the prefix check uses case-sensitive
startswith(), but the database query uses case-insensitivekey__iexact.The test documents that uppercase input like
"WWW-COMMITTEE-Project"results in double-prefixing to"www-committee-WWW-COMMITTEE-Project". While the case-insensitive database lookup still finds the resource (or returns 404), normalizing the prefix check would prevent the double-prefixing and provide clearer behavior.All existing committee keys in the codebase use lowercase
"www-committee-"consistently. Consider updating the check tocommittee_id.lower().startswith("www-committee-")to handle variations like"WWW-COMMITTEE-"or"Www-Committee-", and update the test to validate the normalized behavior.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
backend/tests/apps/api/decorators/cache_test.py(2 hunks)backend/tests/apps/api/internal/mutations/api_key_test.py(2 hunks)backend/tests/apps/api/rest/v0/chapter_test.py(2 hunks)backend/tests/apps/api/rest/v0/committee_test.py(2 hunks)backend/tests/apps/api/rest/v0/event_test.py(2 hunks)backend/tests/apps/api/rest/v0/issue_test.py(2 hunks)backend/tests/apps/api/rest/v0/label_test.py(2 hunks)backend/tests/apps/api/rest/v0/member_test.py(2 hunks)backend/tests/apps/api/rest/v0/milestone_test.py(2 hunks)backend/tests/apps/api/rest/v0/organization_test.py(2 hunks)backend/tests/apps/api/rest/v0/pagination_test.py(1 hunks)backend/tests/apps/api/rest/v0/project_test.py(2 hunks)backend/tests/apps/api/rest/v0/release_test.py(2 hunks)backend/tests/apps/api/rest/v0/repository_test.py(2 hunks)backend/tests/apps/api/rest/v0/sponsor_test.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (15)
backend/tests/apps/api/decorators/cache_test.py (1)
backend/apps/api/decorators/cache.py (1)
cache_response(19-56)
backend/tests/apps/api/rest/v0/issue_test.py (2)
backend/apps/api/rest/v0/issue.py (4)
IssueDetail(34-37)get_issue(106-120)list_issues(73-92)Issue(30-31)backend/apps/github/models/issue.py (1)
Issue(18-213)
backend/tests/apps/api/rest/v0/repository_test.py (2)
backend/apps/api/rest/v0/repository.py (4)
RepositoryDetail(31-34)get_repository(89-101)list_repository(61-75)Repository(27-28)backend/apps/github/models/repository.py (1)
Repository(25-352)
backend/tests/apps/api/rest/v0/milestone_test.py (1)
backend/apps/api/rest/v0/milestone.py (4)
MilestoneDetail(35-41)get_milestone(110-124)list_milestones(77-94)Milestone(31-32)
backend/tests/apps/api/rest/v0/project_test.py (1)
backend/apps/api/rest/v0/project.py (3)
ProjectDetail(39-42)get_project(95-107)list_projects(68-81)
backend/tests/apps/api/rest/v0/sponsor_test.py (1)
backend/apps/api/rest/v0/sponsor.py (3)
SponsorDetail(32-38)get_sponsor(97-105)list_sponsors(74-83)
backend/tests/apps/api/rest/v0/pagination_test.py (1)
backend/apps/api/rest/v0/pagination.py (2)
CustomPagination(10-58)paginate_queryset(31-58)
backend/tests/apps/api/rest/v0/organization_test.py (1)
backend/apps/api/rest/v0/organization.py (3)
OrganizationDetail(32-36)get_organization(90-101)list_organization(63-76)
backend/tests/apps/api/rest/v0/member_test.py (1)
backend/apps/api/rest/v0/member.py (3)
MemberDetail(34-44)get_member(94-102)list_members(71-80)
backend/tests/apps/api/rest/v0/label_test.py (1)
backend/apps/api/rest/v0/label.py (1)
list_label(51-66)
backend/tests/apps/api/rest/v0/release_test.py (2)
backend/apps/api/rest/v0/release.py (4)
ReleaseDetail(32-35)get_release(107-121)list_release(68-93)Release(28-29)backend/apps/github/models/release.py (1)
Release(10-135)
backend/tests/apps/api/rest/v0/event_test.py (1)
backend/apps/api/rest/v0/event.py (3)
EventDetail(33-36)get_event(75-83)list_events(53-61)
backend/tests/apps/api/rest/v0/chapter_test.py (1)
backend/apps/api/rest/v0/chapter.py (3)
ChapterDetail(37-41)get_chapter(87-99)list_chapters(64-73)
backend/tests/apps/api/internal/mutations/api_key_test.py (2)
backend/tests/apps/api/internal/queries/api_key_test.py (1)
mock_info(8-16)backend/apps/api/internal/mutations/api_key.py (2)
create_api_key(44-85)CreateApiKeyResult(29-36)
backend/tests/apps/api/rest/v0/committee_test.py (1)
backend/apps/api/rest/v0/committee.py (3)
CommitteeDetail(37-40)get_chapter(79-94)list_committees(57-65)
🔇 Additional comments (24)
backend/tests/apps/api/internal/mutations/api_key_test.py (2)
36-95: Excellent validation test coverage!These four new test methods comprehensively cover the input validation paths for API key creation:
- Empty and whitespace-only names
- Name length exceeding the maximum
- Expiry dates in the past
The tests are well-structured with clear docstrings, appropriate assertions on all result fields, and correctly use mocking (including the
@patchdecorator forMAX_WORD_LENGTH). This significantly improves coverage of the validation logic.
146-146: Good improvement to test assertion specificity.Explicitly retrieving the user and verifying the arguments passed to
ApiKey.createmakes this test more thorough and consistent with the other tests in the suite (test_create_api_key_successandtest_create_api_key_limit_reached).Also applies to: 152-152
backend/tests/apps/api/rest/v0/label_test.py (3)
1-5: LGTM: Imports are appropriate.The imports correctly include the necessary mocking utilities and the
list_labelfunction being tested.
35-53: LGTM: Well-structured test for the ordering path.The test correctly mocks the queryset chain and verifies that
order_byis called with the expected ordering parameter. The assertions comprehensively validate both the method call chain and the final return value.
55-71: LGTM: Correctly tests the no-ordering path.The test properly verifies that when
ordering=None, theorder_bymethod is not invoked and the filtered queryset is returned directly. Together with the previous test, this achieves complete coverage of the conditional ordering logic.backend/tests/apps/api/rest/v0/project_test.py (1)
2-3: LGTM! Imports are appropriate for unit testing.The imported modules and functions are necessary for the test implementation: HTTPStatus for status code verification, mock utilities for ORM chain mocking, and the functions under test.
Also applies to: 7-7
backend/tests/apps/api/rest/v0/committee_test.py (3)
10-42: LGTM! Good test coverage for the serializer.The parametrized test provides good coverage for the CommitteeDetail serializer with varied data, and the MockCommittee approach properly simulates ORM behavior.
45-72: LGTM! Comprehensive ordering tests.The tests properly verify both custom and default ordering behavior with appropriate mocks and assertions.
78-130: Excellent test coverage for the endpoint logic.The tests thoroughly cover the key scenarios:
- Committee ID with and without the prefix
- 404 handling when committee not found
- Proper use of mocks to isolate the endpoint logic
- Clear assertions on both the queryset filtering and response handling
backend/tests/apps/api/rest/v0/repository_test.py (3)
11-35: LGTM! Schema validation tests are well-structured.The parametrized approach efficiently tests schema field mapping with multiple data sets.
38-120: LGTM! Comprehensive list endpoint testing.The tests thoroughly cover ordering options and filtering scenarios, with proper verification of ORM method calls and return values.
123-162: LGTM! Proper 404 error handling verification.Both found and not-found scenarios are tested correctly, including verification of the error message content.
backend/tests/apps/api/rest/v0/member_test.py (3)
10-51: LGTM! Good defensive testing for sensitive fields.The explicit check that email is excluded (line 51) is excellent practice for ensuring sensitive data doesn't leak through API responses.
54-89: LGTM! Ordering and filtering logic properly validated.Tests correctly verify the order_by → filters.filter chain matches the implementation.
92-125: LGTM! Get endpoint tests cover both success and error paths.The tests properly verify the filter().first() pattern and 404 error responses.
backend/tests/apps/api/rest/v0/sponsor_test.py (3)
9-68: LGTM! Thorough schema validation coverage.Testing both complete and minimal data scenarios ensures the schema handles optional fields correctly.
71-123: LGTM! Comprehensive ordering scenarios covered.The inclusion of reverse ordering testing (lines 109-123) provides more thorough coverage than similar tests in other files.
126-159: LGTM! Get endpoint properly tested.Tests verify correct filter usage and 404 error handling.
backend/tests/apps/api/rest/v0/event_test.py (3)
10-39: LGTM! Schema validation test is correct.The module-level parametrized test is a valid pytest pattern and properly validates schema fields.
42-69: LGTM! Correctly validates multi-field ordering.Tests properly verify that both primary and secondary sort fields are passed to order_by, matching the implementation.
72-105: LGTM! Get endpoint tests cover both scenarios.Tests properly verify found/not-found cases with correct 404 error handling.
backend/tests/apps/api/rest/v0/organization_test.py (3)
14-44: LGTM! Schema validation test is correct.Parametrized test efficiently validates schema field mapping.
47-88: LGTM! Complex ORM chain properly tested.Tests correctly validate the three-step query chain with is_owasp_related_organization filter, ordering, and additional filtering.
91-130: LGTM! Get endpoint correctly validates dual-filter logic.Tests properly verify that both is_owasp_related_organization and login__iexact filters are applied, with correct 404 error handling.
|
Hi @arkid15r ,Pr is ready for review. Thanks! |
There was a problem hiding this comment.
@EclipseAditya yes, you can fix the related issues you found while working on the tests right in this PR.
Thank you!
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
backend/apps/api/rest/v0/event.py (1)
61-64: Sophisticated ordering logic with semantic correctness.The implementation ensures that when sorting by
end_date, the secondary sort bystart_datematches the same direction (ascending/descending), which makes semantic sense for chronological data. The finalidtie-breaker maintains determinism.While correct, the complexity is higher than other endpoints in this PR. Consider adding a brief inline comment explaining why end_date ordering gets special treatment with matching start_date direction.
For clarity, consider adding a comment like:
# When ordering by end_date, use start_date as secondary sort in the same direction # to maintain chronological consistency. For other orderings, use default tie-breakers. if ordering and ordering.lstrip("-") == "end_date": secondary = "-start_date" if ordering.startswith("-") else "start_date" return EventModel.objects.order_by(ordering, secondary, "id") return EventModel.objects.order_by(ordering or "-start_date", "-end_date", "id")backend/tests/apps/api/rest/v0/issue_test.py (2)
43-171: LGTM! Comprehensive filtering and ordering tests.The test suite effectively validates all filtering options (organization, repository, state) and ordering behaviors. The mocking correctly simulates the ORM chain and verifies the expected method calls with appropriate arguments.
Consider adding a test for combined filters (e.g., organization + repository + state together) to ensure they work correctly in combination:
@patch("apps.api.rest.v0.issue.IssueModel.objects") def test_list_issues_with_multiple_filters(self, mock_objects): """Test listing issues with multiple filters combined.""" mock_request = MagicMock() mock_filters = MagicMock() mock_filters.organization = "OWASP" mock_filters.repository = "Nest" mock_filters.state = "open" mock_select_related = MagicMock() mock_filtered = MagicMock() mock_objects.select_related.return_value = mock_select_related mock_select_related.filter.return_value = mock_filtered mock_filtered.filter.return_value = mock_filtered mock_filtered.order_by.return_value = MagicMock() list_issues(mock_request, filters=mock_filters, ordering=None) # Verify filter was called for each filter in sequence assert mock_select_related.filter.call_count >= 1
173-214: LGTM! Good coverage of both success and error cases.The tests correctly validate the
get_issueendpoint for both found and not-found scenarios, including proper 404 response handling.The inline import of
IssueModelon line 198 works but could be moved to the top of the file for consistency. Alternatively, you could catch the exception more generically:# Option 1: Move import to top of file (line 7) from apps.github.models.issue import Issue as IssueModel # Then on line 202, no import needed mock_objects.get.side_effect = IssueModel.DoesNotExistbackend/apps/api/rest/v0/project.py (1)
102-104: LGTM! Consider extracting the prefix handling pattern.The case-insensitive prefix detection is implemented correctly and consistently with the chapter endpoint.
Since this pattern now appears in three files (chapter.py, project.py, committee.py) with only the prefix string varying, consider extracting it into a shared helper function to reduce duplication:
def normalize_entity_key(entity_id: str, prefix: str) -> str: """Normalize entity ID by adding prefix if not already present (case-insensitive check).""" return entity_id if entity_id.lower().startswith(prefix.lower()) else f"{prefix}{entity_id}"This would make future maintenance easier and ensure consistent behavior across all endpoints.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
backend/apps/api/rest/v0/chapter.py(1 hunks)backend/apps/api/rest/v0/committee.py(1 hunks)backend/apps/api/rest/v0/event.py(1 hunks)backend/apps/api/rest/v0/issue.py(1 hunks)backend/apps/api/rest/v0/milestone.py(2 hunks)backend/apps/api/rest/v0/project.py(1 hunks)backend/apps/api/rest/v0/release.py(1 hunks)backend/apps/api/rest/v0/repository.py(1 hunks)backend/tests/apps/api/rest/v0/chapter_test.py(2 hunks)backend/tests/apps/api/rest/v0/committee_test.py(2 hunks)backend/tests/apps/api/rest/v0/event_test.py(2 hunks)backend/tests/apps/api/rest/v0/issue_test.py(2 hunks)backend/tests/apps/api/rest/v0/milestone_test.py(2 hunks)backend/tests/apps/api/rest/v0/project_test.py(2 hunks)backend/tests/apps/api/rest/v0/release_test.py(2 hunks)backend/tests/apps/api/rest/v0/repository_test.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/tests/apps/api/rest/v0/chapter_test.py
🧰 Additional context used
🧬 Code graph analysis (8)
backend/tests/apps/api/rest/v0/project_test.py (1)
backend/apps/api/rest/v0/project.py (3)
ProjectDetail(39-42)get_project(95-109)list_projects(68-81)
backend/tests/apps/api/rest/v0/issue_test.py (1)
backend/apps/api/rest/v0/issue.py (4)
IssueDetail(34-37)get_issue(108-122)list_issues(73-94)Issue(30-31)
backend/tests/apps/api/rest/v0/event_test.py (1)
backend/apps/api/rest/v0/event.py (3)
EventDetail(33-36)get_event(78-86)list_events(53-64)
backend/tests/apps/api/rest/v0/committee_test.py (1)
backend/apps/api/rest/v0/committee.py (3)
CommitteeDetail(37-40)get_committee(79-94)list_committees(57-65)
backend/apps/api/rest/v0/committee.py (1)
backend/apps/owasp/api/internal/queries/committee.py (1)
committee(14-28)
backend/tests/apps/api/rest/v0/release_test.py (2)
backend/apps/api/rest/v0/release.py (4)
ReleaseDetail(32-35)get_release(109-123)list_release(68-95)Release(28-29)backend/apps/github/models/release.py (1)
Release(10-135)
backend/tests/apps/api/rest/v0/repository_test.py (3)
backend/apps/github/api/internal/queries/repository.py (1)
repository(14-35)backend/apps/api/rest/v0/repository.py (4)
RepositoryDetail(31-34)get_repository(91-103)list_repository(61-77)Repository(27-28)backend/apps/github/models/repository.py (1)
Repository(25-352)
backend/tests/apps/api/rest/v0/milestone_test.py (1)
backend/apps/api/rest/v0/milestone.py (4)
MilestoneDetail(35-41)get_milestone(115-129)list_milestones(77-99)Milestone(31-32)
🔇 Additional comments (22)
backend/apps/api/rest/v0/release.py (2)
93-95: Excellent addition of deterministic tie-breaker.Adding
idas a final sort key ensures consistent, deterministic results across paginated queries. This is a best practice that prevents non-deterministic ordering when multiple records share the same sort field values.
90-91: Note:tag_nameuses exact match vs. case-insensitive elsewhere.The
tag_namefilter uses exact match (tag_name=filters.tag_name) while other filters likeorganizationandrepositoryuse case-insensitive matching (__iexact). This is likely intentional since Git tags are case-sensitive, but worth noting for consistency awareness.backend/apps/api/rest/v0/repository.py (1)
75-77: LGTM - consistent deterministic ordering.The addition of
idas a tie-breaker follows the same pattern as other endpoints in this PR, ensuring stable pagination results.backend/tests/apps/api/rest/v0/release_test.py (2)
41-181: Excellent comprehensive test coverage for list_release.The test suite thoroughly covers:
- Custom and default ordering with proper tie-breakers
- All filter combinations (organization, repository, tag_name)
- Correct ORM chain mocking and assertions
This significantly improves test coverage as intended by the PR objectives.
184-229: Well-structured tests for get_release endpoint.Both success and 404 error paths are tested, including proper verification of HTTP status codes and error message content. The use of
HTTPStatus.NOT_FOUNDand content assertions ensures correct error handling.backend/tests/apps/api/rest/v0/repository_test.py (2)
38-140: Thorough test coverage for list_repository with multiple scenarios.The tests cover:
- Multiple ordering scenarios (custom, by updated_at, default)
- Organization filter applied and not applied
- Proper verification that filter is NOT called when organization_id is None (lines 136-137)
This comprehensive coverage aligns well with the PR's goal to reach 90%+ coverage.
142-181: Good coverage of get_repository success and error paths.The tests properly verify both the found case with correct select_related usage and the not-found case with appropriate 404 status and error message.
backend/apps/api/rest/v0/milestone.py (2)
80-83: Good fix for parameter documentation consistency.As noted in the PR description, changing from a plain default
NonetoQuery(None, description="Ordering field")makes this endpoint consistent with others that useQuery()for API documentation. This improves the auto-generated API docs.
97-99: Deterministic ordering applied consistently.The addition of
idas a tie-breaker maintains consistency with other list endpoints updated in this PR (release, repository, issue).backend/tests/apps/api/rest/v0/milestone_test.py (2)
95-213: Comprehensive coverage of list_milestones filtering and ordering.The test suite covers:
- Custom ordering scenarios (created_at, -updated_at)
- All three filter types independently (organization, repository, state)
- Proper ORM chain verification including select_related with nested relationships
This significantly contributes to the PR's 90%+ coverage goal.
216-261: Well-tested get_milestone endpoint with proper error handling.Both success and 404 paths are tested with appropriate assertions on status code and error message content.
backend/apps/api/rest/v0/issue.py (1)
92-94: Consistent deterministic ordering pattern.The addition of
idas a tie-breaker aligns with the broader changes across all list endpoints in this PR, ensuring stable pagination results.backend/tests/apps/api/rest/v0/issue_test.py (1)
1-40: LGTM! Well-structured schema validation tests.The parametrized tests provide good coverage for the IssueDetail schema, validating all fields correctly.
backend/tests/apps/api/rest/v0/event_test.py (3)
1-39: LGTM! Well-structured event schema validation.The parametrized test effectively validates the EventDetail schema with comprehensive field checks across multiple test cases.
42-95: LGTM! Excellent coverage of the complex ordering logic.The tests thoroughly validate all ordering scenarios, including the special-case logic for
end_dateordering with appropriate secondary sort fields. The test cases correctly verify that:
- Default ordering uses
-start_dateas primaryend_dateordering usesstart_dateas secondary (and vice versa with proper sign handling)- All ordering includes
idas a final tie-breaker
98-131: LGTM! Complete coverage of the get_event endpoint.The tests properly validate both success and error paths, correctly mocking the
filter().first()pattern and verifying the 404 response with appropriate error message.backend/apps/api/rest/v0/chapter.py (1)
94-96: LGTM! Case-insensitive prefix detection improves robustness.The change to use
.lower().startswith("www-chapter-")makes the prefix detection case-insensitive while preserving the original casing when querying. This handles variants like "WWW-CHAPTER-" or "Www-Chapter-" correctly. Sincekey__iexactperforms case-insensitive matching, this approach is sound.backend/apps/api/rest/v0/committee.py (1)
79-79: Excellent fix! Function rename resolves the documented inconsistency.The rename from
get_chaptertoget_committeeaddresses the naming inconsistency you documented in the PR description. The updated docstring and case-insensitive prefix handling complete the fix. This makes the endpoint naming consistent with its purpose and aligns with the test expectations.Also applies to: 83-83, 88-88
backend/tests/apps/api/rest/v0/committee_test.py (2)
45-72: LGTM! Comprehensive coverage of list_committees ordering behavior.The tests effectively validate both custom and default ordering scenarios. The mock assertions correctly verify that
order_byis called with the expected parameters and that the result passes through correctly.
75-148: Excellent test coverage for get_committee endpoint!The four test methods comprehensively cover:
- ✓ Prefixed IDs (preserves as-is)
- ✓ Unprefixed IDs (adds prefix)
- ✓ Case-insensitive prefix detection (uppercase variant)
- ✓ Not found scenario (validates both status code and error message)
The tests validate the case-insensitive prefix handling improvement and properly verify the 404 response structure. This thorough coverage aligns well with the PR's objective to improve test coverage.
backend/tests/apps/api/rest/v0/project_test.py (2)
48-87: LGTM! Tests correctly validate multi-key ordering.The tests properly account for the project endpoint's multi-key ordering strategy (primary ordering + secondary sort by stars and forks). The assertions at lines 64-66 and 83-85 correctly verify that
order_byreceives all three parameters, matching the implementation in project.py.
90-155: Excellent test coverage with consistent structure!The test methods provide comprehensive coverage of the get_project endpoint:
- ✓ Prefix handling (with and without prefix)
- ✓ Case-insensitive prefix detection
- ✓ 404 response validation (status and message)
The consistent test structure across committee_test.py and project_test.py makes the test suite maintainable and easy to understand. This contributes well to the PR's goal of achieving 90%+ test coverage.
| ) -> list[Event]: | ||
| """Get all events.""" | ||
| return EventModel.objects.order_by(ordering or "-start_date", "-end_date") | ||
| if ordering and ordering.lstrip("-") == "end_date": |
There was a problem hiding this comment.
Why don't we want to rely on user provided ordering here?
There was a problem hiding this comment.
I added the special logic for end_date because I thought when two events end on the same date, sorting by start_date seemed a bit more meaningful (longer events come first).
e.g. if Event A runs June 1-15 and Event B runs June 10-15, ordering by end_date, start_date would show A first.
But I am a bit in the middle whether we should add semantic logic or keeping it simple? would like a suggestion here.
| if ordering and ordering.lstrip("-") == "end_date": | ||
| secondary = "-start_date" if ordering.startswith("-") else "start_date" | ||
| return EventModel.objects.order_by(ordering, secondary, "id") | ||
| return EventModel.objects.order_by(ordering or "-start_date", "-end_date", "id") |
There was a problem hiding this comment.
Let's use event name for tie break.
| issues = issues.filter(state=filters.state) | ||
|
|
||
| return issues.order_by(ordering or "-created_at", "-updated_at") | ||
| if ordering: |
There was a problem hiding this comment.
What's the reasoning behind this change?
There was a problem hiding this comment.
I used id so that pagination becomes deterministic. Also, the earlier version had a double ordering bug, if a user provides ordering=updated_at, this would result in order_by("updated_at", "-updated_at"), which is contradictory. same with other files too.
|
|
||
| return milestones.order_by(ordering or "-created_at", "-updated_at") | ||
| if ordering: | ||
| return milestones.order_by(ordering, "id") |
There was a problem hiding this comment.
Any reason why you change the user provided ordering params?
| assert result["total_count"] == 5 | ||
| assert result["total_pages"] == 1 | ||
| assert result["has_next"] is False | ||
| assert result["has_previous"] is False |
There was a problem hiding this comment.
I'm not sure if there is a way to enforce it -- we don't use is for boolean assertions.
| assert result["has_previous"] is False | |
| assert not result["has_previous"] |
| assert result["total_count"] == 23 | ||
| assert result["total_pages"] == 3 | ||
| assert result["has_next"] is False | ||
| assert result["has_previous"] is True |
There was a problem hiding this comment.
| assert result["has_previous"] is True | |
| assert result["has_previous"] |
2 participants
Resolves #2521
Added comprehensive unit tests across 15 files.
Test Coverage Report: [Attach screenshot]
While writing tests, I discovered some inconsistencies in the source code:
Function naming bug in
committee.pybackend/apps/api/rest/v0/committee.pyLine 82get_chapterbut should beget_committeeMilestone ordering parameter inconsistency
backend/apps/api/rest/v0/milestone.pyLine 80ordering: ... | None = Noneinstead ofordering: ... | None = Query(None, description="Ordering field")Query()wrapper for consistent API documentationShould I create separate issues for these inconsistencies?
Checklist
make check-testlocally; all checks and tests passed.