ocket8888 commented on code in PR #7400: URL: https://github.com/apache/trafficcontrol/pull/7400#discussion_r1143595558
########## traffic_ops/testing/api_contract/v4/conftest.py: ########## @@ -0,0 +1,122 @@ +# +# 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. +# + +"""This module is used to create a Traffic Ops session +and to store prerequisite data for endpoints.""" +import json +import logging +import sys +from random import randint +from urllib.parse import urlparse +import pytest +from trafficops.tosession import TOSession +from trafficops.restapi import OperationError + + +# Create and configure logger +logger = logging.getLogger() + + +def pytest_addoption(parser: object) -> None: Review Comment: The parser type is known to be more specific than a generic `object`. Specifically it's a [`pytest.Parser`](https://docs.pytest.org/en/7.1.x/reference/reference.html#pytest.Parser) instance. And that's necessary because `object` isn't specific enough: ```python-traceback Traceback (most recent call last): File "<stdin>", line 1, in <module> AttributeError: 'object' object has no attribute 'addoption' ``` ... is what happens if you try to call an `addoption` method on a simple `object`. ########## traffic_ops/testing/api_contract/v4/conftest.py: ########## @@ -0,0 +1,122 @@ +# +# 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. +# + +"""This module is used to create a Traffic Ops session +and to store prerequisite data for endpoints.""" +import json +import logging +import sys +from random import randint +from urllib.parse import urlparse +import pytest +from trafficops.tosession import TOSession +from trafficops.restapi import OperationError + + +# Create and configure logger +logger = logging.getLogger() + + +def pytest_addoption(parser: object) -> None: + """Passing in Traffic Ops arguments [Username, Password, Url and Hostname] from command line. + :param parser: Parser to parse command line arguments + """ + parser.addoption( + "--to-user", action="store", help="User name for Traffic Ops Session." + ) + parser.addoption( + "--to-password", action="store", help="Password for Traffic Ops Session." + ) + parser.addoption( + "--to-url", action="store", help="Traffic Ops URL." + ) + + [email protected](name="to_args") +def to_data(pytestconfig: pytest.Config) -> dict: + """PyTest fixture to store Traffic ops arguments passed from command line. + :param pytestconfig: Session-scoped fixture that returns the session's pytest.Config object + :returns args: Return Traffic Ops arguments + """ + args = {} + with open("to_data.json", encoding="utf-8", mode="r") as session_file: + data = json.load(session_file) + session_data = data["test"] + args["api_version"] = urlparse( + session_data.get("url")).path.strip("/").split("/")[1] + args["port"] = session_data.get("port") + + to_user = pytestconfig.getoption("--to-user") + to_password = pytestconfig.getoption("--to-password") + to_url = pytestconfig.getoption("--to-url") + + if not all([to_user, to_password, to_url]): + logger.info( + "Traffic Ops session data were not passed from Command line Args.") + args["user"] = session_data.get("user") + args["password"] = session_data.get("password") + args["url"] = session_data.get("url") + else: + args["user"] = to_user + args["password"] = to_password + args["url"] = to_url + logger.info("Parsed Traffic ops session data from args %s", args) Review Comment: this means that if I pass one of these things on the command-line, I need to pass all of them. I'd like to be able to override the configuration file for just one property - probably the URL or port - without needing to reiterate the others. ########## traffic_ops/testing/api_contract/v4/conftest.py: ########## @@ -0,0 +1,122 @@ +# +# 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. +# + +"""This module is used to create a Traffic Ops session +and to store prerequisite data for endpoints.""" +import json +import logging +import sys +from random import randint +from urllib.parse import urlparse +import pytest +from trafficops.tosession import TOSession +from trafficops.restapi import OperationError + + +# Create and configure logger +logger = logging.getLogger() + + +def pytest_addoption(parser: object) -> None: + """Passing in Traffic Ops arguments [Username, Password, Url and Hostname] from command line. + :param parser: Parser to parse command line arguments + """ + parser.addoption( + "--to-user", action="store", help="User name for Traffic Ops Session." + ) + parser.addoption( + "--to-password", action="store", help="Password for Traffic Ops Session." + ) + parser.addoption( + "--to-url", action="store", help="Traffic Ops URL." + ) + + [email protected](name="to_args") +def to_data(pytestconfig: pytest.Config) -> dict: Review Comment: The type returned by this function is known to be more specific than just `dict`; specifically I think it should be a `dict[str, Optional[int | str]]`. But honestly I think a `typing.NamedTuple` would be better, something like ```python3 ArgsType = NamedTuple("ArgsType", [("user", str), ("password", str), ("url", str), ("port", int)]) ``` would work but personally I think ```python3 class ArgsType(NamedTuple): user: str password: str url: str port: int ``` is easier to read - and also you can attach a docstring to it, which is a nice bonus. Plus, this way you can expect the returned value to actually have all of its properties defined, whereas with just a `dict` you don't know what the keys are, and currently the values of those keys are allowed to be `None`. ########## traffic_ops/testing/api_contract/v4/conftest.py: ########## @@ -0,0 +1,122 @@ +# +# 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. +# + +"""This module is used to create a Traffic Ops session +and to store prerequisite data for endpoints.""" +import json +import logging +import sys +from random import randint +from urllib.parse import urlparse +import pytest +from trafficops.tosession import TOSession +from trafficops.restapi import OperationError + + +# Create and configure logger +logger = logging.getLogger() + + +def pytest_addoption(parser: object) -> None: + """Passing in Traffic Ops arguments [Username, Password, Url and Hostname] from command line. + :param parser: Parser to parse command line arguments + """ + parser.addoption( + "--to-user", action="store", help="User name for Traffic Ops Session." + ) + parser.addoption( + "--to-password", action="store", help="Password for Traffic Ops Session." + ) + parser.addoption( + "--to-url", action="store", help="Traffic Ops URL." + ) + + [email protected](name="to_args") +def to_data(pytestconfig: pytest.Config) -> dict: + """PyTest fixture to store Traffic ops arguments passed from command line. + :param pytestconfig: Session-scoped fixture that returns the session's pytest.Config object + :returns args: Return Traffic Ops arguments + """ + args = {} + with open("to_data.json", encoding="utf-8", mode="r") as session_file: + data = json.load(session_file) + session_data = data["test"] + args["api_version"] = urlparse( + session_data.get("url")).path.strip("/").split("/")[1] + args["port"] = session_data.get("port") + + to_user = pytestconfig.getoption("--to-user") + to_password = pytestconfig.getoption("--to-password") + to_url = pytestconfig.getoption("--to-url") + + if not all([to_user, to_password, to_url]): + logger.info( + "Traffic Ops session data were not passed from Command line Args.") + args["user"] = session_data.get("user") + args["password"] = session_data.get("password") + args["url"] = session_data.get("url") + else: + args["user"] = to_user + args["password"] = to_password + args["url"] = to_url + logger.info("Parsed Traffic ops session data from args %s", args) + return args + + [email protected](name="to_session") +def to_login(to_args: dict) -> TOSession: Review Comment: assuming this `to_args` is the fixture by the same name above, the same notes on the return type thereof also apply to the `to_args` argument ########## traffic_ops/testing/api_contract/v4/to_data.json: ########## @@ -0,0 +1,8 @@ +{ + "test": { Review Comment: this whole object is wrapped in the `"test"` property - but why? Is the plan to add other top-level properties besides `"test"`? ########## traffic_ops/testing/api_contract/v4/test_cdns.py: ########## @@ -0,0 +1,72 @@ +# +# 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 cdns endpoint.""" +import json +import logging +import pytest + +# Create and configure logger +logger = logging.getLogger() + + [email protected](name="get_cdn_data") +def get_cdn_prereq_data() -> dict: Review Comment: this isn't known to be a `dict`; it could be anything or even nothing, depending on the contents of the prerequisites file. So really you only know that it's returning some `object`. ########## traffic_ops/testing/api_contract/v4/conftest.py: ########## @@ -0,0 +1,122 @@ +# +# 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. +# + +"""This module is used to create a Traffic Ops session +and to store prerequisite data for endpoints.""" +import json +import logging +import sys +from random import randint +from urllib.parse import urlparse +import pytest +from trafficops.tosession import TOSession +from trafficops.restapi import OperationError + + +# Create and configure logger +logger = logging.getLogger() + + +def pytest_addoption(parser: object) -> None: + """Passing in Traffic Ops arguments [Username, Password, Url and Hostname] from command line. + :param parser: Parser to parse command line arguments + """ + parser.addoption( + "--to-user", action="store", help="User name for Traffic Ops Session." + ) + parser.addoption( + "--to-password", action="store", help="Password for Traffic Ops Session." + ) + parser.addoption( + "--to-url", action="store", help="Traffic Ops URL." + ) + + [email protected](name="to_args") +def to_data(pytestconfig: pytest.Config) -> dict: + """PyTest fixture to store Traffic ops arguments passed from command line. + :param pytestconfig: Session-scoped fixture that returns the session's pytest.Config object + :returns args: Return Traffic Ops arguments + """ + args = {} + with open("to_data.json", encoding="utf-8", mode="r") as session_file: + data = json.load(session_file) + session_data = data["test"] + args["api_version"] = urlparse( + session_data.get("url")).path.strip("/").split("/")[1] + args["port"] = session_data.get("port") + + to_user = pytestconfig.getoption("--to-user") + to_password = pytestconfig.getoption("--to-password") + to_url = pytestconfig.getoption("--to-url") + + if not all([to_user, to_password, to_url]): + logger.info( + "Traffic Ops session data were not passed from Command line Args.") + args["user"] = session_data.get("user") + args["password"] = session_data.get("password") + args["url"] = session_data.get("url") + else: + args["user"] = to_user + args["password"] = to_password + args["url"] = to_url + logger.info("Parsed Traffic ops session data from args %s", args) + return args + + [email protected](name="to_session") +def to_login(to_args: dict) -> TOSession: + """PyTest Fixture to create a Traffic Ops session from Traffic Ops Arguments + passed as command line arguments in to_args fixture in conftest. + :param to_args: Fixture to get Traffic ops session arguments + :returns to_session: Return Traffic ops session + """ + # Create a Traffic Ops V4 session and login + to_url = urlparse(to_args["url"]) + to_host = to_url.hostname + try: + to_session = TOSession(host_ip=to_host, host_port=to_args["port"], + api_version=to_args["api_version"], ssl=True, verify_cert=False) + logger.info("Established Traffic Ops Session.") + except OperationError: + sys.exit(-1) + + # Login To TO_API + to_session.login(to_args["user"], to_args["password"]) + logger.info("Successfully logged into Traffic Ops.") + return to_session + + [email protected]() +def cdn_prereq(to_session: TOSession, get_cdn_data: dict) -> list: Review Comment: This can return `None` in addition to a list, so it should be annotated with `Optional`. Also, the list has a more specific type than `list[any]`, it's `list[dict[str, str] | CDN]` but idk how you wanna type a CDN. It's meant to be data read in from a file that is used to create a CDN, so you can probably make that type pretty specific. Speaking of, though, these names confuse me. This fixture is named `cdn_prereq`, but what it returns is data from the API - in a *POST* request, too, not *GET* - and the input argument `get_cdn_data` isn't actually retrieved from the API in a *GET* request, that's the prerequisite data? The same seeming mismatch is occurring in `test_cdns.py`. ########## traffic_ops/testing/api_contract/v4/test_cdns.py: ########## @@ -0,0 +1,72 @@ +# +# 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 cdns endpoint.""" +import json +import logging +import pytest + +# Create and configure logger +logger = logging.getLogger() + + [email protected](name="get_cdn_data") +def get_cdn_prereq_data() -> dict: + """PyTest Fixture to store prereq data for cdns endpoint. + :returns cdn_data: Returns prerequisite data for cdns endpoint + """ + # Response keys for cdns endpoint + with open("prerequisite_data.json", encoding="utf-8", mode="r") as prereq_file: + data = json.load(prereq_file) + cdn_data = data["cdns"] + return cdn_data + + +def test_get_cdn(to_session: object, get_cdn_data: dict, cdn_prereq: list) -> None: + """Test step to validate keys, values and data types from cdns endpoint response. + :param to_session: Fixture to get Traffic ops session + :param get_cdn_data: Fixture to get cdn data from a prereq file + :param cdn_prereq: Fixture to get sample cdn data and actual cdn response + """ + # validate CDN keys from cdns get response + logger.info("Accessing Cdn endpoint through Traffic ops session.") + cdn_name = cdn_prereq[0]["name"] + cdn_get_response = to_session.get_cdns( + query_params={"name": cdn_name}) + try: + cdn_data = cdn_get_response[0] + cdn_keys = list(cdn_data[0].keys()) + logger.info( + "CDN Keys from cdns endpoint response %s", cdn_keys) + # validate cdn values from prereq data in cdns get response. + prereq_values = [cdn_prereq[0]["name"], cdn_prereq[0] + ["domainName"], cdn_prereq[0]["dnssecEnabled"]] + get_values = [cdn_data[0]["name"], cdn_data[0] + ["domainName"], cdn_data[0]["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.sort() == list(get_cdn_data.keys()).sort() + assert get_values == prereq_values + except IndexError: + logger.error("No CDN data from cdns get request") + pytest.fail("Response from get request is empty, Failing test_get_cdn") + finally: + # Delete CDN after test execution to avoid redundancy. + try: + cdn_response = cdn_prereq[1] + cdn_id = cdn_response["id"] + to_session.delete_cdn_by_id(cdn_id=cdn_id) + except IndexError: + logger.error("CDN wasn't created") Review Comment: should this mark the test as failed? ########## traffic_ops/testing/api_contract/v4/test_cdns.py: ########## @@ -0,0 +1,72 @@ +# +# 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 cdns endpoint.""" +import json +import logging +import pytest + +# Create and configure logger +logger = logging.getLogger() + + [email protected](name="get_cdn_data") +def get_cdn_prereq_data() -> dict: + """PyTest Fixture to store prereq data for cdns endpoint. + :returns cdn_data: Returns prerequisite data for cdns endpoint + """ + # Response keys for cdns endpoint + with open("prerequisite_data.json", encoding="utf-8", mode="r") as prereq_file: + data = json.load(prereq_file) + cdn_data = data["cdns"] + return cdn_data + + +def test_get_cdn(to_session: object, get_cdn_data: dict, cdn_prereq: list) -> None: Review Comment: the type of `to_session` is probably actually `TOSession`. At the very least it can't be `object`, because `object` doesn't have a `delete_cdn_by_id` attribute. assuming that `get_cdn_data` is the fixture by the same name above, the most specific you can be about its type is that it's an `object` - unless you want to do some checking in that fixture and return a more specific, more concrete type. assuming that `cdn_prereq` is the same as the fixture by the same name in the `conftest.py` file, the type is known to be more specific than `list` (see above note) ########## traffic_ops/testing/api_contract/v4/conftest.py: ########## @@ -0,0 +1,122 @@ +# +# 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. +# + +"""This module is used to create a Traffic Ops session +and to store prerequisite data for endpoints.""" +import json +import logging +import sys +from random import randint +from urllib.parse import urlparse +import pytest +from trafficops.tosession import TOSession +from trafficops.restapi import OperationError + + +# Create and configure logger +logger = logging.getLogger() + + +def pytest_addoption(parser: object) -> None: + """Passing in Traffic Ops arguments [Username, Password, Url and Hostname] from command line. + :param parser: Parser to parse command line arguments + """ + parser.addoption( + "--to-user", action="store", help="User name for Traffic Ops Session." + ) + parser.addoption( + "--to-password", action="store", help="Password for Traffic Ops Session." + ) + parser.addoption( + "--to-url", action="store", help="Traffic Ops URL." + ) + + [email protected](name="to_args") +def to_data(pytestconfig: pytest.Config) -> dict: + """PyTest fixture to store Traffic ops arguments passed from command line. + :param pytestconfig: Session-scoped fixture that returns the session's pytest.Config object + :returns args: Return Traffic Ops arguments + """ + args = {} + with open("to_data.json", encoding="utf-8", mode="r") as session_file: Review Comment: With this as written, I could provide `--to-user`, `--to-password`, and `--to-url` but if this configuration file doesn't exist the tests crash - even though the configuration file wouldn't be used at all. That doesn't seem right to me; I think the only thing that ought to really matter is that by the time parsing is all done for the configuration file, the command-line arguments, and any and all environment variables, that every necessary piece of the configuration is defined somehow. ########## traffic_ops/testing/api_contract/v4/conftest.py: ########## @@ -0,0 +1,122 @@ +# +# 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. +# + +"""This module is used to create a Traffic Ops session +and to store prerequisite data for endpoints.""" +import json +import logging +import sys +from random import randint +from urllib.parse import urlparse +import pytest +from trafficops.tosession import TOSession +from trafficops.restapi import OperationError + + +# Create and configure logger +logger = logging.getLogger() + + +def pytest_addoption(parser: object) -> None: + """Passing in Traffic Ops arguments [Username, Password, Url and Hostname] from command line. + :param parser: Parser to parse command line arguments + """ + parser.addoption( + "--to-user", action="store", help="User name for Traffic Ops Session." + ) + parser.addoption( + "--to-password", action="store", help="Password for Traffic Ops Session." + ) + parser.addoption( + "--to-url", action="store", help="Traffic Ops URL." + ) + + [email protected](name="to_args") +def to_data(pytestconfig: pytest.Config) -> dict: + """PyTest fixture to store Traffic ops arguments passed from command line. + :param pytestconfig: Session-scoped fixture that returns the session's pytest.Config object + :returns args: Return Traffic Ops arguments + """ + args = {} + with open("to_data.json", encoding="utf-8", mode="r") as session_file: + data = json.load(session_file) + session_data = data["test"] + args["api_version"] = urlparse( + session_data.get("url")).path.strip("/").split("/")[1] + args["port"] = session_data.get("port") + + to_user = pytestconfig.getoption("--to-user") + to_password = pytestconfig.getoption("--to-password") + to_url = pytestconfig.getoption("--to-url") + + if not all([to_user, to_password, to_url]): + logger.info( + "Traffic Ops session data were not passed from Command line Args.") + args["user"] = session_data.get("user") + args["password"] = session_data.get("password") + args["url"] = session_data.get("url") + else: + args["user"] = to_user + args["password"] = to_password + args["url"] = to_url + logger.info("Parsed Traffic ops session data from args %s", args) + return args + + [email protected](name="to_session") +def to_login(to_args: dict) -> TOSession: + """PyTest Fixture to create a Traffic Ops session from Traffic Ops Arguments + passed as command line arguments in to_args fixture in conftest. + :param to_args: Fixture to get Traffic ops session arguments + :returns to_session: Return Traffic ops session + """ + # Create a Traffic Ops V4 session and login + to_url = urlparse(to_args["url"]) + to_host = to_url.hostname + try: + to_session = TOSession(host_ip=to_host, host_port=to_args["port"], + api_version=to_args["api_version"], ssl=True, verify_cert=False) + logger.info("Established Traffic Ops Session.") + except OperationError: + sys.exit(-1) Review Comment: won't this suppress the error message and cause the user to have no idea why the tests crashed? It's not printing anything and it's catching the `OperationError` so that its stack trace isn't printed. -- 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]
