fix(upload): resolve login_required test TODOs & patch Exception TypeError#1044
Open
MrButtCode wants to merge 3 commits intoCCExtractor:masterfrom
Open
fix(upload): resolve login_required test TODOs & patch Exception TypeError#1044MrButtCode wants to merge 3 commits intoCCExtractor:masterfrom
MrButtCode wants to merge 3 commits intoCCExtractor:masterfrom
Conversation
…Exception - Defined missing mock_decorator helper used by login_required patch - Fixed import-time decorator issue by patching at source and reloading module - Rewrote test_link_id_confirm_valid to use HTTP test client with real session - Added default message param to QueuedSampleNotFoundException.__init__() to restore backwards compatibility with all four call sites
MrButtCode
requested review from
canihavesomecoffee and
thealphadollar
as code owners
|
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.
TL;DR
QueuedSampleNotFoundExceptionwas raising aTypeErrordue to a missing mandatorymessageargument across four call sites.test_controllers.pyby resolving import-time decorator mocking failures.1. Production Bug Fix (
exceptions.py)The Problem: The
QueuedSampleNotFoundExceptionclass was updated at some point to require a mandatorymessageargument in its__init__. However, four call sites inmod_upload/controllers.pywere never updated. If triggered in production, this would crash the application with aTypeErrorrather than raising the catchable exception.The Fix: Added a default value (
message="Queued sample not found") to the exception's constructor. This is backwards-compatible: existing call sites without arguments now work safely, while custom messages can still be passed.2. Test Restoration (
test_controllers.py)The Problem:
test_link_id_confirm_invalidandtest_link_id_confirm_validwere commented out. The original author attempted to patchlogin_requiredusingside_effect=mock_decorator(which wasn't defined) and fundamentally misunderstood that Python evaluates decorators at import time, making standard runtimemock.patchineffective.The Fix: I implemented two distinct testing strategies to handle the decorator:
test_link_id_confirm_invalid: Restored the test by defining a module-levelmock_decoratorpassthrough and using areload()strategy. By patching the decorator at its source inmod_auth.controllersand reloading mod_upload.controllers, the decorators are correctly re-applied under the patch.test_link_id_confirm_valid: Rewrote this as a true integration test using the Flask HTTP test client. To satisfy the controller's ownership verification without throwing a 404, I seeded the test database with the complete required relational chain (User->Upload->Sample->QueuedSample).