Modernize Test Suite and Improve Tests #72
Merged
Conversation
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
- Add new tests for spatial filters (near, distance_lt, bounds) in src/sa_api_v2/tests/test_spatial_views.py. - Fix GEOSException import and handling in to_geom utility to gracefully handle invalid geometry strings. - Improve error handling for bounding box filters in LocatedResourceMixin to return 400 Bad Request instead of 500 when provided with invalid inputs. - Explicitly set SRID 4326 for bounding box polygons to ensure consistency with database queries.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR modernizes the test infrastructure by migrating from Django's built-in test runner to pytest and increases test coverage for spatial filters and bulk data API endpoints. The PR also includes critical bug fixes for spatial filtering functionality.
Changes:
- Migrated test runner from Django's
manage.py testtopytestwith coverage reporting - Fixed spatial filter bugs causing 500 errors and empty query results
- Added comprehensive test coverage for spatial views and bulk data API
Reviewed changes
Copilot reviewed 10 out of 13 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| pytest.ini | Pytest configuration with Django settings and database reuse options |
| requirements.txt | Added pytest, pytest-django, pytest-cov, and pytest-xdist dependencies |
| Makefile | Added test-coverage and test-coverage-html targets for coverage reporting |
| compose.yml | Updated test service to use pytest and mount coverage output volumes |
| Containerfile | Copy pytest.ini to container for test execution |
| .gitignore | Added htmlcov/ directory to ignore coverage reports |
| src/sa_api_v2/views/base_views.py | Fixed bbox ValueError handling and added SRID 4326 assignment |
| src/sa_api_v2/utils.py | Added GEOSException handling for robust geometry parsing |
| src/sa_api_v2/tests/test_spatial_views.py | New test file with comprehensive spatial filter tests |
| src/sa_api_v2/views/bulk_data_views.py | Fixed deprecated request.DATA, added renderer_classes, but contains debug statement |
| src/sa_api_v2/renderers.py | Fixed GeoJSONRenderer format for proper content negotiation |
| src/sa_api_v2/tests/test_bulk_data_views.py | New test file with bulk data API coverage, but has incorrect assertion |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| renderer_classes = (JSONRenderer, renderers.JSONPRenderer, renderers.GeoJSONPRenderer, renderers.GeoJSONRenderer, CSVRenderer) | ||
|
|
||
| def get(self, request, owner_username, dataset_slug, submission_set_name, data_guid, format=None): | ||
| print(f"DEBUG: View called. GUID: {data_guid}, Format: {format}") |
There was a problem hiding this comment.
Debug print statement should be removed before merging to production. This will pollute logs and can expose sensitive information.
Suggested change
| print(f"DEBUG: View called. GUID: {data_guid}, Format: {format}") | |
| log.debug("View called. GUID: %s, Format: %s", data_guid, format) |
| request.user = self.owner | ||
| response = self.view(request, **request_kwargs) | ||
|
|
||
| self.assertEqual(response.status_code, 404) |
There was a problem hiding this comment.
Test expects status code 404 for invalid format, but the actual implementation returns 400 (line 197 in bulk_data_views.py). The test assertion should be updated to expect 400, which is the correct HTTP status code for invalid input parameters.
| def test_GET_with_existing_snapshots(self): | ||
| """GET should return list of existing snapshots.""" | ||
| # Create a snapshot request | ||
| snapshot = DataSnapshotRequest.objects.create( |
There was a problem hiding this comment.
Variable snapshot is not used.
Suggested change
| snapshot = DataSnapshotRequest.objects.create( | |
| DataSnapshotRequest.objects.create( |
| def test_POST_returns_existing_pending_request(self, mock_store): | ||
| """POST should return existing pending request instead of creating duplicate.""" | ||
| # Create an existing pending request | ||
| existing = DataSnapshotRequest.objects.create( |
There was a problem hiding this comment.
Variable existing is not used.
Suggested change
| existing = DataSnapshotRequest.objects.create( | |
| DataSnapshotRequest.objects.create( |
| def test_GET_pending_snapshot(self): | ||
| """GET for pending snapshot should return 503.""" | ||
| # Create a pending request without fulfillment | ||
| pending_request = DataSnapshotRequest.objects.create( |
There was a problem hiding this comment.
Variable pending_request is not used.
Suggested change
| pending_request = DataSnapshotRequest.objects.create( | |
| DataSnapshotRequest.objects.create( |
|
|
||
| def test_GET_failed_snapshot(self): | ||
| """GET for failed snapshot should return 404.""" | ||
| failed_request = DataSnapshotRequest.objects.create( |
There was a problem hiding this comment.
Variable failed_request is not used.
Suggested change
| failed_request = DataSnapshotRequest.objects.create( | |
| DataSnapshotRequest.objects.create( |
| @@ -0,0 +1,279 @@ | |||
| import json | |||
There was a problem hiding this comment.
Import of 'json' is not used.
Suggested change
| import json |
| from django.core.cache import cache as django_cache | ||
| from unittest import mock | ||
|
|
||
| from ..models import User, DataSet, Place, DataSnapshotRequest, DataSnapshot |
There was a problem hiding this comment.
Import of 'Place' is not used.
Suggested change
| from ..models import User, DataSet, Place, DataSnapshotRequest, DataSnapshot | |
| from ..models import User, DataSet, DataSnapshotRequest, DataSnapshot |
| from django.test import TestCase | ||
| from django.test.client import RequestFactory | ||
| from django.urls import reverse | ||
| from django.conf import settings |
There was a problem hiding this comment.
Import of 'settings' is not used.
Suggested change
| from django.conf import settings |
- Remove USE_L10N setting - Switch to Django's storages API - Use new GISModelAdmin class - Opt in to use new DjangoDivFormRenderer
mjumbewu
force-pushed
the
test-modernization
branch
from
7a30512 to
5987193
Compare
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.
Increases test coverage specifically focusing on Bulk Data Snapshots and Spatial Filters.
Key Changes
1. Test Infrastructure Modernization
pytest: Migrated the test runner from Django'smanage.py testtopytestandpytest-djangofor better performance and flexibility.test-coveragetarget to theMakefileand configuredpytest-covto generate HTML reports.2. Spatial Filter Enhancements
Polygon.from_bboxwould raise an unhandledValueErrorfor malformed inputs, causing 500 errors. It now correctly returns a 400 Bad Request.utils.to_geomto handleGEOSExceptionand provide robust fallback parsing forlat,lngcoordinate strings.near(Point) sorting.bounds(Bbox) filtering.distance_ltfiltering.3. Bulk Data API Improvements
src/sa_api_v2/tests/test_bulk_data_views.py, increasing code coverage for bulk snapshot views from ~29% to ~98%.DataSnapshotInstanceViewto explicitly include required renderers (GeoJSON, CSV, JSONP)..geojsonfiles by aligning the renderer format in `render