feat: pact contract for get job by id and post job - #106
duvanmoionq wants to merge 3 commits into
Conversation
|
Thank you @duvanmoionq! before a full review, a question: should the new tests be added to the CI too? or does |
Yes! we would do that later for now we are only adding test contracts examples that we can follow to add more contracts. we will add later the CI logic whenever we configure the Broker we need. |
antalszava
left a comment
antalszava
left a comment
There was a problem hiding this comment.
Thank you so much @duvanmoionq! 👏 Very interesting direction.
I've left a couple of comments, my main feedback is: could we somehow dynamically depend on the API spec that we have in the repo in the pact?
Also, could you further describe what we gain from using pact instead of "simply" writing pytest tests with expected responses?
| @@ -0,0 +1,165 @@ | |||
| """Pact HTTP contract: ionq-core-python (consumer) -> cloud-job-manager (provider). | |||
There was a problem hiding this comment.
Is it a convention to have a consumers folder inside the pact folder?
First it seems a bit unnecessary to have two levels of folder nesting atm.
| """Pact HTTP contract: ionq-core-python (consumer) -> cloud-job-manager (provider). | ||
|
|
||
| Pins the two jobs-API interactions the SDK depends on, in ONE pact | ||
| (ionq-core-python-cloud-job-manager.json): |
There was a problem hiding this comment.
The mention of ionq-core-python-cloud-job-manager.json seems a bit confusing here since it's not a file in the repo - where does it live?
| Pins the two jobs-API interactions the SDK depends on, in ONE pact | ||
| (ionq-core-python-cloud-job-manager.json): | ||
|
|
||
| 1. POST /v0.4/jobs create a circuit job -> 201 {id, status, session_id} |
There was a problem hiding this comment.
This endpoint can have different inputs: single-circuit,multi-circuit, quantum function and qaoa function. Should that matter here/does that matter for the contract?
| 1. POST /v0.4/jobs create a circuit job -> 201 {id, status, session_id} | ||
| 2. GET /v0.4/jobs/{id} fetch a completed job -> 200 (every SingleCircuitJob |
There was a problem hiding this comment.
Imho these should be coming dynamically from this repo's API spec version. Accordingly, I think it would be good to remove/keep the endpoint and payload descriptions minimal here because they need to be changed here each time their spec changes.
| artifact DESCRIPTORS) | ||
|
|
||
| The SDK carries /v0.4 in its base_url (client config, not contract): the pact | ||
| records the WIRE paths the provider serves, so the mock base_url below is the |
| Consumer-driven notes (generated openapi-python-client code: parse IS | ||
| consumption — from_dict pops every required key): | ||
| - Create response: session_id is REQUIRED-nullable — new vs the retired | ||
| python-ionq contract, which read only id + status. | ||
| - Get response: SingleCircuitJob.from_dict hard-requires ~23 keys; nullable | ||
| ones must be PRESENT (null is fine). results entries are v1 artifact | ||
| descriptors {id, format, media_type} KEYED BY FORMAT — the provider also | ||
| sends legacy {url} pointer entries alongside (extra keys, allowed by Pact, | ||
| ignored by this assertion). | ||
| - Client: bare AuthenticatedClient, never the IonQClient factory — the factory | ||
| pins a platform-varying User-Agent and warns on non-HTTPS base urls, which | ||
| this repo's filterwarnings=error would turn into a failure. |
There was a problem hiding this comment.
I find this paragraph challenging to understand completely, could it be rephrased a bit?
|
|
||
| # The real request model drives the pact request body (never hand-written, so | ||
| # it can't drift from what the SDK serializes). Enriched with the optional | ||
| # fields the SDK really sends: name, settings.error_mitigation and noise. |
There was a problem hiding this comment.
I think it's good to remove
name, settings.error_mitigation and noise
since these can change and then the comment becomes easily outdated.
| # Every SingleCircuitJob required key, with the values the provider state's | ||
| # seed produces; required-nullable keys are asserted as null. results pins the | ||
| # format-keyed v1 descriptor the provider synthesizes (its legacy {url} | ||
| # entries ride along as unasserted extras). |
There was a problem hiding this comment.
I find these comments hard to read and understand, could they be rephrased?
| PACT_DIR.mkdir(exist_ok=True) | ||
| pact.write_file(PACT_DIR, overwrite=True) |
There was a problem hiding this comment.
What do these instructions do?
2 participants
QTPF-9072
install pact dependency
Adding pact contract for http POST /job
Adding pact contract for http GEt /job/{id}