fixing broken unit tests in cvat/app/events #8867
Conversation
archibald1418
requested review from
zhiltsov-max and
Marishka17
as code owners
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe pull request introduces two new time-related constants, Changes
Poem
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
cvat/apps/events/tests/test_events.py (1)
19-22: Clarify logic behind short vs. long gap thresholds.It's a good practice to add inline documentation or a comment explaining that
_SHORT_GAPis just a millisecond below the threshold, while_LONG_GAPmatches the threshold exactly. This helps future maintainers quickly grasp the test scenario's purpose.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cvat/apps/events/const.py(1 hunks)cvat/apps/events/handlers.py(1 hunks)cvat/apps/events/tests/test_events.py(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- cvat/apps/events/const.py
🔇 Additional comments (4)
cvat/apps/events/tests/test_events.py (3)
15-15: Use constants directly in tests to ensure consistent reference.
Importing TIME_THRESHOLD and WORKING_TIME_RESOLUTION from cvat.apps.events.const centralizes control over these time-related values. It helps keep the tests consistent with the production code. This is good practice and makes updating thresholds less error-prone.
37-37: Leverage integer arithmetic or explicit rounding for clarity.
When dividing by WORKING_TIME_RESOLUTION, ensure the resulting integer is correct for all potential durations. Consider an explicit rounding step to ensure the intended integer behavior is well-documented and unambiguous.
41-41: Rename usage references to match the updated function name _working_time_event.
Ensure that all references to the old _working_time method are updated throughout the test suite. Verify the rename does not break any existing method calls or test code.
cvat/apps/events/handlers.py (1)
34-34: Centralizing constants in const.py is a good improvement.
Importing TIME_THRESHOLD and WORKING_TIME_RESOLUTION from a dedicated constants file fosters better maintainability and consistency across modules. Good job!
archibald1418
changed the title
archibald1418
changed the title
archibald1418
changed the title
archibald1418
changed the title
archibald1418
changed the title
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #8867 +/- ##
===========================================
+ Coverage 73.89% 73.90% +0.01%
===========================================
Files 408 411 +3
Lines 44131 44174 +43
Branches 3986 3993 +7
===========================================
+ Hits 32611 32648 +37
- Misses 11520 11526 +6
|
Co-authored-by: Maxim Zhiltsov <[email protected]>
…vat into ov/events_unit_tests_issue8349
cvat/apps/events/const.py
Outdated
| MAX_EVENT_DURATION = datetime.timedelta(seconds=100) | ||
| WORKING_TIME_RESOLUTION = datetime.timedelta(milliseconds=1) | ||
| WORKING_TIME_SCOPE = 'send:working_time' | ||
| COLLAPSED_EVENT_SCOPES = frozenset(("change:frame",)) |
There was a problem hiding this comment.
Probably, it makes sense to rename either this constant or the compressed_* tests to use the same term. I'd vote for the constant renaming, because the word "collapse" primarily means something different.
|
Includes and closes #8339, also closes #8349
This adressess issue #8349. Necessary changes proposed in #8339 are blocked by runtime failures in cvat/apps/events unit-testing suite. The tests rely on functionality that was made obsolete after changes made in #7958.
__init__.pyfiles as proposed in Add missing __init__.py files #8339cvat/apps/events/handler.py::handle_client_events_pushavailable to both the handler and the unit test, renamed some of themMotivation and context
How has this been tested?
Unit tests on events were run both locally and inside CI
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
New Features
TIME_THRESHOLD(100 seconds) andWORKING_TIME_RESOLUTION(1 millisecond).Bug Fixes
Tests