feat: forest-cli car concat #3150
Conversation
hanabi1224
requested review from
elmattic,
lemmih,
LesnyRumcajs,
jdjaustin,
creativcoder,
sudo-shashank,
aatifsyed and
ruseinov
as code owners
Co-authored-by: David Himmelstrup <david.himmelstrup@chainsafe.io>
src/cli/subcommands/car_cmd.rs
Outdated
| #[derive(Debug, Subcommand)] | ||
| pub enum CarCommands { | ||
| Concat { | ||
| first: PathBuf, |
There was a problem hiding this comment.
In general, what is preventing us from being more flexible here? That is to say, accept an arbitrary number of files, like the regular cat command. It should be straightforward to do with the existing code at no extra mental overhead cost.
There was a problem hiding this comment.
Yeah this is feasible, fixed
| use crate::common::cli; | ||
|
|
||
| #[tokio::test] | ||
| async fn forest_cli_car_concat() -> Result<()> { |
There was a problem hiding this comment.
I don't think these tests are useful. Correct me if I'm wrong, but they just check if the output file is a valid CAR file, right? I can imagine lots of incorrect implementations that would pass these tests.
Let's either remove the tests or make them more rigorous.
There was a problem hiding this comment.
Unlike unit tests, the main purpose of these bin tests ensure the executable runs without failures, and the assertions ensure the output file exists and can be parsed by CarReader. Any suggestions on adding more checks of the output car file?
There was a problem hiding this comment.
Would love unit tests that didn't use any files at all.
This PR is about a function that computes the union of a list of sets. We can test that thoroughly with quickcheck. Generate arbitrary sets of data and verify that the union contains all the elements of the input sets with no duplicates.
∀AB. A⊆(A∪B)
∀AB. B⊆(A∪B)
∀AB. A∪B = B∪A
∀ABC. (A ∪ B) ∪ C = A ∪ (B ∪ C)
∀A. A∪A = A
There was a problem hiding this comment.
asides:
- I think we should invest in some mock IPLD/ car creation utils rather than checking in test files
- I think we should have better guidelines on when and where to write integration tests. Part of the "engineering values" piece
Co-authored-by: David Himmelstrup <david.himmelstrup@chainsafe.io>
hanabi1224
force-pushed
the
hm/car-concat
branch
from
a685cb5
to
fd3fce3
Compare
Co-authored-by: David Himmelstrup <david.himmelstrup@chainsafe.io>
Summary of changes
As part of #3081
Changes introduced in this pull request:
forest-cli car concatsubcommandReference issue to close (if applicable)
Closes
Other information and links
Change checklist