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

funnyVOLT · 2024-01-15T21:34:34Z

Covered update_one unit test
Covered update_all unit test
In the third key point, you mentioned the potential substitution of update_all with multiple instances of the update_one method.
update_one method have some problems.
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.
To leverage the update_one method in a manner similar to the update_all method, it is imperative to refine and update the update_one method accordingly.
Ensure that update_all and update_one are consistent #41

src/index.rs

kirillt · 2024-01-16T05:04:47Z

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.

kirillt · 2024-01-16T05:05:39Z

src/index.rs

+                                // updated
+                                return Err(ArklibError::Collision(
+                                    "New content has the same id".into(),
+                                ));
                            }


Might be worth to write something into the log in else branch.

funnyVOLT · 2024-01-16T10:36:07Z

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 · 2024-01-17T12:21:13Z

@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 · 2024-01-18T02:28:20Z

@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 · 2024-01-18T02:31:42Z

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 · 2024-01-18T04:06:29Z

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 · 2024-01-18T12:00:16Z

@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 · 2024-01-18T15:51:18Z

@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 · 2024-01-20T07:23:13Z

src/index.rs

+        let deleted: HashSet<ResourceId> = HashSet::new();
+        let added: HashMap<CanonicalPathBuf, ResourceId> = HashMap::new();
+        let mut update = IndexUpdate { deleted, added };


These structures seem to be unused.

kirillt · 2024-01-20T07:23:34Z

src/index.rs

+                                // update.added.insert(
+                                //     canonical_path_buf.clone(),
+                                //     new_entry.id,
+                                // );
+                                // self.insert_entry(
+                                //     canonical_path_buf.clone(),
+                                //     new_entry,
+                                // );
+                                // update.deleted.insert(old_id);


Commented-out code should be removed before merging.

Commented-out code should be removed before merging.

@Kirill I would like to ask you:
Why don't you do anything when the file has changed?

Commented-out code should be removed before merging.

#kirill I would like to ask you: Why do you want to return "Err" when a file has been modified?
Shouldn't some information be returned when an existing file is modified?

kirillt · 2024-01-20T08:01:15Z

src/index.rs

+                                //     new_entry,
+                                // );
+                                // update.deleted.insert(old_id);
+                                return Ok(update);


It's better to keep returning Err as it was before. Not because it leads to incorrect state (it doesn't), but because the caller must ensure that the path being updated, was modified indeed. Otherwise, updating doesn't make sense and only spends computation resources.

kirillt · 2024-01-20T08:34:22Z

src/index.rs

@@ -984,4 +1003,198 @@ mod tests {
        assert!(new2 > old2);
        assert!(new2 > new1);
    }
