-
Notifications
You must be signed in to change notification settings - Fork 7
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add Unit tests for VssAccessor #3
Conversation
Needs a rebase it seems:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some more comments, let me know if this is good for another round of review.
f4e55f8
to
5ee8edf
Compare
@@ -5,14 +5,16 @@ edition = "2021" | |||
build = "build.rs" | |||
|
|||
[dependencies] | |||
prost = "0.11.9" | |||
prost = "0.11.6" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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()
};
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense!
// 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds Good, it is already on my todo list 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, mod the outstanding comments.
Depends on #1 and #2