Conversation
em-baggie
requested review from
CarolineMorton and
oylenshpeegul
and removed request for
CarolineMorton
CarolineMorton
left a comment
CarolineMorton
left a comment
There was a problem hiding this comment.
Hey @em-baggie
Thanks for getting this done. This is really fantastic. Well done.
Sorry for the delay in review.
I am going to approve this but if you could add a couple of docs mentioned above this would be great. thanks.
| async fn test_download_usage_from_url() -> Result<(), CodeListBuilderError> { | ||
| let mock_server = MockServer::start().await; | ||
| let usage_year = UsageYear::Y2020_21; | ||
|
|
||
| let test_data = LONG_TEST_DATA; | ||
|
|
||
| Mock::given(method("GET")) | ||
| .and(path(usage_year.path())) | ||
| .respond_with(ResponseTemplate::new(200).set_body_string(test_data)) | ||
| .mount(&mock_server) | ||
| .await; | ||
|
|
There was a problem hiding this comment.
This is cool to see. I hadn't come across this crate before.
That being said I am not sure we really need to test the functionality of what is essentially a reqwest call. It might be better to concentrate on calls that fail and if the error message that gets passed back is understandable. has the url changed? is the file corrupted? is it too big to be downloaded? etc.
for now, lets keep this test in as it is useful documentation anyway.
There was a problem hiding this comment.
Makes sense. I'll add in a test that mocks the server to return an error code and make sure the custom error picks this up and displays this in a useful way.
There was a problem hiding this comment.
that sounds like a great use of this crate.
|
I've improved the docs and error handling, so will merge this now. |
3 participants
Closes #99
See #102