ocket8888 commented on code in PR #7424: URL: https://github.com/apache/trafficcontrol/pull/7424#discussion_r1159028422
########## traffic_ops/testing/api_contract/v4/test_cachegroups.py: ########## @@ -0,0 +1,100 @@ +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +# + +"""API Contract Test Case for cachegroup endpoint.""" +import logging +import pytest +import requests + +from trafficops.tosession import TOSession + +# Create and configure logger +logger = logging.getLogger() + +primitive = bool | int | float | str | None + [email protected]('request_template_data', ["cachegroup"], indirect=True) +def test_cachegroup_contract(to_session: TOSession, request_template_data: + list[dict[str, object] | list[object] | primitive], + response_template_data: dict[str, object],cachegroup_post_data: dict[str, object]) -> None: + """ + Test step to validate keys, values and data types from cachegroup endpoint + response. + :param to_session: Fixture to get Traffic Ops session. + :param request_template_data: Fixture to get request template data from a prerequisites file. + :param response_template_data: Fixture to get response template data from a prerequisites file. + :param cachegroup_post_data: Fixture to get sample cachegroup data and actual cachegroup response. + """ + # validate CDN keys from cdns get response + logger.info("Accessing /cachegroup endpoint through Traffic ops session.") + + cachegroup = request_template_data[0] + if not isinstance(cachegroup, dict): + raise TypeError("malformed cachegroup in prerequisite data; not an object") + + cachegroup_name = cachegroup.get("name") + if not isinstance(cachegroup_name, str): + raise TypeError("malformed cachegroup in prerequisite data; 'name' not a string") + + cachegroup_get_response: tuple[ + dict[str, object] | list[dict[str, object] | list[object] | primitive] | primitive, + requests.Response + ] = to_session.get_cachegroups(query_params={"name": str(cachegroup_name)}) + try: + cachegroup_data = cachegroup_get_response[0] + if not isinstance(cachegroup_data, list): + raise TypeError("malformed API response; 'response' property not an array") + + first_cachegroup = cachegroup_data[0] + if not isinstance(first_cachegroup, dict): + raise TypeError("malformed API response; first Cache group in response is not an object") + 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") Review Comment: > 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). ########## traffic_ops/testing/api_contract/v4/test_cachegroups.py: ########## @@ -0,0 +1,100 @@ +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +# + +"""API Contract Test Case for cachegroup endpoint.""" +import logging +import pytest +import requests + +from trafficops.tosession import TOSession + +# Create and configure logger +logger = logging.getLogger() + +primitive = bool | int | float | str | None + [email protected]('request_template_data', ["cachegroup"], indirect=True) +def test_cachegroup_contract(to_session: TOSession, request_template_data: + list[dict[str, object] | list[object] | primitive], + response_template_data: dict[str, object],cachegroup_post_data: dict[str, object]) -> None: + """ + Test step to validate keys, values and data types from cachegroup endpoint + response. + :param to_session: Fixture to get Traffic Ops session. + :param request_template_data: Fixture to get request template data from a prerequisites file. + :param response_template_data: Fixture to get response template data from a prerequisites file. + :param cachegroup_post_data: Fixture to get sample cachegroup data and actual cachegroup response. + """ + # validate CDN keys from cdns get response + logger.info("Accessing /cachegroup endpoint through Traffic ops session.") + + cachegroup = request_template_data[0] + if not isinstance(cachegroup, dict): + raise TypeError("malformed cachegroup in prerequisite data; not an object") + + cachegroup_name = cachegroup.get("name") + if not isinstance(cachegroup_name, str): + raise TypeError("malformed cachegroup in prerequisite data; 'name' not a string") + + cachegroup_get_response: tuple[ + dict[str, object] | list[dict[str, object] | list[object] | primitive] | primitive, + requests.Response + ] = to_session.get_cachegroups(query_params={"name": str(cachegroup_name)}) + try: + cachegroup_data = cachegroup_get_response[0] + if not isinstance(cachegroup_data, list): + raise TypeError("malformed API response; 'response' property not an array") + + first_cachegroup = cachegroup_data[0] + if not isinstance(first_cachegroup, dict): + raise TypeError("malformed API response; first Cache group in response is not an object") + 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") + # validate cachegroup values from prereq data in cachegroup get response. + prereq_values = [cachegroup_post_data["name"],cachegroup_post_data["shortName"], + cachegroup_post_data["fallbackToClosest"],cachegroup_post_data["typeId"]] + get_values = [first_cachegroup["name"],first_cachegroup["shortName"], + first_cachegroup["fallbackToClosest"],first_cachegroup["typeId"]] + get_types = {} + for key in first_cachegroup: + get_types[key] = type(first_cachegroup[key]).__name__ Review Comment: 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 ```python3 for key, value in first_cachegroup.items(): get_types[key] = type(value).__name__ ``` ########## traffic_ops/testing/api_contract/v4/test_cachegroups.py: ########## @@ -0,0 +1,100 @@ +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +# + +"""API Contract Test Case for cachegroup endpoint.""" +import logging +import pytest +import requests + +from trafficops.tosession import TOSession + +# Create and configure logger +logger = logging.getLogger() + +primitive = bool | int | float | str | None + [email protected]('request_template_data', ["cachegroup"], indirect=True) +def test_cachegroup_contract(to_session: TOSession, request_template_data: + list[dict[str, object] | list[object] | primitive], + response_template_data: dict[str, object],cachegroup_post_data: dict[str, object]) -> None: + """ + Test step to validate keys, values and data types from cachegroup endpoint + response. + :param to_session: Fixture to get Traffic Ops session. + :param request_template_data: Fixture to get request template data from a prerequisites file. + :param response_template_data: Fixture to get response template data from a prerequisites file. + :param cachegroup_post_data: Fixture to get sample cachegroup data and actual cachegroup response. + """ + # validate CDN keys from cdns get response + logger.info("Accessing /cachegroup endpoint through Traffic ops session.") + + cachegroup = request_template_data[0] + if not isinstance(cachegroup, dict): + raise TypeError("malformed cachegroup in prerequisite data; not an object") + + cachegroup_name = cachegroup.get("name") + if not isinstance(cachegroup_name, str): + raise TypeError("malformed cachegroup in prerequisite data; 'name' not a string") + + cachegroup_get_response: tuple[ + dict[str, object] | list[dict[str, object] | list[object] | primitive] | primitive, + requests.Response + ] = to_session.get_cachegroups(query_params={"name": str(cachegroup_name)}) + try: + cachegroup_data = cachegroup_get_response[0] + if not isinstance(cachegroup_data, list): + raise TypeError("malformed API response; 'response' property not an array") + + first_cachegroup = cachegroup_data[0] + if not isinstance(first_cachegroup, dict): + raise TypeError("malformed API response; first Cache group in response is not an object") + 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") + # validate cachegroup values from prereq data in cachegroup get response. + prereq_values = [cachegroup_post_data["name"],cachegroup_post_data["shortName"], + cachegroup_post_data["fallbackToClosest"],cachegroup_post_data["typeId"]] + get_values = [first_cachegroup["name"],first_cachegroup["shortName"], + first_cachegroup["fallbackToClosest"],first_cachegroup["typeId"]] + get_types = {} + for key in first_cachegroup: + get_types[key] = type(first_cachegroup[key]).__name__ Review Comment: 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: ```pycon >>> import munch >>> munch.Munch.__name__ 'Munch' >>> class Munch: ... pass ... >>> Munch.__name__ 'Munch' >>> Munch.__name__ == munch.Munch.__name__ True >>> isinstance(Munch(), munch.Munch) False ``` ########## traffic_ops/testing/api_contract/v4/test_cachegroups.py: ########## @@ -0,0 +1,100 @@ +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +# + +"""API Contract Test Case for cachegroup endpoint.""" +import logging +import pytest +import requests + +from trafficops.tosession import TOSession + +# Create and configure logger +logger = logging.getLogger() + +primitive = bool | int | float | str | None + [email protected]('request_template_data', ["cachegroup"], indirect=True) +def test_cachegroup_contract(to_session: TOSession, request_template_data: + list[dict[str, object] | list[object] | primitive], + response_template_data: dict[str, object],cachegroup_post_data: dict[str, object]) -> None: + """ + Test step to validate keys, values and data types from cachegroup endpoint + response. + :param to_session: Fixture to get Traffic Ops session. + :param request_template_data: Fixture to get request template data from a prerequisites file. + :param response_template_data: Fixture to get response template data from a prerequisites file. + :param cachegroup_post_data: Fixture to get sample cachegroup data and actual cachegroup response. + """ + # validate CDN keys from cdns get response + logger.info("Accessing /cachegroup endpoint through Traffic ops session.") + + cachegroup = request_template_data[0] + if not isinstance(cachegroup, dict): + raise TypeError("malformed cachegroup in prerequisite data; not an object") + + cachegroup_name = cachegroup.get("name") + if not isinstance(cachegroup_name, str): + raise TypeError("malformed cachegroup in prerequisite data; 'name' not a string") + + cachegroup_get_response: tuple[ + dict[str, object] | list[dict[str, object] | list[object] | primitive] | primitive, + requests.Response + ] = to_session.get_cachegroups(query_params={"name": str(cachegroup_name)}) + try: + cachegroup_data = cachegroup_get_response[0] + if not isinstance(cachegroup_data, list): + raise TypeError("malformed API response; 'response' property not an array") + + first_cachegroup = cachegroup_data[0] + if not isinstance(first_cachegroup, dict): + raise TypeError("malformed API response; first Cache group in response is not an object") + 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") + # validate cachegroup values from prereq data in cachegroup get response. + prereq_values = [cachegroup_post_data["name"],cachegroup_post_data["shortName"], + cachegroup_post_data["fallbackToClosest"],cachegroup_post_data["typeId"]] + get_values = [first_cachegroup["name"],first_cachegroup["shortName"], + first_cachegroup["fallbackToClosest"],first_cachegroup["typeId"]] + get_types = {} + for key in first_cachegroup: + get_types[key] = type(first_cachegroup[key]).__name__ + logger.info("types from cachegroup get response %s", get_types) + response_template_types= {} + 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") Review Comment: 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. ########## traffic_ops/testing/api_contract/v4/conftest.py: ########## @@ -293,18 +300,81 @@ def to_login(to_args: ArgsType) -> TOSession: return to_session [email protected](name="request_template_data", scope="session") +def request_prerequiste_data(pytestconfig: pytest.Config, request: pytest.FixtureRequest + ) -> list[dict[str, object] | list[object] | primitive]: + """ + PyTest Fixture to store POST request template data for api endpoint. + :param pytestconfig: Session-scoped fixture that returns the session's pytest.Config object. + :param request: Fixture to access information about the requesting test function and its fixtures + + :returns: Prerequisite request data for api endpoint. + """ + request_template_path = pytestconfig.getoption("--request-template") + if not isinstance(request_template_path, str): + # unlike the configuration file, this must be present + raise ValueError("prereqisites path not configured") + + # Response keys for api endpoint + data: dict[ + str, + list[dict[str, object] | list[object] | primitive] |\ + dict[object, object] |\ + primitive + ] |\ + primitive = None + with open(request_template_path, encoding="utf-8", mode="r") as prereq_file: + data = json.load(prereq_file) + if not isinstance(data, dict): + raise TypeError(f"request template data must be an object, not '{type(data)}'") + request_template = data[request.param] + if not isinstance(request_template, list): + raise TypeError(f"Request template data must be a list, not '{type(request_template)}'") + + return request_template + [email protected]() +def response_template_data(pytestconfig: pytest.Config, + ) -> dict[str, object]: + """ + PyTest Fixture to store response template data for api endpoint. + :param pytestconfig: Session-scoped fixture that returns the session's pytest.Config object. + :returns: Prerequisite response data for api endpoint. + """ + prereq_path = pytestconfig.getoption("--response-template") + if not isinstance(prereq_path, str): + # unlike the configuration file, this must be present + raise ValueError("prereqisites path not configured") + + # Response keys for api endpoint + response_template: dict[ + str, + list[dict[str, object] | list[object] | primitive] |\ + dict[object, object] |\ + primitive + ] |\ + primitive = None + with open(prereq_path, encoding="utf-8", mode="r") as prereq_file: + response_template = json.load(prereq_file) + if not isinstance(response_template, dict): + raise TypeError(f"Response template data must be an object, not '{type(response_template)}'") + + return response_template Review Comment: the type of `response_template` doesn't match this function's declared return type ########## traffic_ops/testing/api_contract/v4/test_cdns.py: ########## @@ -98,21 +65,29 @@ def test_cdn_contract( cdn_keys = set(first_cdn.keys()) logger.info("CDN Keys from cdns endpoint response %s", cdn_keys) + response_template = response_template_data.get("cdns").get("properties") # validate cdn values from prereq data in cdns get response. prereq_values = [ cdn_post_data["name"], cdn_post_data["domainName"], cdn_post_data["dnssecEnabled"] ] get_values = [first_cdn["name"], first_cdn["domainName"], first_cdn["dnssecEnabled"]] - # validate data types for values from cdn get json response. - for (prereq_value, get_value) in zip(prereq_values, get_values): - assert isinstance(prereq_value, type(get_value)) - assert cdn_keys == set(cdn_post_data.keys()) + 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) Review Comment: this section has all the same problems I talked about with cachegroups above, since they're doing things the same way now. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
