Add Unit tests for VssAccessor#3
Conversation
|
Needs a rebase it seems: |
tnull
left a comment
There was a problem hiding this comment.
Some more comments, let me know if this is good for another round of review.
f4e55f8 to
5ee8edf
Compare
|
|
||
| [dependencies] | ||
| prost = "0.11.9" | ||
| prost = "0.11.6" |
There was a problem hiding this comment.
- to keep msrv low
- will eventually start enforcing it
tests/tests.rs
Outdated
| let base_url = mockito::server_url().to_string(); | ||
|
|
||
| // Set up the mock request/response. | ||
| let get_request = build_get_request(); |
There was a problem hiding this comment.
It would be better to inline the request here so the test is self-contained. Otherwise, you have to jump to the end of the file to see how this relates to the response.
There was a problem hiding this comment.
I agree,
in fact it was like that earlier, only reason i extracted it to function was because it was repeated ~4 times.
Personally i prefer test to be self-contained as well than to over-emphasize on not repeating.
Inlining such functions.
tests/tests.rs
Outdated
| let mut mock_response = GetObjectResponse::default(); | ||
| mock_response.value = Some(KeyValue { key: "k1".to_string(), version: 2, value: b"k1v2".to_vec() }); |
There was a problem hiding this comment.
Little more idiomatic to initialize using one statement.
let mock_response = GetObjectResponse {
value: Some(KeyValue { key: "k1".to_string(), version: 2, value: b"k1v2".to_vec() }),
..Default::default()
};| // Requests to endpoints are no longer mocked and will result in network error. | ||
| drop(_mock_server); | ||
|
|
||
| let get_network_err = vss_client.get_object(&get_request).await; | ||
| assert!(matches!(get_network_err.unwrap_err(), VssError::InternalError { .. })); | ||
|
|
||
| let put_network_err = vss_client.put_object(&put_request).await; | ||
| assert!(matches!(put_network_err.unwrap_err(), VssError::InternalError { .. })); | ||
|
|
||
| let list_network_err = vss_client.list_key_versions(&list_request).await; | ||
| assert!(matches!(list_network_err.unwrap_err(), VssError::InternalError { .. })); |
There was a problem hiding this comment.
Wasn't clear to me why this results in an InternalError. In a separate commit, could you add documentation to the enum variants and configure the library as following?
#![deny(missing_docs)]
#![deny(unsafe_code)]There was a problem hiding this comment.
Sounds Good, it is already on my todo list 👍
tnull
left a comment
There was a problem hiding this comment.
LGTM, mod the outstanding comments.
Depends on #1 and #2