-
Notifications
You must be signed in to change notification settings - Fork 2.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[GSoC2024] - add/rest-api-tests-for-attribute-rename #7754
base: develop
Are you sure you want to change the base?
[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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since a label with attributes is selected from existing ones, each attribute must contain id
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be handled in _get_patch_data
function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please remove the debug code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
@Marishka17 |
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
develop
branch- [ ] 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.