Fix unstable honeypot reroll test #8639
Conversation
WalkthroughThe changes in this pull request involve two main files: Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test Suite
participant Task as Task Management System
Test->>Task: Attempt to change honeypot frames
alt Random outcome fails
Test->>Test: Retry up to _MAX_RANDOM_ATTEMPTS
Test->>Task: Retry change
end
Test->>Task: Confirm change success or fail with message
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
tests/python/rest_api/test_tasks.py (2)
4534-4535: Add docstring for the constant.The constant
_MAX_RANDOM_ATTEMPTSwould benefit from a docstring explaining its purpose and why the value 20 was chosen.- _MAX_RANDOM_ATTEMPTS = 20 # This test can have random outcomes, it's expected + # Maximum number of attempts to retry random honeypot frame selection before failing the test. + # The value is chosen to balance between test reliability and execution time. + _MAX_RANDOM_ATTEMPTS = 20
4557-4584: Consider extracting retry logic into a helper function.The retry logic for handling random honeypot frame selection could be extracted into a reusable helper function to improve code maintainability and reusability.
+ def _retry_random_frame_selection(self, api_client, job_id, params, old_validation_layout): + """ + Helper function to retry random honeypot frame selection. + Returns the new validation layout after successful selection or fails the test. + """ + attempt = 0 + while attempt < self._MAX_RANDOM_ATTEMPTS: + new_validation_layout = json.loads( + api_client.jobs_api.partial_update_validation_layout( + job_id, + patched_job_validation_layout_write_request=( + models.PatchedJobValidationLayoutWriteRequest(**params) + ), + )[1].data + ) + + if ( + params["frame_selection_method"] != "random_uniform" + or new_validation_layout["honeypot_real_frames"] + != old_validation_layout["honeypot_real_frames"] + ): + return new_validation_layout + + attempt += 1 + + pytest.fail( + f"Failed to get different honeypot frames after {attempt} attempts. " + "This may indicate an issue with the random selection logic." + ) - attempt = 0 - while attempt < _MAX_RANDOM_ATTEMPTS: - new_validation_layout = json.loads( - api_client.jobs_api.partial_update_validation_layout( - annotation_job["id"], - patched_job_validation_layout_write_request=( - models.PatchedJobValidationLayoutWriteRequest(**params) - ), - )[1].data - ) - - new_honeypot_real_frames = new_validation_layout["honeypot_real_frames"] - - if ( - frame_selection_method == "random_uniform" - and new_honeypot_real_frames - == old_validation_layout["honeypot_real_frames"] - ): - attempt += 1 - else: - break - - if attempt >= _MAX_RANDOM_ATTEMPTS and frame_selection_method == "random_uniform": - pytest.fail(f"too many attempts ({attempt}) with random honeypot updating") + new_validation_layout = self._retry_random_frame_selection( + api_client, annotation_job["id"], params, old_validation_layout + ) + new_honeypot_real_frames = new_validation_layout["honeypot_real_frames"]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
cvat/apps/engine/cache.py(1 hunks)tests/python/rest_api/test_tasks.py(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- cvat/apps/engine/cache.py
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #8639 +/- ##
===========================================
- Coverage 74.30% 74.30% -0.01%
===========================================
Files 401 401
Lines 43419 43421 +2
Branches 3951 3951
===========================================
+ Hits 32263 32264 +1
- Misses 11156 11157 +1
|
Motivation and context
The test can fail sometimes because of the random endpoint behavior (this is by design). Allow some flexibility in expected cases
How has this been tested?
Checklist
developbranch(cvat-canvas,
cvat-core,
cvat-data and
cvat-ui)
License
Feel free to contact the maintainers if that's a concern.
Summary by CodeRabbit
Bug Fixes
Tests