feat: CLI tests for iroh add, and API changes #343
Conversation
There was a problem hiding this comment.
Looks good to me. Nice tests for the actual command output.
You have to be careful not to get bogged down by these kinds of tests though. We don't want to spend days making sure that every byte coming out of stdout is the same when refactoring.
But the way it is done here seems to be quick to update.
| @@ -29,6 +29,16 @@ pub trait ApiExt: Api { | |||
| save_get_stream(&root_path, blocks).await?; | |||
| Ok(root_path) | |||
| } | |||
|
|
|||
| async fn add(&self, path: &Path, wrap: bool) -> Result<Cid> { | |||
There was a problem hiding this comment.
So the code to figure out if it is a file or a dir is moved to ApiExt, and the main fns in api require you to be explicit? I like it.
There was a problem hiding this comment.
Yes, and the check for the recursive flag before copying a directory was removed from the API entirely, because the use case for writing code like this is very different from the reason it's in the CLI (which is to prevent accidents in interactive mode).
| store: Option<S>, | ||
| path: &Path, | ||
| wrap: bool, | ||
| recursive: bool, |
There was a problem hiding this comment.
Why is the recursive flag not needed anymore? Do we always use recursive?
There was a problem hiding this comment.
The use case for the recursive flag in my mind is to prevent people from accidentally adding a directory in the CLI. That use case doesn't exist on the API level, so I've removed it from the API. The implementation is now on the CLI level which is much simpler than maintaining this in the UnixFS builder.
| let mut api = MockApi::default(); | ||
| api.expect_add_file().returning(|_ipfs_path, _| { | ||
| Box::pin(future::ready( | ||
| Cid::from_str("QmYbcW4tXLXHWw753boCK8Y7uxLu5abXjyYizhLznq9PUR").map_err(|e| e.into()), |
There was a problem hiding this comment.
So this returns the same cid whatever you add. Is the intention to make it more realistic, or just write another mock for that?
There was a problem hiding this comment.
Just write another mock if that becomes a use case. In general I don't think we should strive for realism in these mocks, just fulfilling the contract in the easiest way possible for a particular test case.
Unfortunately this means for add there isn't all that much to mock yet. Perhaps that will change somewhat once the add API starts to return more stuff related to progress, though I suspect tests for that will have to be normal Rust unit tests, not trycmd tests.
I agree. |
Needs more tests for various scenarios now
It's really only needed there, no need to pass a boolean in deeply. Even the API doesn't really need this - this is really a flag to help people avoid command-line mistakes.
This contains work on the add command, with a few CLI tests and some
API tweaks.
add layering
add_file and add_dir in the core Api that's mockable, and the slightly higher level
addis above it inApiExt.recursive simplication
Test for the
-rflag in the CLI and just letadd_dirin the resolver add a directory without worrying. This is tested.a few add CLI tests
A few basic add CLI tests.
CLI test cleanup
I've went through the
cli_tests.rsmodule step by step cleaning and reorganizing the tests so it's more easy to maintain them. Unfortunatelytrycmdgets too finicky if I try to use its globbing feature (as input and output directories seem shared sometimes). So now I maintain an explicit list in the same order as they show up alphabetically. It's too verbose but it's the best we can do for now.