+
+    #[test]
+    fn should_correctly_update_one_resource() {


This test case is not correct. See test case fn should_not_update_nonexistent_path.

kirillt · 2024-01-20T08:38:31Z

src/index.rs

+    }
+
+    #[test]
+    fn should_correctly_update_all_resource() {


This test case is a duplicate. See fn should_update_resource_index_adding_1_additional_file_successfully.

kirillt · 2024-01-20T08:45:10Z

src/index.rs

+
+            create_file_at(path.clone(), Some(FILE_SIZE_1), Some(FILE_NAME_1));
+
+            for (old_path, old_index_enry) in &actual.path2id {


This is just 1 iteration, at this moment.

kirillt · 2024-01-20T08:47:25Z

src/index.rs

+            for (cur_path, cur_entry) in curr_entries {
+                let one_index_update = actual
+                    .update_one(&cur_path.clone(), cur_entry.id)
+                    .expect("Should update one resource correctly");
+
+                for (add_path, &add_id) in one_index_update.added.iter() {
+                    all_added.insert(add_path.clone(), add_id.clone());
+                }
+
+                for delete_id in one_index_update.deleted {
+                    all_deleted.insert(delete_id);
+                }
+            }


This test case contains no assertion, what is your expectation of the final state?

Also, please don't commit debugging prints (println!).

kirillt · 2024-01-20T08:56:54Z

src/index.rs

+            let updated_using_update_one = IndexUpdate {
+                deleted: all_deleted,
+                added: all_added,
+            };


It's a good idea to compare IndexUpdate structures, however we should focus first on Index instances themselves.

Something like this is easier to start with:

#[test] fn sequence_of_update_one_calls_should_imitate_update_all() { run_test_and_clean_up(|path| { create_file_at(path.clone(), Some(FILE_SIZE_2), Some(FILE_NAME_2)); let index_initial = ResourceIndex::build(path.clone()); create_file_at(path.clone(), Some(FILE_SIZE_1), Some(FILE_NAME_1)); let mut index_full = index_initial.clone(); index_full.update_all() .expect("Should update the index correctly"); let curr_paths: HashMap<CanonicalPathBuf, DirEntry> = discover_paths(path.clone()); let curr_entries = scan_entries(curr_paths); let mut index_by_one = index_initial.clone(); // only 2 iterations at this moment for (cur_path, cur_entry) in curr_entries { index_by_one .update_one(&cur_path.clone(), cur_entry.id) .expect("Should update one resource correctly"); } assert_eq!(index_full, index_by_one); }) }

kirillt · 2024-01-20T11:17:21Z

Superseded by #72

make unit tests of update_one and update_all methord

6104142

kirillt reviewed Jan 16, 2024

View reviewed changes

src/index.rs Show resolved Hide resolved

kirillt reviewed Jan 16, 2024

View reviewed changes

kirillt assigned funnyVOLT Jan 16, 2024

function of 'update_all' implemented using 'update_one'.

30cb80a

make test update_all compare update_one

a05e2d0

kirillt reviewed Jan 20, 2024

View reviewed changes

kirillt closed this Jan 20, 2024

kirillt mentioned this pull request Jan 20, 2024

#41: Add unit tests, fix update_one #72

Closed

kirillt changed the title ~~make unit tests of update_one and update_all method~~ #41: Make unit tests of update_one and update_all method Feb 6, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

funnyVOLT commented Jan 15, 2024 •

edited by kirillt

Loading

kirillt commented Jan 16, 2024

kirillt Jan 16, 2024

funnyVOLT commented Jan 16, 2024

kirillt commented Jan 17, 2024

kirillt commented Jan 18, 2024

kirillt commented Jan 18, 2024 •

edited

Loading

funnyVOLT commented Jan 18, 2024

kirillt commented Jan 18, 2024

funnyVOLT commented Jan 18, 2024 •

edited

Loading

kirillt Jan 20, 2024

kirillt Jan 20, 2024

funnyVOLT Jan 20, 2024

kirillt Jan 20, 2024

kirillt Jan 20, 2024

kirillt Jan 20, 2024

kirillt Jan 20, 2024

kirillt Jan 20, 2024

kirillt Jan 20, 2024

kirillt Jan 20, 2024

kirillt commented Jan 20, 2024


		create_file_at(path.clone(), Some(FILE_SIZE_1), Some(FILE_NAME_1));

		for (old_path, old_index_enry) in &actual.path2id {

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

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

Conversation

funnyVOLT commented Jan 15, 2024 • edited by kirillt Loading

kirillt commented Jan 16, 2024

Choose a reason for hiding this comment

funnyVOLT commented Jan 16, 2024

kirillt commented Jan 17, 2024

kirillt commented Jan 18, 2024

kirillt commented Jan 18, 2024 • edited Loading

funnyVOLT commented Jan 18, 2024

kirillt commented Jan 18, 2024

funnyVOLT commented Jan 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kirillt commented Jan 20, 2024

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

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

funnyVOLT commented Jan 15, 2024 •

edited by kirillt

Loading

kirillt commented Jan 18, 2024 •

edited

Loading

funnyVOLT commented Jan 18, 2024 •

edited

Loading