-
Notifications
You must be signed in to change notification settings - Fork 353
Added Api contract test case for cachegroup endpoint #7424
Added Api contract test case for cachegroup endpoint #7424
Conversation
… endpoints and refactored the previous code
| cachegroup_keys = set(first_cachegroup.keys()) | ||
|
|
||
| logger.info("Cache group Keys from cachegroup endpoint response %s", cachegroup_keys) | ||
| response_template = response_template_data.get("cachegroup").get("properties") |
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.
Sorry, My mistake, The type of response_template_data should be dict[str, object], Updated this in code.
Actually, you're doing a .get on the result of that .get. Then you store that result and later do a .get on it and then a .get on that. So the type has to be dict[str, dict[str, dict[str, dict[str, object]]]] or something more specific assignable to that.
But updating that typing isn't enough to completely fix it, because since you're using .get to do safe access, you can get None in addition to the value type. So right now you have response_template_data which has the type dict[str, dict[str, dict[str, dict[str, object]]]], and therefore response_template_data.get("cachegroup") has the type dict[str, dict[str, dict[str, object]]] | None, and you can't do .get on that type because None has no get method. You need to deal with the possibility that the data you're accessing doesn't exist. You can do that by checking for None a bunch, or you can do it by except-ing a KeyError (or even not except-ing it if the caller can be expected to handle it well enough).
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.
Added checks for the response template data. added except -ing a Keyerror.
| for key in first_cachegroup: | ||
| get_types[key] = type(first_cachegroup[key]).__name__ |
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.
you can avoid the whole indexing of things by just iterating the key-value pairs instead of iterating the keys and then retrieving the values, so
for key, value in first_cachegroup.items():
get_types[key] = type(value).__name__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.
Modified the code by iterating the key-value pairs.
| for key in response_template: | ||
| optional = response_template.get(key).get("optional") | ||
| if optional: | ||
| if key in cachegroup: | ||
| response_template_types[key] = response_template.get(key).get("typeA") | ||
| else: | ||
| response_template_types[key] = response_template.get(key).get("typeB") | ||
| else: | ||
| response_template_types[key] = response_template.get(key).get("type") |
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.
the above advice is even more useful here, because you have the same issue with .get possibly returning None as in the previous comment about typing.
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.
Added checks for the response template types and added except -ing a Keyerror.
| first_cachegroup["fallbackToClosest"],first_cachegroup["typeId"]] | ||
| get_types = {} | ||
| for key in first_cachegroup: | ||
| get_types[key] = type(first_cachegroup[key]).__name__ |
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 get the name of the type instead of just the type? Then you can use isinstance to ensure that you have an actual instance of the type, instead of just a type of the same name - because those are in no way guaranteed to be unique. For example, I can make a new Munch class that has the same name as the munch package's Munch class, but is not related to the type in any way:
>>> import munch
>>> munch.Munch.__name__
'Munch'
>>> class Munch:
... pass
...
>>> Munch.__name__
'Munch'
>>> Munch.__name__ == munch.Munch.__name__
True
>>> isinstance(Munch(), munch.Munch)
FalseThere 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.
yes, But in my case, I'm not actually validating the instance of the object.
I have hardcoded response template json file.
Example:
"cdns": {
"type": "object",
"properties": {
"name": {
"type": "str"
},
"domainName": {
"type": "str"
},
"dnssecEnabled": {
"type": "bool"
},
"id": {
"type": "int"
},
"lastUpdated": {
"type": "str"
}
}
}
Here i'm checking the "type" in the response template with the key datatype from api get response.
Thats why i'm extracting just the name of the value's object from the actual response and comparing it with the "type" value in response template
| if not isinstance(response_template, dict): | ||
| raise TypeError(f"Response template data must be an object, not '{type(response_template)}'") | ||
|
|
||
| return response_template |
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.
the type of response_template doesn't match this function's declared return type
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.
yes, My mistake, Updated the response_template return type in function's declaration.
| get_types = {} | ||
| for key in first_cdn: | ||
| get_types[key] = type(first_cdn[key]).__name__ | ||
| logger.info("types from cdn get response %s", get_types) | ||
| response_template_types= {} | ||
| for key in response_template: | ||
| response_template_types[key] = response_template.get(key).get("type") | ||
| logger.info("types from cdn response template %s", response_template_types) |
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 section has all the same problems I talked about with cachegroups above, since they're doing things the same way now.
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.
Changed this testcase code based on the comments from cachegroup test case.
| cachegroup_response_template = response_template_data.get("cachegroup") | ||
| response_template = cachegroup_response_template.get("properties") if isinstance( | ||
| cachegroup_response_template, dict) else None | ||
| if response_template is None or not isinstance(response_template, dict): |
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.
Checking if an object is of type dict guards the type so that it narrows to dict[Unknown, Unknown]. So there are still some typing issues stemming from the fact that beyond this point the type of response_template is partially Unknown, meaning that when you do .items below, the returned key and value are both Unknown, which also is infecting everything you do with them.
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.
Added type annotation for response template to avoid typing issues for both cachegroups and cdns test cases.
| response_template: dict[str, list[dict[str, object] | list[object] | primitive] |\ | ||
| dict[object, object] |\ | ||
| primitive | ||
| ] |\ |
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 don't know why pytest doesn't fail on this, but when I try to do something like this in my terminal I get a TypeError because the syntax is incorrect:
Python 3.10.6 (main, Mar 10 2023, 10:55:28) [GCC 11.3.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> a: None | a = None
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
TypeError: unsupported operand type(s) for |: 'NoneType' and 'NoneType'
>>> 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.
Resolved syntax issues.
ericholguin
left a comment
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.
LGTM. Tests are passing
Adding API Contract Test Case for cachegroup endpoint and refactored some of the code in conftest and cdns test case.
Which Traffic Control components are affected by this PR?
What is the best way to verify this PR?
Step 1 : Install requirements
pip install -r requirements.txt
Step 2 : To execute cachegroup API Contract Testcase , navigate to traffic_ops/testing/api_contract/v4
pytest --to-user Username --to-password Password --to-url URL --config Config Path --request-template Request template --response-template Response template test_cachegroups.py
Note: Config, Request template and response template paths are optional.
PR submission checklist