-
Notifications
You must be signed in to change notification settings - Fork 37
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
feat: CLI integration test improvements #754
Conversation
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.
Nice! Looks much better. I left some comments but overall looks good to me!
@@ -150,6 +150,7 @@ impl SyncSummary { | |||
self.consumed_notes.append(&mut other.consumed_notes); | |||
self.updated_accounts.append(&mut other.updated_accounts); | |||
self.locked_accounts.append(&mut other.locked_accounts); | |||
self.committed_transactions.append(&mut other.committed_transactions); |
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.
Hmm, do we want to also add this to is_empty
? Wonder if there is a better way to manage these
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.
Yeah, just realized about this error because I wanted to use that value. Seems we weren't checking on it on the other integration tests
(store_path, temp_dir) | ||
} | ||
|
||
fn init_cli_with_store_path(network: &str, store_path: &Path) -> PathBuf { |
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.
Can init_test_without_params()
also use these?
@@ -640,26 +380,135 @@ async fn test_init_with_testnet() { | |||
assert!(config_file_str.contains(&Endpoint::testnet().to_string())); | |||
} | |||
|
|||
#[tokio::test] | |||
async fn debug_mode_outputs_logs() { |
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.
Could we take the chance to document what this does briefly? Since it's fairly complex for something that may look simple
bin/miden-cli/src/tests.rs
Outdated
|
||
// Create a Client and a custom note | ||
let store_path = create_test_store_path(); | ||
let (mut client, authenticator) = create_test_client_with_store_path(&store_path).await; |
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 think we could rename this function to something like create_rust_client_with_store_path
in order to describe it better, but not too strong of an opinion so feel free to disregard
bin/miden-cli/src/tests.rs
Outdated
// HELPERS | ||
// ================================================================================================ | ||
|
||
fn init_cli(network: &str) -> (PathBuf, PathBuf) { | ||
let store_path = create_test_store_path(); | ||
let temp_dir = init_cli_with_store_path(network, &store_path); | ||
|
||
(store_path, temp_dir) | ||
} |
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 brief doc comments on helper functions would be great for future reference, even though most of these are simple, they are the library of functions that we should use to develop future tests.
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.
Thanks!
Based on some issues mentioned in #506. This PR simplifies the CLI tests by moving the command calling to functions that also read the output and return useful information so that it doesn't need to be retrieved again.