ARK-Builders / arklib

Core of the programs in ARK family
MIT License
1 stars 10 forks source link

#41: Make unit tests of `update_one` and `update_all` method #71

Closed funnyVOLT closed 8 months ago

funnyVOLT commented 8 months ago
kirillt commented 8 months ago

For instance, when the first parameter of the update_one method is the path of a newly added file, it returns an error. Furthermore, the same holds true when the first parameter of the update_one method corresponds to a file that has not been updated.

Interesting, thank you for this observation. Initially, the idea was that update_one must be called only on up-to-date index (when we know that exactly 1 path was updated). But it might be a good idea to allow updates on outdated index to make it more robust.

funnyVOLT commented 8 months ago

For instance, when the first parameter of the update_one method is the path of a newly added file, it returns an error. Furthermore, the same holds true when the first parameter of the update_one method corresponds to a file that has not been updated.

Interesting, thank you for this observation. Initially, the idea was that update_one must be called only on up-to-date index (when we know that exactly 1 path was updated). But it might be a good idea to allow updates on outdated index to make it more robust.

May I update 'update_one' method?

kirillt commented 8 months ago

@funnyVOLT yes, sure. Although you've already changed it to work on non-up-to-date index by ignoring the lookup error, haven't you? Any way, new ideas are welcome.

kirillt commented 8 months ago

@funnyVOLT good stuff, but could you also compare results in the test? Sequence of update_one calls and update_all should provide exactly the same result. Would be great to have some assertions about it.

kirillt commented 8 months ago

Something like this:

let mut index1 = index.clone();
index1.update_all(...);

let mut index2 = index;
for (path, id) in single_updates {
    index2.update_one(path, id);
}

assert_eq!(index1, index2);

Cherry on top would be randomly generating single_updates and doing this assertion in a loop.

funnyVOLT commented 8 months ago

Something like this:

let mut index1 = index.clone();
index1.update_all(...);

let mut index2 = index;
for (path, id) in single_updates {
    index2.update_one(path, id);
}

assert_eq!(index1, index2);

Cherry on top would be randomly generating single_updates and doing this assertion in a loop.

@kirillt, I already implemented similar test case in this function. https://github.com/funnyVOLT/arklib/blob/30cb80abb296d1a0b71345224545601af975d38a/src/index.rs#L1090

kirillt commented 8 months ago

@funnyVOLT yes, I've seen this. But this test case doesn't compare output of update_all and update_one. It only asserts on output of update_one.

funnyVOLT commented 8 months ago

@funnyVOLT yes, I've seen this. But this test case doesn't compare output of update_all and update_one. It only asserts on output of update_one.

@kirill Added test code comparing update_one and update_all. https://github.com/funnyVOLT/arklib/blob/a05e2d0300aef3824b4b347b443d2f60e466165c/src/index.rs#L1160

kirillt commented 8 months ago

Superseded by #72