add test case for runId addition #115
Conversation
Signed-off-by: SreeV <441385+sreev@users.noreply.github.com>
Codecov Report
@@ Coverage Diff @@
## main #115 +/- ##
==========================================
+ Coverage 37.07% 38.16% +1.09%
==========================================
Files 21 21
Lines 847 862 +15
==========================================
+ Hits 314 329 +15
Misses 533 533
Continue to review full report at Codecov.
|
| assert str(response['id']) is not None | ||
| assert str(response['location']) == location | ||
| assert str(response['runId']) == run_id | ||
|
|
There was a problem hiding this comment.
Mind if we also add an assert that the mock_put was called only once?
There was a problem hiding this comment.
it's redundant. but if you insist. why not, sure.
julienledem
left a comment
julienledem
left a comment
There was a problem hiding this comment.
Looking good!
Please see my comment inline
tests/test_marquez_client.py
Outdated
|
|
||
| assert str(response['id']) is not None | ||
| assert str(response['location']) == location | ||
| assert str(response['runId']) == run_id |
There was a problem hiding this comment.
you need to check that mock_put was called with the parameters you expect.
Here is an example here:
https://github.com/MarquezProject/marquez-airflow/blob/a5a72879cfa7b4a5db1bcd6b3a11fc942943405e/tests/test_marquez_dag.py#L97
you can also assert that response is mock_put.return_value since it should returning that.
There was a problem hiding this comment.
it makes sense in the dag as there are many calls happening. this method is directly calling only one.
return value already there few lines above:
marquez-python/tests/test_marquez_client.py
Line 283 in abae9bc
There was a problem hiding this comment.
response = return value from create_job (inside return value of mock.put).
python magic.
There was a problem hiding this comment.
As mentioned in my comment above, you need to check that mock_put was called with the parameters you expect.
You want to verify that the run_id parameter passed to create_job is actually sent to the backend.
I would assert mock_put.call_args.kwargs["runId"] == run_id (or something similar)
Doc here: https://docs.python.org/3/library/unittest.mock.html#unittest.mock.Mock.call_args
Signed-off-by: SreeV <441385+sreev@users.noreply.github.com>
tests/test_marquez_client.py
Outdated
|
|
||
| assert str(response['id']) is not None | ||
| assert str(response['location']) == location | ||
| assert str(response['runId']) == run_id |
There was a problem hiding this comment.
As mentioned in my comment above, you need to check that mock_put was called with the parameters you expect.
You want to verify that the run_id parameter passed to create_job is actually sent to the backend.
I would assert mock_put.call_args.kwargs["runId"] == run_id (or something similar)
Doc here: https://docs.python.org/3/library/unittest.mock.html#unittest.mock.Mock.call_args
Signed-off-by: SreeV <441385+sreev@users.noreply.github.com>
| "runId": run_id | ||
| } | ||
|
|
||
| response = self.client.create_job( |
There was a problem hiding this comment.
A small clarification here: the job create response won't contain the run ID. The optional run ID will only be used to link a newly created job version to an existing job run, but not returned to the caller. You'd want to update your test to assert runId is part if the request.
Signed-off-by: SreeV <441385+sreev@users.noreply.github.com>
tests/test_marquez_client.py
Outdated
| == run_id | ||
|
|
||
| assert str(response['inputs']) is not None | ||
| assert response['latestRun'] is None |
There was a problem hiding this comment.
The latestRun wouldn't be None is this case. When a run ID is provided, the Marquez API assumes the job has a run in a RUNNING state. We'd want to make sure the contract is correct and mock out a latest run object in the response using the run ID passed to Marquez.
The main goal for adding run ID support is to allow the caller to modify a running job while linking the new job version to the existing run. As an example, you can reference the java client test we have testCreateJobWithRunId()
There was a problem hiding this comment.
i agree with the main goal.
Signed-off-by: SreeV <441385+sreev@users.noreply.github.com>
wslulciuc
left a comment
wslulciuc
left a comment
There was a problem hiding this comment.
I think there might be a few comments @julienledem made that would need to be addressed? but otherwise LGTM 👍
Addressed. |
Signed-off-by: SreeV 441385+sreev@users.noreply.github.com