Update requesting orders #519
Conversation
kevinlacaille
added
the
CLI/SDK Interface
…d additional logic for request to manage clip and tools.
|
Now to implement:
|
| mock_resp = httpx.Response(HTTPStatus.OK, json=order_description) | ||
| respx.post(TEST_ORDERS_URL).return_value = mock_resp |
There was a problem hiding this comment.
Not high priority, but it would be cool if we could find a way to hoist these lines up above the body of the function. For clarity. I don't know enough about respx to see how to do it, though.
| sent_request = json.loads(respx.calls.last.request.content) | ||
| assert sent_request == order_request | ||
|
|
||
|
|
||
| @pytest.mark.parametrize( |
There was a problem hiding this comment.
What is the benefit of testing the permutation of id strings in multiple tests?
There was a problem hiding this comment.
I took most tests for create and translated them for request, simply because a lot of create's functionality was transferred over to request. The permutation of ID strings was just something that we did for this test back when it was for create. Thoughts on removing it? cc @sarasafavi
| f'--id={id_string}', | ||
| '--bundle=analytic', | ||
| '--item-type=PSOrthoTile' | ||
| ]) |
There was a problem hiding this comment.
@kevinlacaille I think it would be better to invoke planet-data-request and planet-data-create in different tests, not in the same test. Then we'll have two simpler tests (no temp JSON file) that will be easier to read and modify down the road.
tests/integration/test_orders_cli.py
Outdated
| def test_cli_orders_request_tools(invoke, geom_geojson, | ||
| write_to_tmp_json_file): | ||
| tools_json = [{'clip': {'aoi': geom_geojson}}, {'composite': {}}] | ||
| tools_file = write_to_tmp_json_file(tools_json, 'tools.json') |
There was a problem hiding this comment.
@kevinlacaille what would you think about combining geom_geojson and write_to_tmp_json_file into a single pytest fixture? Something like this:
| def test_cli_orders_request_tools(invoke, geom_geojson, | |
| write_to_tmp_json_file): | |
| tools_json = [{'clip': {'aoi': geom_geojson}}, {'composite': {}}] | |
| tools_file = write_to_tmp_json_file(tools_json, 'tools.json') | |
| @pytest.fixture | |
| def tools_file(geom_geojson, write_to_tmp_json_file): | |
| tools_json = [{'clip': {'aoi': geom_geojson}}, {'composite': {}}] | |
| return write_to_tmp_json_file(tools_json, 'tools.json') | |
| def test_cli_orders_request_tools(invoke, tools_file): |
There was a problem hiding this comment.
Could be a useful pattern for the cloud config files too?
There was a problem hiding this comment.
@kevinlacaille this is excellent work. I have a few comments and suggestions. I hope you find them helpful!
|
Lots of good feedback here. I think this looks great @kevinlacaille !! |
|
Let's get this merged and we'll deal with more cosmetic / less consequential fixes in this issue: #584 |
Created orders request skeleton