Skip to content
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

Merged
merged 1 commit into from
Aug 8, 2023

Conversation

G8XSU
Copy link
Collaborator

@G8XSU G8XSU commented Apr 27, 2023

Depends on #1 and #2

@G8XSU G8XSU marked this pull request as ready for review April 27, 2023 17:14
vss-accessor/src/vss_error.rs Outdated Show resolved Hide resolved
vss-accessor/tests/tests.rs Outdated Show resolved Hide resolved
vss-accessor/tests/tests.rs Outdated Show resolved Hide resolved
vss-accessor/tests/tests.rs Outdated Show resolved Hide resolved
@G8XSU G8XSU force-pushed the accessor_tests branch from 506e45f to 0d4dd8d Compare May 4, 2023 03:04
@tnull
Copy link
Contributor

tnull commented May 4, 2023

Needs a rebase it seems:

> cargo test
   Compiling vss-accessor v0.1.0 (/Users/ero/workspace/vss-rust-client/vss-accessor)
error[E0308]: arguments to this method are incorrect
  --> vss-accessor/tests/tests.rs:32:31
   |
32 |         let actual_result = vss_acc.get("store", "k1").await.unwrap();
   |                                     ^^^ -------  ---- expected struct `String`, found `&str`
   |                                         |
   |                                         expected struct `String`, found `&str`
   |
note: associated function defined here
  --> /Users/ero/workspace/vss-rust-client/vss-accessor/src/lib.rs:29:15
   |
29 |     pub async fn get(&self, store: String, key: String) -> Result<GetObjectResponse, VssError> {
   |                  ^^^
help: try using a conversion method
   |
32 |         let actual_result = vss_acc.get("store".to_string(), "k1").await.unwrap();
   |                                                ++++++++++++
help: try using a conversion method
   |
32 |         let actual_result = vss_acc.get("store", "k1".to_string()).await.unwrap();
   |                                                      ++++++++++++

error[E0308]: arguments to this method are incorrect
  --> vss-accessor/tests/tests.rs:64:31
   |
64 |         let actual_result = vss_acc.put("store", Some(4), "k1", 2, b"k1v3").await.unwrap();
   |                                     ^^^ -------           ---- expected struct `String`, found `&str`
   |                                         |
   |                                         expected struct `String`, found `&str`
   |
note: associated function defined here
  --> /Users/ero/workspace/vss-rust-client/vss-accessor/src/lib.rs:46:15
   |
46 |     pub async fn put(
   |                  ^^^
help: try using a conversion method
   |
64 |         let actual_result = vss_acc.put("store".to_string(), Some(4), "k1", 2, b"k1v3").await.unwrap();
   |                                                ++++++++++++
help: try using a conversion method
   |
64 |         let actual_result = vss_acc.put("store", Some(4), "k1".to_string(), 2, b"k1v3").await.unwrap();
   |                                                               ++++++++++++

...

Copy link
Contributor

@tnull tnull left a 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.

vss-accessor/tests/generate_protos.rs Outdated Show resolved Hide resolved
vss-accessor/tests/generate_protos.rs Outdated Show resolved Hide resolved
vss-accessor/tests/tests.rs Outdated Show resolved Hide resolved
@@ -5,14 +5,16 @@ edition = "2021"
build = "build.rs"

[dependencies]
prost = "0.11.9"
prost = "0.11.6"
Copy link
Collaborator Author

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

@G8XSU G8XSU requested a review from jkczyz July 20, 2023 19:52
tests/tests.rs Outdated
let base_url = mockito::server_url().to_string();

// Set up the mock request/response.
let get_request = build_get_request();
Copy link

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.

Copy link
Collaborator Author

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
Comment on lines 25 to 26
let mut mock_response = GetObjectResponse::default();
mock_response.value = Some(KeyValue { key: "k1".to_string(), version: 2, value: b"k1v2".to_vec() });
Copy link

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()
};

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense!

tests/tests.rs Outdated Show resolved Hide resolved
tests/tests.rs Outdated Show resolved Hide resolved
tests/tests.rs Outdated Show resolved Hide resolved
tests/tests.rs Outdated Show resolved Hide resolved
@G8XSU G8XSU requested a review from jkczyz August 7, 2023 03:06
tests/tests.rs Outdated Show resolved Hide resolved
tests/tests.rs Outdated Show resolved Hide resolved
Comment on lines +278 to +289
// 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 { .. }));
Copy link

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)]

Copy link
Collaborator Author

@G8XSU G8XSU Aug 8, 2023

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 👍

Copy link
Contributor

@tnull tnull left a 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.

Cargo.toml Outdated Show resolved Hide resolved
tests/tests.rs Outdated Show resolved Hide resolved
@G8XSU G8XSU requested a review from jkczyz August 8, 2023 19:50
@G8XSU G8XSU merged commit 3770cef into lightningdevkit:main Aug 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants