[GSoC2024] - add/rest-api-tests-for-attribute-rename #7754
Conversation
|
Hi @Marishka17 @nmanovic |
|
Hi @azhavoro, |
tests/python/rest_api/test_labels.py
Outdated
| label = next( | ||
| iter( | ||
| self._labels_by_source( | ||
| self.labels, source_key=self._get_source_info(source).label_source_key | ||
| ).values() | ||
| ) | ||
| )[0] | ||
|
|
||
| if label.get("attributes"): |
There was a problem hiding this comment.
Only a label with attributes should be selected here. Overwise the test will always pass if a label has no attributes and there will be no point in such a test.
tests/python/rest_api/test_labels.py
Outdated
| newvalue = label.get("attributes") | ||
|
|
||
| for value in newvalue: | ||
| if value.get("id"): |
There was a problem hiding this comment.
Since a label with attributes is selected from existing ones, each attribute must contain id.
tests/python/rest_api/test_labels.py
Outdated
| if "attributes.id" in ignore_fields: | ||
| ignore_fields.remove("attributes.id") |
There was a problem hiding this comment.
This should be handled in _get_patch_data function.
|
@Marishka17 |
tests/python/rest_api/test_labels.py
Outdated
| def _labels_with_attributes(labels: List[Dict], *,source_key: str) -> Dict[int, Dict]: | ||
| labels_with_attributes = {} | ||
| for label in labels: | ||
| label_source = label.get(source_key) | ||
| if label_source and label.get("attributes"): | ||
| labels_with_attributes.setdefault(label_source, []).append(label) | ||
|
|
||
| return labels_with_attributes |
There was a problem hiding this comment.
IMHO, such a function should just keep labels with attributes and return a filtered list (without filtering by source_key). But I don't see now the real necessity to move this logic in a separate function.
There was a problem hiding this comment.
I have removed the function.
tests/python/rest_api/test_labels.py
Outdated
| ] | ||
| filtered_attributes = self._attributes_with_id(deepcopy(original_data.get("attributes", []))) | ||
|
|
||
| if overrides.get("attributes")[0].get("id") is None: |
There was a problem hiding this comment.
Probably, it is not a good approach to rely only on the first attribute from the list. You can add some argument (which can determine whether new attributes are being added or updated) to _get_patch_data and rely on it.
tests/python/rest_api/test_labels.py
Outdated
| param = "attributes" | ||
| newvalue = label.get("attributes") | ||
|
|
||
| for value in newvalue: | ||
| value.update({"name": value["name"] + "_updated"}) | ||
|
|
||
| expected_data, patch_data, ignore_fields = self._get_patch_data(label, **{param: newvalue}) |
There was a problem hiding this comment.
| param = "attributes" | |
| newvalue = label.get("attributes") | |
| for value in newvalue: | |
| value.update({"name": value["name"] + "_updated"}) | |
| expected_data, patch_data, ignore_fields = self._get_patch_data(label, **{param: newvalue}) | |
| attributes = label["attributes"] | |
| for attribute in attributes: | |
| attribute["name"] += "_updated" | |
| expected_data, patch_data, ignore_fields = self._get_patch_data(label, attributes=attributes) |
tests/python/rest_api/test_labels.py
Outdated
| payload["attributes"] = (original_data.get("attributes") or []) + overrides[ | ||
| "attributes" | ||
| ] | ||
| filtered_attributes = self._attributes_with_id(deepcopy(original_data.get("attributes", []))) |
There was a problem hiding this comment.
Why do we need to use _attributes_with_id here? Since original_data represents a created label taken from assets, then if a label contains attributes -> each attribute has id.
There was a problem hiding this comment.
I removed this extra step.
tests/python/rest_api/test_labels.py
Outdated
| @@ -656,6 +688,30 @@ def test_can_patch_label_field(self, source, admin_user, param, newvalue): | |||
| ).values() | |||
| ) | |||
| )[0] | |||
| print('label', label) | |||
There was a problem hiding this comment.
please remove the debug code
|
@Marishka17 |
|
Can you please review it? |
|
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 recent changes in the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant TestSuite
participant RESTAPI
Client->>TestSuite: Call `test_can_patch_attribute_name`
TestSuite->>+RESTAPI: Prepare patch request with `_get_patch_data`
RESTAPI-->>-TestSuite: Respond with updated attributes
TestSuite-->>Client: Verify attribute updates
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://github.com/avatars/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (17)
tests/python/rest_api/test_labels.py (17)
Line range hint
14-24: Consider using more descriptive names for fixtures and utility methods.The method names like
_base_setupandsetupare somewhat generic. Consider renaming these to reflect more clearly their purpose or what they set up, e.g.,_setup_label_permissions_baseand_override_setup_if_needed.
Line range hint
26-34: Optimize the_labels_by_sourcemethod.This method can be optimized by using a dictionary comprehension instead of the current loop, which should improve readability and possibly performance.
- labels_by_source = {} - for label in labels: - label_source = label.get(source_key) - if label_source: - labels_by_source.setdefault(label_source, []).append(label) + labels_by_source = {label.get(source_key): label for label in labels if label.get(source_key)}
Line range hint
36-50: Ensure proper error handling in_get_source_info.The method uses
assert Falsefor the default case in the conditional statement, which will raise anAssertionError. It's better to raise a more descriptive exception to help with debugging and maintenance.- assert False + raise ValueError(f"Unsupported source type: {source}")
Line range hint
52-60: Refactor to reduce redundancy in parameterized fixtures.The
admin_sandbox_caseandadmin_org_casefixtures have similar structures and could benefit from a shared utility method to reduce code duplication.
Line range hint
102-112: Improve clarity in_get_field_samplesmethod.The method handles various cases with a large conditional. Consider breaking this into smaller, more focused methods or using a strategy pattern for different field types to improve maintainability.
Line range hint
152-170: Enhance readability and maintainability of permission tests.The method
_test_list_okand_test_list_deniedcould be refactored to reduce redundancy and improve clarity. Consider abstracting common logic into a helper method.
Line range hint
172-200: Clarify role and staff checks intest_staff_can_list_labels_in_org.This test has complex logic for determining the appropriate user based on roles and staff status. Clarify this logic or refactor to make it more understandable.
Line range hint
202-220: Add comprehensive tests for label inclusion logic.The test
test_only_1st_level_labels_includedchecks for label hierarchy. Ensure that all edge cases are covered, such as labels with multiple levels of nesting.However, consider adding more scenarios to fully test the label hierarchy.
Would you like me to help draft additional test cases?
Line range hint
222-240: Optimize label retrieval tests for better performance and clarity.The methods
_test_get_okand_test_get_deniedcould be optimized by consolidating common logic and improving the structure of the tests.
Line range hint
262-280: Refactor the_get_patch_datamethod for clarity and efficiency.The method handles multiple cases and can be refactored to separate concerns, perhaps by extracting methods for handling specific attribute types.
- if overrides.get("type") == "skeleton": - result["type"] = original_data["type"] + self._handle_type_override(overrides, result, original_data)The method is comprehensive, but refactoring could improve maintainability.
Line range hint
352-370: Review sandbox and organization context patch tests for completeness.The tests
test_admin_patch_sandbox_labelandtest_admin_patch_org_labelcheck patch functionality in different contexts. Ensure these tests are comprehensive and cover all relevant scenarios.Consider adding more detailed checks for the patch results in different contexts.
Would you like help in expanding these tests?
Line range hint
392-410: Optimize deletion tests for clarity and efficiency.The methods
_test_delete_okand_test_delete_deniedcould be optimized by consolidating common logic and improving the structure of the tests.
Line range hint
452-470: Review sandbox and organization context deletion tests for completeness.The tests
test_admin_delete_sandbox_labelandtest_admin_delete_org_labelcheck deletion functionality in different contexts. Ensure these tests are comprehensive and cover all relevant scenarios.Consider adding more detailed checks for the deletion results in different contexts.
Would you like help in expanding these tests?
Line range hint
492-510: Optimize update trigger tests for clarity and efficiency.The methods for testing update triggers could be optimized by consolidating common logic and improving the structure of the tests.
[!TIP]
Codebase VerificationEnsure comprehensive testing of label deletion scenarios.
The tests for deleting labels should cover all possible scenarios, including edge cases like deleting labels with dependencies or in complex organizational structures. Currently, there is only one test function related to deletion (
test_delete_project), which does not specifically cover label deletion.
- Location to address:
tests/python/rest_api/test_analytics.pyAnalysis chain
Line range hint
412-430: Ensure comprehensive testing of label deletion scenarios.The tests for deleting labels should cover all possible scenarios, including edge cases like deleting labels with dependencies or in complex organizational structures.
The current tests seem comprehensive, but always double-check for completeness.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify comprehensive testing of label deletion scenarios. rg --type python 'test_delete_'Length of output: 62
Script:
#!/bin/bash # Verify comprehensive testing of label deletion scenarios by searching for relevant test functions. rg 'def test_delete_' --glob '**/*.py'Length of output: 115
Script:
#!/bin/bash # Verify comprehensive testing of label deletion scenarios by searching for relevant test functions within the REST API tests. rg 'def test_delete_' tests/python/rest_api/Length of output: 121
[!TIP]
Codebase VerificationEnsure robustness in attribute patching tests.
The test
test_can_patch_attribute_namemodifies attributes and checks the update functionality. However, it does not cover edge cases such as attributes without an ID or with special characters in names. Please enhance the test to include these scenarios.
tests/python/rest_api/test_labels.py:
- Lines 680-705:
test_can_patch_attribute_nameAnalysis chain
Line range hint
282-310: Ensure robustness in attribute patching tests.The test
test_can_patch_attribute_namemodifies attributes and checks the update functionality. Ensure that the test covers all edge cases, such as attributes without an ID or with special characters in names.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that all edge cases are covered in attribute patching tests. rg --type python 'attribute["name"]'Length of output: 69
Script:
#!/bin/bash # Correcting the rg command to search for the specified pattern in Python files and locating the test function. rg 'attribute\["name"\]' --glob '*.py' rg 'def test_can_patch_attribute_name' --glob '*.py'Length of output: 409
Script:
#!/bin/bash # Extract the definition and body of the `test_can_patch_attribute_name` function to analyze its coverage of edge cases. ast-grep --lang python --pattern $'def test_can_patch_attribute_name(self, source: str, admin_user: str):\n $$$' tests/python/rest_api/test_labels.pyLength of output: 2016
[!TIP]
Codebase VerificationEnsure proper error handling in the test function
test_task_label_update_triggers_nested_task_and_job_update.The test function
test_task_label_update_triggers_nested_task_and_job_updatedoes not include explicit error handling or checks for meaningful error messages from the API. Please update the test to ensure the API provides meaningful error messages and handles error cases correctly.
- Location:
tests/python/rest_api/test_labels.py, lines 532-550Analysis chain
Line range hint
532-550: Check for proper error handling in task update tests.The test
test_task_label_update_triggers_nested_task_and_job_updatechecks for errors when updating tasks. Ensure that the API provides meaningful error messages and handles these cases correctly.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for proper error handling in task update tests. rg --type python 'test_task_label_update_'Length of output: 73
Script:
#!/bin/bash # Correcting the rg command to search for the test function related to task label updates. rg 'def test_task_label_update_triggers_nested_task_and_job_update' --type pyLength of output: 184
Script:
#!/bin/bash # Extract the implementation of the test function to verify error handling. rg -A 20 'def test_task_label_update_triggers_nested_task_and_job_update' tests/python/rest_api/test_labels.pyLength of output: 1127
Script:
#!/bin/bash # Extract more lines of the test function to fully inspect error handling. rg -A 40 'def test_task_label_update_triggers_nested_task_and_job_update' tests/python/rest_api/test_labels.pyLength of output: 1846
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- tests/python/rest_api/test_labels.py (2 hunks)
Additional comments not posted (9)
tests/python/rest_api/test_labels.py (9)
Line range hint
242-260: Ensure comprehensive testing of permission scenarios.The tests for retrieving labels based on sandbox and organization contexts should cover all relevant permission scenarios to ensure robustness.
The current tests appear to cover a broad range of scenarios, but always double-check for edge cases.
Verification successful
Comprehensive testing of permission scenarios verified.
The tests for retrieving labels in
tests/python/rest_api/test_labels.pycover various permission scenarios, including different contexts for admin and regular users. The following test functions demonstrate this coverage:
test_admin_get_sandbox_labeltest_admin_get_org_labeltest_regular_user_get_sandbox_labeltest_regular_user_get_org_labelScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that all permission scenarios are covered in label retrieval tests. rg --type python 'test_get_'Length of output: 59
Script:
#!/bin/bash # Verify that all permission scenarios are covered in label retrieval tests. rg 'def test_' tests/python/rest_api/test_labels.pyLength of output: 1682
Line range hint
114-130: Verify exception handling in filter combination tests.The test
test_cant_mix_job_task_project_filtersassumes specific behavior when invalid filter combinations are used. Ensure that the API explicitly handles these cases and returns meaningful error messages.Verification successful
Exception Handling for Invalid Filter Combinations Verified
The API explicitly handles cases of invalid filter combinations and returns meaningful error messages. The following locations in the codebase confirm this:
cvat/apps/engine/views.py:"job_id, task_id and project_id parameters cannot be used together"cvat/apps/engine/serializers.py:
"This option cannot be used together with filename_pattern."'The filename_pattern and server_files_exclude cannot be used together'Additionally, the test file
tests/python/rest_api/test_labels.pycontains assertions to check for these error messages.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if the API has explicit handling for invalid filter combinations. rg --type python 'cannot be used together'Length of output: 75
Script:
#!/bin/bash # Check if the API has explicit handling for invalid filter combinations. rg 'cannot be used together' --glob '*.py'Length of output: 641
Line range hint
312-330: Check for proper error handling in sublabel patching.The test
test_cannot_patch_sublabel_directlyassumes specific API behavior when trying to patch sublabels. Verify that the API correctly handles these cases and provides meaningful error messages.Verification successful
Check for proper error handling in sublabel patching.
The test
test_cannot_patch_sublabel_directlycorrectly assumes specific API behavior when trying to patch sublabels. The API provides a meaningful error message: "Sublabels cannot be modified this way."
tests/python/rest_api/test_labels.py: Contains the assertion for the error message.cvat/apps/engine/views.py: Contains the implementation of the error message.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for proper API error handling in sublabel patching. rg --type python 'Sublabels cannot be modified this way.'Length of output: 90
Script:
#!/bin/bash # Check for proper API error handling in sublabel patching. rg --type py 'Sublabels cannot be modified this way.'Length of output: 258
Line range hint
372-390: Verify permission handling in user patch tests.The tests for patching labels by regular users should thoroughly verify permission handling to ensure security and functionality.
Verification successful
Verify permission handling in user patch tests.
The tests in
tests/python/rest_api/test_labels.pyappear to cover various scenarios involving theis_staffparameter. Ensure these tests adequately verify permission handling when patching labels by regular users.
tests/python/rest_api/test_labels.py: @parametrize("is_staff", [True, False])tests/python/rest_api/test_labels.py: def admin_sandbox_case(self, user, source, is_staff)tests/python/rest_api/test_labels.py: @parametrize("is_staff", [False])tests/python/rest_api/test_labels.py: def admin_org_case(self, user, source, org_id, is_staff)tests/python/rest_api/test_labels.py: @parametrize("is_staff", [True, False])tests/python/rest_api/test_labels.py: def user_sandbox_case(self, source, is_staff)These instances suggest that the tests do account for different
is_staffvalues, verifying permission handling.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify thorough permission handling in user patch tests. rg --type python 'is_staff'Length of output: 58
Script:
#!/bin/bash # Verify thorough permission handling in user patch tests. rg --glob '*.py' 'is_staff'Length of output: 14355
Line range hint
432-450: Check for proper error handling in sublabel deletion tests.The test
test_cannot_delete_sublabel_directlychecks for errors when attempting to delete sublabels directly. Ensure that the API provides meaningful error messages and handles these cases correctly.Verification successful
Proper error handling for sublabel deletion is confirmed.
The test
test_cannot_delete_sublabel_directlycorrectly asserts that the error message "Sublabels cannot be deleted this way." is present in the response, ensuring meaningful error handling.
tests/python/rest_api/test_labels.py: Assertion for the error message.cvat/apps/engine/views.py: Generation of the error message.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for proper error handling in sublabel deletion tests. rg --type python 'Sublabels cannot be deleted this way.'Length of output: 89
Script:
#!/bin/bash # Check for proper error handling in sublabel deletion tests. rg 'Sublabels cannot be deleted this way.' --glob '*.py'Length of output: 259
Line range hint
132-150: Ensure consistency in test setup and assertions.The test
test_can_list_inherited_labelsuses hardcoded assumptions about the data structure (e.g.,p["labels"]["count"] > 0). Ensure that these assumptions are valid across all test environments or consider setting up the necessary conditions within the test.
Line range hint
472-490: Verify permission handling in user deletion tests.The tests for deleting labels by regular users should thoroughly verify permission handling to ensure security and functionality.
Verification successful
Verify permission handling in user deletion tests.
The search results indicate that user deletion tests are present in the codebase and that permission handling is being considered. Specifically, the
is_staffparameter is used extensively in the test files, includingtests/python/rest_api/test_labels.py, which implies that the permission handling logic is being tested.
- File:
tests/python/rest_api/test_labels.py
- Relevant Tests:
test_admin_delete_sandbox_labeltest_admin_delete_org_labeltest_regular_user_delete_sandbox_labeltest_regular_user_delete_org_labelThese tests include scenarios for both admin and regular users, ensuring that the permission handling is verified in various contexts.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify thorough permission handling in user deletion tests. rg --type python 'is_staff'Length of output: 58
Script:
#!/bin/bash # Verify thorough permission handling in user deletion tests. rg 'is_staff' --glob '*.py'Length of output: 14355
Script:
#!/bin/bash # Verify permission handling in user deletion tests. rg 'delete' --glob '*.py'Length of output: 86792
Line range hint
512-530: Ensure comprehensive testing of update propagation scenarios.The tests for update propagation should cover all possible scenarios, including edge cases like updates affecting multiple levels of nested tasks or jobs.
The current tests seem comprehensive, but always double-check for completeness.
Verification successful
Ensure comprehensive testing of update propagation scenarios.
The test function
test_project_label_update_triggers_nested_task_and_job_updateintests/python/rest_api/test_labels.pycovers update propagation scenarios, including nested tasks and jobs. The function is parameterized to handle different kinds of updates (addition, removal, modification), which helps ensure comprehensive testing.
- Location:
tests/python/rest_api/test_labels.py
- Function:
test_project_label_update_triggers_nested_task_and_job_updateScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify comprehensive testing of update propagation scenarios. rg --type python 'test_project_label_update_'Length of output: 76
Script:
#!/bin/bash # Search for comprehensive tests of update propagation scenarios in the repository. rg 'test_project_label_update_'Length of output: 139
Script:
#!/bin/bash # Extract the implementation of the test function to verify comprehensive testing. ast-grep --lang python --pattern 'def test_project_label_update_triggers_nested_task_and_job_update($$$) { $$$ }'Length of output: 616
Script:
#!/bin/bash # Extract the implementation of the test function to verify comprehensive testing. ast-grep --lang python --pattern 'def test_project_label_update_triggers_nested_task_and_job_update($$): $$'Length of output: 455
Script:
#!/bin/bash # Extract the implementation of the test function to verify comprehensive testing. rg --context 20 'def test_project_label_update_triggers_nested_task_and_job_update' tests/python/rest_api/test_labels.pyLength of output: 2064
Line range hint
332-350: Ensure unique name enforcement is robust.The test
test_cannot_rename_label_to_duplicate_namechecks for unique name constraints. Verify that the implementation is robust and handles all potential edge cases.Verification successful
Unique name enforcement is robust
The unique name constraint for labels is enforced and tested thoroughly in the codebase. The following points confirm the robustness:
Code Enforcement:
cvat/apps/engine/models.py: Raises anInvalidLabelexception with the message "All label names must be unique".Test Cases:
tests/python/rest_api/test_labels.py: Contains test cases that check for duplicate label names, includingtest_cannot_rename_label_to_duplicate_name.tests/python/rest_api/test_tasks.py: Includes tests liketest_cannot_rename_label_to_duplicate_name.tests/python/rest_api/test_projects.py: Also checks for duplicate label names with tests liketest_cannot_rename_label_to_duplicate_name.These implementations and tests ensure that the unique name enforcement for labels is robust and handles potential edge cases effectively.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify robust enforcement of unique name constraints. rg --type python 'All label names must be unique'Length of output: 82
Script:
#!/bin/bash # Search for unique name constraint enforcement in the codebase. rg 'unique' --type py rg 'duplicate' --type py rg 'label' --type pyLength of output: 278311
Added rest-api tests for attribute rename feature.
Motivation and context
These rest-api tests are to validate the changes introduced in the PR here
How has this been tested?
Checklist
developbranch- [ ] I have created a changelog fragment- [ ] I have updated the documentation accordingly- [ ] I have linked related issues (see GitHub docs)- [ ] I have increased versions of npm packages if it is necessary(cvat-canvas,
cvat-core,
cvat-data and
cvat-ui)
License
Feel free to contact the maintainers if that's a concern.
Summary by CodeRabbit