Add REST API tests to check access rights when changing source/target storage #8479
Conversation
|
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 changes encompass various modifications across multiple test files in the REST API suite, enhancing test logic, adding new test cases, and updating data structures in JSON files. Key updates include improved date comparison logic, refined task selection criteria, and the introduction of new entries in annotation, job, label, project, and quality settings JSON files. Additionally, type annotations were added to several test methods for better clarity, and filtering logic was optimized in the 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: 7
🧹 Outside diff range and nitpick comments (7)
tests/python/rest_api/test_organizations.py (2)
42-64: LGTM: Improved type annotations and parameter handling.The changes enhance type safety and code organization. The introduction of type annotations and the
filtersdictionary improves code clarity and maintainability.A minor suggestion for further improvement:
Consider using a dictionary comprehension for
filtersto make it even more concise:filters = {key: value for key, value in { "privilege": privilege, "role": role, "org": org, "exclude_org": exclude_org, }.items() if value is not None}This would reduce the number of lines while maintaining readability.
246-268: LGTM: Consistent improvements across all methods.The changes in this method maintain the pattern of improvements seen throughout the file. The addition of type annotations and the use of the
filtersdictionary continue to enhance code clarity and maintainability.As mentioned in the previous comment, the repeated
filtersdictionary creation logic across multiple methods suggests an opportunity for refactoring. Implementing a helper method for creating the filters would significantly reduce code duplication and improve overall code maintainability.tests/python/shared/fixtures/data.py (1)
Line range hint
370-384: Reconsider changes in thefind_usersfunctionThe modifications to this function raise some concerns:
Removal of the assertion:
- This allowed the function to validate that at least one filter criterion was provided.
- Without it, calling the function with empty
kwargsmight lead to unexpected behavior, potentially returning all users instead of a filtered subset.Removal of
Nonevalue filtering:
- This change might alter how the function processes its input, potentially including
Nonevalues in the filtering process.- This could lead to unintended filtering results if
Nonevalues are present inkwargs.Consider the following improvements:
- Reinstate the assertion or add a check to handle empty
kwargs:def find(**kwargs): if not kwargs: raise ValueError("At least one filter criterion must be provided")
- Clarify the intended behavior for
Nonevalues inkwargs. If they should be ignored, reinstate the filtering:kwargs = {k: v for k, v in kwargs.items() if v is not None}These changes will help maintain the function's robustness and clarify its behavior.
tests/python/rest_api/test_jobs.py (1)
13-13: Ensure consistent import ordering.The import of
datetimehas been moved. While this change is minor, it's good to maintain consistent import ordering throughout the codebase.Consider grouping imports according to PEP 8 guidelines: standard library imports, third-party imports, and local imports, each group separated by a blank line.
tests/python/rest_api/test_tasks.py (3)
3346-3355: Add assertion messages for better test diagnosticsIncluding messages in assert statements enhances clarity and aids in debugging when tests fail.
Apply this diff to add assertion messages:
def test_can_update_assignee_updated_date_on_assignee_updates(...): ... if isinstance(updated_task.assignee_updated_date, datetime): assert op( str(updated_task.assignee_updated_date.isoformat()).replace("+00:00", "Z"), task["assignee_updated_date"], + f"Assignee updated date should {'match' if op == operator.eq else 'not match'} when assignee IDs are {'the same' if new_assignee_id == old_assignee_id else 'different'}." ) else: assert op( updated_task.assignee_updated_date, task["assignee_updated_date"], + f"Assignee updated date should {'match' if op == operator.eq else 'not match'} when assignee IDs are {'the same' if new_assignee_id == old_assignee_id else 'different'}." ) ...
3356-3374: Add type annotations and docstring to_test_patch_linked_storagemethodIncluding type annotations and a docstring improves code readability and maintainability.
Apply this diff to enhance the method:
@staticmethod -def _test_patch_linked_storage( - user: str, task_id: int, *, expected_status: HTTPStatus = HTTPStatus.OK -) -> None: +def _test_patch_linked_storage( + user: str, + task_id: int, + *, + expected_status: HTTPStatus = HTTPStatus.OK +) -> None: + """ + Helper method to test patching linked storage for a given task and user. + """ with make_api_client(user) as api_client: for associated_storage in ("source_storage", "target_storage"): patch_data = { associated_storage: { "location": "local", } } (_, response) = api_client.tasks_api.partial_update( task_id, patched_task_write_request=patch_data, _check_status=False, _parse_response=False, ) assert response.status == expected_status, response.status
3384-3390: Add type annotations to function parametersAdding type annotations for
tasksandfind_usersenhances code clarity and facilitates static type checking.Apply this diff to include type annotations:
def test_update_task_linked_storage_by_org_roles( self, role: str, is_allow: bool, - tasks, - find_users, + tasks: List[Dict[str, Any]], + find_users: Callable[..., List[Dict[str, Any]]], ): ...
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
tests/python/shared/assets/cvat_db/cvat_data.tar.bz2is excluded by!**/*.bz2
📒 Files selected for processing (13)
- tests/python/rest_api/test_jobs.py (2 hunks)
- tests/python/rest_api/test_labels.py (1 hunks)
- tests/python/rest_api/test_organizations.py (5 hunks)
- tests/python/rest_api/test_projects.py (6 hunks)
- tests/python/rest_api/test_tasks.py (3 hunks)
- tests/python/shared/assets/annotations.json (2 hunks)
- tests/python/shared/assets/cvat_db/data.json (11 hunks)
- tests/python/shared/assets/jobs.json (2 hunks)
- tests/python/shared/assets/labels.json (2 hunks)
- tests/python/shared/assets/projects.json (3 hunks)
- tests/python/shared/assets/quality_settings.json (2 hunks)
- tests/python/shared/assets/tasks.json (1 hunks)
- tests/python/shared/fixtures/data.py (2 hunks)
🔇 Additional comments (42)
tests/python/rest_api/test_organizations.py (2)
9-9: LGTM: Import statement for type hinting added.The addition of
Optionalfrom thetypingmodule is appropriate for the type annotations introduced in the file. This change enhances type safety and code readability.
91-113: LGTM: Consistent improvements in type annotations and parameter handling.The changes in this method mirror those made in the
test_can_send_options_requestmethod, maintaining consistency throughout the file. The type annotations andfiltersdictionary approach continue to enhance code clarity and maintainability.tests/python/shared/assets/quality_settings.json (5)
2-2: Verify consistency of "count" fieldThe "count" field has been updated from 20 to 23, which correctly reflects the addition of three new entries in the "results" array.
386-404: New entry for task_id 26 looks goodThe new entry with id: 21 and task_id: 26 follows the existing structure and maintains consistency with other entries in the array. All fields are present with appropriate data types and values.
405-423: New entry for task_id 27 is consistentThe new entry with id: 22 and task_id: 27 maintains the same structure and consistency as other entries in the array. All fields are present with appropriate data types and values.
424-441: New entry for task_id 28 adheres to the established patternThe new entry with id: 23 and task_id: 28 follows the existing structure and maintains consistency with other entries in the array. All fields are present with appropriate data types and values.
Line range hint
2-441: Changes align with PR objectives and maintain consistencyThe updates to the
quality_settings.jsonfile are consistent with the PR objectives of enhancing REST API tests. The changes include:
- Updating the "count" field from 20 to 23.
- Adding three new entries (ids: 21, 22, 23) for tasks 26, 27, and 28.
These new entries maintain the same structure and data types as existing entries, ensuring consistency throughout the file. The additions likely correspond to new test cases for verifying access rights when changing source/target storage, as mentioned in the PR objectives.
tests/python/shared/fixtures/data.py (1)
Line range hint
70-96: Excellent optimization in thefilter_assetsfunction!The changes in this function improve both readability and performance:
- The introduction of the
is_matchedvariable makes the logic clearer by separating the matching process from the appending action.- The early break in the loop when a mismatch is found improves efficiency, especially for resources that don't match the criteria.
- The final check of
is_matchedbefore appending ensures that only resources meeting all criteria are included.These changes maintain the original functionality while making the code more maintainable and potentially faster for large datasets.
tests/python/shared/assets/projects.json (4)
Line range hint
1-624: Summary of changes and approvalThe
projects.jsonfile has been successfully updated with the following key changes:
- Increased project count from 13 to 15.
- Added two new projects (ID 16 and 15) with comprehensive details.
- Updated Project ID 10 with dimension, tasks count, and updated date changes.
These modifications enhance the test data set and appear to be consistent with the existing structure. Verification steps have been provided for critical changes to ensure data integrity.
The file changes are approved, pending successful execution of the provided verification scripts.
6-95: New projects added successfullyThe two new projects (ID 16 and 15) have been added with comprehensive details, maintaining consistency with the existing project structure. This addition enhances the test data set.
To ensure the integrity of the project data, please run the following script to verify the uniqueness of project IDs:
#!/bin/bash # Description: Verify the uniqueness of project IDs # Extract all project IDs project_ids=$(jq '.results[].id' tests/python/shared/assets/projects.json | sort -n) # Check for duplicates duplicates=$(echo "$project_ids" | uniq -d) if [ -z "$duplicates" ]; then echo "All project IDs are unique." else echo "Duplicate project IDs found:" echo "$duplicates" fi # Display all project IDs for reference echo "All project IDs:" echo "$project_ids"
Line range hint
266-297: Verify updates to Project ID 10Project ID 10 has been updated with the following changes:
- The "dimension" field is now set to "2d" (previously null).
- The "tasks" count has increased from 0 to 1.
- The "updated_date" has been set to a more recent date.
These changes appear to reflect updates in the project's status and content. Please confirm that these modifications are intentional and accurately represent the project's current state.
To ensure the consistency of these changes, run the following script:
#!/bin/bash # Description: Verify the updates to Project ID 10 # Extract Project ID 10 data project_10=$(jq '.results[] | select(.id == 10)' tests/python/shared/assets/projects.json) # Check specific fields dimension=$(echo "$project_10" | jq -r '.dimension') tasks_count=$(echo "$project_10" | jq -r '.tasks.count') updated_date=$(echo "$project_10" | jq -r '.updated_date') echo "Project ID 10 updates:" echo "Dimension: $dimension" echo "Tasks count: $tasks_count" echo "Updated date: $updated_date" # Verify if the updated_date is more recent than the created_date created_date=$(echo "$project_10" | jq -r '.created_date') if [[ "$updated_date" > "$created_date" ]]; then echo "Updated date is more recent than created date." else echo "Warning: Updated date is not more recent than created date." fi
2-2: Verify project count consistencyThe project count has been updated from 13 to 15, which is consistent with the addition of two new projects. However, it's crucial to ensure that this count matches the actual number of projects in the "results" array.
To confirm the accuracy of the project count, run the following script:
✅ Verification successful
Project count is consistent
The
countfield is correctly set to 15, which matches the number of projects in theresultsarray.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the project count matches the number of projects in the results array # Get the count value count=$(jq '.count' tests/python/shared/assets/projects.json) # Get the number of projects in the results array actual_count=$(jq '.results | length' tests/python/shared/assets/projects.json) echo "Declared count: $count" echo "Actual count: $actual_count" if [ "$count" -eq "$actual_count" ]; then echo "Project count is consistent." else echo "Project count mismatch: declared $count, actual $actual_count" fiLength of output: 5877
Script:
#!/bin/bash # Description: Verify the project count matches the number of projects in the results array # Get the count value count=$(jq '.count' tests/python/shared/assets/projects.json) # Get the number of projects in the results array actual_count=$(jq '.results | length' tests/python/shared/assets/projects.json) echo "Declared count: $count" echo "Actual count: $actual_count" if [ "$count" -eq "$actual_count" ]; then echo "Project count is consistent." else echo "Project count mismatch: declared $count, actual $actual_count" fiLength of output: 326
tests/python/shared/assets/labels.json (3)
2-2: LGTM: Count field correctly updatedThe "count" field has been incremented from 41 to 43, which accurately reflects the addition of two new label objects to the "results" array.
831-841: LGTM: New label object correctly structuredThe new label object for "cat" (id: 75, task_id: 26) is well-formed and consistent with existing label definitions. It maintains the same structure, color, and type as other "cat" labels in the file.
843-852: LGTM: New label objects correctly addedThe second new label object for "cat" (id: 76, task_id: 27) is also well-formed and consistent with existing label definitions. Both new label objects maintain the same structure, color, and type as other "cat" labels in the file.
These additions align with the PR objectives of enhancing the test suite, as they provide additional label definitions that can be used in REST API tests for access rights when changing source/target storage.
tests/python/shared/assets/tasks.json (5)
2-2: LGTM: Task count updated correctly.The "count" field has been accurately updated to 23, reflecting the addition of three new task entries.
6-56: LGTM: New task entry (ID: 28) is well-formed.The new task entry for "task 1" (ID: 28) is correctly structured and contains all necessary fields. It's properly associated with a project (project_id: 10) and has no assignee, which is valid.
57-113: LGTM: New task entry (ID: 27) is correctly structured.The new task entry "task assigned to maintainer" (ID: 27) is properly formatted and includes all necessary fields. It's correctly assigned to user4 (User Fourth) and is not associated with any project (project_id: null), which is a valid configuration.
114-170: LGTM: New task entry (ID: 26) is properly structured.The new task entry "task assigned to owner" (ID: 26) is correctly formatted and contains all required fields. It's appropriately assigned to business1 (Business First), owned by user3 (User Third), and is not associated with any project (project_id: null). This configuration is valid and consistent with the existing structure.
1-170: Summary: All changes to tasks.json are correct and consistent.The updates to
tests/python/shared/assets/tasks.jsonhave been thoroughly reviewed. The file now includes three new task entries (IDs: 28, 27, and 26) with varying configurations, and the total task count has been correctly updated to 23. All new entries are well-structured and consistent with the existing data format. These changes appear to support the PR objective of introducing tests for the REST API to verify access rights when changing source/target storage.tests/python/shared/assets/jobs.json (4)
2-2: LGTM: Job count updated correctly.The "count" field has been accurately updated to 30, reflecting the addition of three new job entries.
6-46: New job entry (ID 37) looks good, but verify the date.The structure and content of the new job entry are consistent with existing entries and contain all required fields. However, please note that the
created_dateandupdated_dateare set to a future date (September 23, 2024). Ensure this is intentional for testing purposes.
47-128: New job entries (IDs 36 and 35) are consistent and well-structured.The new job entries with IDs 36 and 35 are consistent in structure and content with both the first new entry (ID 37) and existing entries. All required fields are present, and the data appears valid. The use of future dates (September 23, 2024) is consistent across all new entries, which is likely intentional for testing purposes.
Line range hint
1-1158: Overall changes are consistent and suitable for testing purposes.The updates to the
jobs.jsonfile are well-structured and consistent:
- The job count has been correctly updated to 30.
- Three new job entries (IDs 37, 36, and 35) have been added with all required fields.
- The new entries use future dates (September 23, 2024) consistently, which appears to be intentional for testing scenarios.
- Existing entries remain unchanged.
These changes seem appropriate for testing purposes, particularly for scenarios involving future-dated jobs. Ensure that your test cases are designed to handle these future dates appropriately.
tests/python/rest_api/test_labels.py (1)
931-935: Improved task selection criteria for the test case.The changes enhance the specificity of the task selection for the
test_task_label_update_triggers_nested_task_and_job_updatefunction. By selecting a task that has jobs, labels, and is not associated with any project, the test case becomes more focused and potentially more reliable. This modification addresses a specific scenario that may not have been covered in the original implementation.tests/python/rest_api/test_jobs.py (4)
8-8: Good addition of theoperatormodule.The import of the
operatormodule is a good choice for implementing dynamic comparison operations. This addition enhances the flexibility of the test logic.
1275-1281: Improved assignee update date comparison logic.The updated code introduces a more flexible and robust way of comparing the
assignee_updated_date:
- It uses the
operatormodule to dynamically select the comparison operation based on whether the assignee has changed.- It checks if
updated_job.assignee_updated_dateis an instance ofdatetimebefore performing the assertion.- It converts the date to ISO format and replaces the timezone offset with "Z" for consistency in comparison.
These changes improve the test's reliability and its ability to handle different date formats.
1283-1283: Fallback comparison for non-datetime objects.The else clause provides a fallback comparison for cases where
assignee_updated_dateis not a datetime object. This ensures that the test can handle different data types for the date field.
1275-1283: Overall improvement in test logic and robustness.The changes to the
test_can_update_assignee_updated_date_on_assignee_updatesfunction significantly improve its flexibility and reliability. By introducing dynamic operator selection and more robust date comparison logic, the test can now handle various scenarios more effectively. These modifications enhance the overall quality of the test suite.tests/python/shared/assets/cvat_db/data.json (6)
1552-1608: LGTM: Newengine.dataentries added correctly.The new entries for the
engine.datamodel (pk: 25, 26, 27) are correctly formatted and consistent. The repetition of identical entries is likely intentional for testing purposes.
3147-3179: LGTM: Newengine.imageentries added correctly.The new entries for the
engine.imagemodel (pk: 491, 492, 493) are correctly formatted and consistent with the newengine.dataentries. Eachengine.imageentry corresponds to one of the newengine.dataentries, maintaining data integrity.
3439-3480: LGTM: Newengine.projectentries provide diverse test cases.The new entries for the
engine.projectmodel (pk: 15, 16) are correctly formatted and provide diverse test cases. They have different names, owners, and assignees, which is excellent for testing various scenarios. Each project also has its own source and target storage, allowing for comprehensive testing of storage-related functionality.
4017-4098: LGTM: Newengine.taskentries provide comprehensive test cases.The new entries for the
engine.taskmodel (pk: 26, 27, 28) are correctly formatted and provide diverse test cases. Each task has different properties (name, owner, assignee, etc.), which is excellent for testing various scenarios. Notably, task 28 is associated with an existing project (pk: 10), allowing for testing of project-task relationships.
5003-5026: LGTM: New entries for related models maintain data integrity.The new entries for
engine.clientfile(pk: 461, 462, 463),engine.segment(pk: 35, 36, 37), andengine.job(pk: 35, 36, 37) are correctly formatted and maintain consistency with the new task entries. This addition ensures that the test data covers the entire task creation process, providing a comprehensive dataset for testing.Also applies to: 5333-5365, 5781-5825
Line range hint
1552-17155: Overall, the changes enhance the test dataset significantly.The additions to
tests/python/shared/assets/cvat_db/data.jsongreatly expand the test dataset for the CVAT system. New entries have been added for various models including projects, tasks, images, labels, and quality control settings. These additions will allow for more comprehensive testing of the system, covering a wide range of scenarios and relationships between different entities.While the changes are generally well-structured and consistent, consider the suggestions made earlier to further improve the diversity of test cases, particularly for storage locations and quality control settings. This could lead to even more robust testing capabilities.
tests/python/rest_api/test_projects.py (6)
374-376: Addingfilter_projectsparameter enhances test flexibilityThe addition of the
filter_projectsparameter to thetest_can_get_backup_project_when_all_tasks_have_no_datamethod allows for more precise project selection based on specific criteria. This improves the test's robustness and maintainability.
381-386: Correct inclusion oforg_idin task creationIncluding the
org_idparameter in thepost_methodcall ensures that the created task is correctly associated with the appropriate organization. This is essential for tests that involve organization-specific behaviors.
389-394: Consistent organization association in task creationAgain, the
org_idparameter is correctly included when creating the second empty task. This maintains consistency and ensures that both tasks are tied to the correct organization within the test.
907-913: Addition oforganizationfield in task creationIncluding the
"organization": project["organization"]field in the task creation payload ensures that the task is properly linked to the project’s organization. This is important for tests that validate behavior in organizational contexts.
934-947: Creating multiple tasks associated with the correct organizationBy adding the
org_idparameter when creating empty tasks, the test accurately simulates scenarios where projects have tasks without data within an organization. This enhances the test coverage for export functionality when dealing with such projects.
1440-1458: Addition of_test_patch_linked_storagemethod improves code reuseThe new static method
_test_patch_linked_storageprovides a reusable way to test updating associated storage fields (source_storageandtarget_storage) of a project. This reduces code duplication and enhances test maintainability.tests/python/rest_api/test_tasks.py (1)
558-561: LGTMThe function
test_member_update_task_annotationhas been updated appropriately to utilizetasks_with_shapes.
Marishka17
force-pushed
the
mk/change_linked_storage_rest_api_tests
branch
from
ba667cd
to
6fa56b0
Compare
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #8479 +/- ##
===========================================
- Coverage 74.34% 74.34% -0.01%
===========================================
Files 394 394
Lines 42216 42212 -4
Branches 3896 3896
===========================================
- Hits 31387 31383 -4
Misses 10829 10829
|
Motivation and context
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
New Features
Bug Fixes
Documentation
Chores