-
Notifications
You must be signed in to change notification settings - Fork 35
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
Fixture generator tool for tests #195
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.
I think there's some general ideas that should be investigated here, but as a starter (and since it's a test tool) I don't have any problems.
The one I would like investigated before merging is usage of serde_with
as we're adding another dep. It's not the end of the world, but if we could get away without it, that would be ideal. Thanks for the great work!
let comment = METADATA_COMMENT.to_string(); | ||
let metadata = Metadata { | ||
args: args.clone(), | ||
version: env!("CARGO_PKG_VERSION").to_string(), |
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.
So this is great for compat, but this also means we should start versioning this crate. And every commit on this crate should bump a version #. When we support reading back the metadata's we'll need versioned readers as well and should come up with a pattern of what we support.
Example:
- We won't support reading major version differences
- Minors are backwards, not fwd compat
- Build versions are backwards and forwards compatible (i.e. no schema change)
This should also go in a readme if we're going to try and do this (can be added when we actually read this back in however)
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'm additionally wondering if the comment-separated sections are going to be painful to parse downstream. Maybe it makes more sense to have a singular root struct which the N components exist in and write a single object? Add the header, no problem, but having a single struct should make deserialization a lot more straightforward I would think. In this case, we'll need to identify the section from the comments, and run a different parser each time right?
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.
Single object idea SGTM. I guess we'd need an order-preserving serialization/deserialization in that case to make sense out of the generated file.
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.
@slawlor @eozturk1 I've thought about using a single struct to simplify parsing, but the main drawback is that we would have to hold the entire struct in memory until all data is populated and we are ready to write. serde_yaml
(or serde_json
for that matter) does not support writing a partial struct.
If we are capturing n states, this would mean holding n directories' worth of db records in memory at one time. This would limit the directory size of fixtures we can generate with the tool.
With the current approach, all I need to do is break the YAML up by the start marker (---
) to get independent objects. For each chunk, I'll try to parse them as a certain struct, and move on to the next if it fails and only give up after attempting all structs. If this proves hard for some reason, I can experiment with using YAML tags to denote the struct types.
Does this make sense?
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.
Added small comments, and a possible idea we can look into about storing node deltas to advance states without performing the actual computation.
The tests can start with a small tree, test the changes on it. Advance to a bigger tree by just applying the node delta, test the changes on the new tree etc. Let me know what you think.
Great work @afterdusk, thank you for working on this!
Ah, this was exactly my intention with this tool! A user can specify Capturing the entire directory state is less space efficient than just the node delta between epochs 9 and 10, but I think this gives rise to a simpler and more flexible design:
Let me know if this makes sense! |
+1 on the list, great summary! I see the advantages of having |
Users of AKD might want to run integration tests against a fixed state of the directory. To this end, this PR adds a CLI tool that generates "fixture" files containing entire DB states, as well as directory updates between states. Users can then load this file to set up directories with fixed states, apply a given changeset, and assert that the output directory is as expected as part of their tests.
Follow-Up Tasks
Support using a database other than the in-memory one.Not necessary, see discussionI will create an issue tracking these tasks after this PR is approved.
Options
The available configuration parameters can be accessed with the
--help
flag:Example
The following is a simple run with just 2 users and 1 epoch, as the fixture file does get pretty large for larger directories. The upcoming compression of the directory state in #189 should help with this.
Output: