-
Notifications
You must be signed in to change notification settings - Fork 2
test: add capability to specify service in client scenarios step #422
Conversation
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.
Hi @Trinaa! 👋🏻
I've added my comments inline. This is looking really good already! 🚀
89450e7
to
6e5ad37
Compare
* add teardown which clears records of contile partner requests between tests * add support for Records response content * update README documentation
529afcf
to
ee72ac5
Compare
@hackebrot ready for review again! |
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.
Hi @Trinaa! 👋🏻
A few minor corrections and then we can merge this.
url: 'https://www.example.org/desktop_windows' | ||
``` | ||
|
||
#### partner Service |
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.
#### partner Service | |
#### Partner Service |
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.
Change applied: 8f3bea8
|
||
* To direct requests to the MTS service, set the `service` value of `request` to | ||
`contile` | ||
* The expected content for a '200 OK' response is a collection of Tiles. |
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 expected content for a '200 OK' response is a collection of Tiles. | |
* The expected content for a `200 OK` response is a collection of tiles. |
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.
Change applied: 8f3bea8
|
||
* To direct requests to the partner service, set the `service` value of `request` to | ||
`partner` | ||
* The expected content for a '200 OK' response is a collection of Records. |
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 expected content for a '200 OK' response is a collection of Records. | |
* The expected content for a `200 OK` response is a collection of records. |
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.
Change applied: 8f3bea8
url = f"{contile_host}{step.request.path}" | ||
headers = {header.name: header.value for header in step.request.headers} | ||
method: str = step.request.method | ||
url: str = f"{hosts[step.request.service.value]}{step.request.path}" |
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.
If we use the enums as keys, this won't need the .value
attribute access.
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.
Change Applied: 32becfb
|
||
from models import Records, Scenario, Service, Tiles | ||
|
||
SERVICE_MODEL: Dict[Service, Union[Type[Records], Type[Tiles]]] = { |
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.
We can use a type alias here for improved readability.
SERVICE_MODEL: Dict[Service, Union[Type[Records], Type[Tiles]]] = { | |
SERVICE_MODEL = Union[Type[Records], Type[Tiles]] | |
SERVICE_MODELS: Dict[Service, SERVICE_MODEL] = { |
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.
Change applied: 433bcc4
# information and FastAPI model instances were created for them. | ||
for scenario in config.contile_scenarios: | ||
for i, step in enumerate(scenario.steps): | ||
|
||
if step.response.status_code != 200: | ||
continue | ||
|
||
if not isinstance(step.response.content, Tiles): | ||
expected_model: Union[Type[Records], Type[Tiles]] = SERVICE_MODEL.get( |
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.
expected_model: Union[Type[Records], Type[Tiles]] = SERVICE_MODEL.get( | |
expected_model: SERVICE_MODEL = SERVICE_MODELS.get( |
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.
Change applied: 433bcc4
partner_host = hosts["partner"] | ||
|
||
def clear_partner_records(): | ||
r: RequestsResponse = requests.delete(f"{partner_host}/records/") |
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.
Interesting. I'd expect flake8 to complain about the use of 1 character variable names. I think it used to do that. 😁
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.
Change Applied: 32becfb
I used the single character to stay consistent with the existing naming found in the test_contile
method, but would actually prefer the full name request
response
. This comment makes me think I can take the liberty of changing it throughout.
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! 🚀
Description
Add a new step type to the Contile contract tests, allowing for verification of the partner request history during scenario execution.
Issue(s)
CONSVC-1293