-
Notifications
You must be signed in to change notification settings - Fork 204
RUST/TEST: test sync_manager #1006
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
base: main
Are you sure you want to change the base?
RUST/TEST: test sync_manager #1006
Conversation
|
👋 Hi evgeny-leksikov! Thank you for contributing to ai-dynamo/nixl. Your PR reviewers will review your contribution then trigger the CI to test your changes. 🚀 |
|
/build |
efa1b17
910f40c to
efa1b17
Compare
|
temporary rebased on top of #829 |
Signed-off-by: Evgeny Leksikov <[email protected]>
cb1deb2 to
5d9544c
Compare
|
/build |
| let data_calls = Rc::new(Cell::new(0)); | ||
| let backend_calls = Rc::new(Cell::new(0)); | ||
|
|
||
| let data = DummyData { | ||
| value: Cell::new(0), | ||
| data_calls: data_calls.clone(), | ||
| should_fail: Cell::new(false), | ||
| }; | ||
| let backend = DummyBackend { | ||
| sync_calls: backend_calls.clone(), | ||
| }; | ||
| let mgr = SyncManager::new(data, backend); |
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.
Let's try to reduce code duplication in the initialization of those tests, maybe we can use some kind of default constructors that will populate those fields:
let data = DummyData::default();
let backend = DummyBackend::default();
// and so on
What?
Add new tests for SyncManager implementation.
Why?
#828 (comment)
NOTE: marked as a draft since the PR #1003 improves sync_manager, so this PR will require to resolve conflicts.