First pass at increasing test coverage #223
Conversation
There was a problem hiding this comment.
Pull request overview
This PR significantly increases test coverage from the previous baseline to 83% by adding comprehensive tests across multiple modules. The PR also includes important bug fixes and code quality improvements in production code, including fixing Http404 exception handling, adding proper HTTP status codes to JSON responses, and renaming functions to follow Python naming conventions.
Key Changes:
- Added extensive test coverage for views, models, admin, and context processors
- Fixed bug where
Http404was returned instead of raised indownload_media_file - Added missing
statusparameter toJsonResponsecalls for proper HTTP status code reporting - Refactored
getPlacesGeoJSONtoget_places_geojsonto follow snake_case convention
Reviewed changes
Copilot reviewed 12 out of 13 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| TEKDB/.coveragerc | Excludes /tests/ directory from coverage metrics to prevent false inflation |
| TEKDB/explore/views.py | Fixed Http404 bug (changed from return to raise) and refactored media file download logic |
| TEKDB/explore/tests/test_views.py | Added comprehensive tests for home, about, help, explore views, search functionality, media downloads, and export features |
| TEKDB/explore/tests/test_context_processors.py | Added tests for explore context processor with various configuration scenarios |
| TEKDB/TEKDB/views.py | Added status codes to JsonResponse, renamed getPlacesGeoJSON to get_places_geojson, fixed whitespace issues |
| TEKDB/TEKDB/tests/test_views.py | Refactored ImportTest to ImportDatabaseTest, added tests for failed imports and GeoJSON endpoint |
| TEKDB/TEKDB/tests/test_models.py | Added extensive tests for model methods including relationships, data, links, and keyword search functionality |
| TEKDB/TEKDB/tests/test_context_processors.py | Added tests for search settings and map default context processors |
| TEKDB/TEKDB/tests/test_admin.py | Added comprehensive tests for admin forms, permissions, and bulk upload functionality |
| TEKDB/TEKDB/models.py | Removed commented-out code and cleaned up whitespace |
| TEKDB/TEKDB/context_processors.py | Simplified error handling by removing unnecessary try-except wrapper |
| TEKDB/TEKDB/admin.py | Added Meta classes to CitationsForm and ResourcesForm, updated function call to use snake_case naming |
| TEKDB/TEKDB/tests/test_files/test_wrong_import.txt | Added test fixture file for testing incorrect file type imports |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| else: | ||
| obj = None | ||
|
|
||
| media = obj.media() |
There was a problem hiding this comment.
if obj is None obj.media() throws an error
| ) | ||
| return response | ||
| else: | ||
| return Http404 |
There was a problem hiding this comment.
I believe we either want to raise Http404 or return HttpResponseNotFound
|
|
||
| places_geojson = get_places_geojson(self.import_request) | ||
|
|
||
| ### 2025-05-09: No one has any idea what a 'placeMap' is. |
There was a problem hiding this comment.
I didn't find having these left in super helpful
|
|
||
| def changelist_view(self, request, extra_context=None): | ||
| from .views import getPlacesGeoJSON | ||
| from .views import get_places_geojson |
There was a problem hiding this comment.
I renamed the function to snake case instead of camel case to follow convention.
|
|
||
|
|
||
| class CitationsForm(forms.ModelForm): | ||
| class Meta: |
There was a problem hiding this comment.
I was getting an error with Meta missing
| if not request.method == "POST": | ||
| status_code = 405 | ||
| status_message = "Request method not allowed. Must be a post." | ||
| return JsonResponse( |
There was a problem hiding this comment.
first arg is data so passing status arg to get response.status in tests
| omit = | ||
| */migrations/* | ||
| */migrations/* | ||
| */tests/* |
There was a problem hiding this comment.
removing tests from coverage stats
asana task
First pass at upping the test coverage. Definitely still more room for improvement, but my brain is a bit tapped out on writing tests.
I removed
/tests/from coverage metrics, which were falsely inflating our coverage number. After these tests coverage is now at 83%.