Skip to content
This repository has been archived by the owner on Sep 16, 2024. It is now read-only.

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

Closed
wants to merge 3 commits into from
Closed

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

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6104142
make unit tests of update_one and update_all methord
funnyVOLT Jan 15, 2024
30cb80a
function of 'update_all' implemented using 'update_one'.
funnyVOLT Jan 17, 2024
a05e2d0
make test update_all compare update_one
funnyVOLT Jan 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
249 changes: 231 additions & 18 deletions src/index.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ pub struct ResourceIndex {
root: PathBuf,
}

#[derive(Debug)]
#[derive(PartialEq, Debug)]
pub struct IndexUpdate {
pub deleted: HashSet<ResourceId>,
pub added: HashMap<CanonicalPathBuf, ResourceId>,
Expand Down Expand Up @@ -352,6 +352,10 @@ impl ResourceIndex {
return self.forget_id(old_id);
}

let deleted: HashSet<ResourceId> = HashSet::new();
let added: HashMap<CanonicalPathBuf, ResourceId> = HashMap::new();
let mut update = IndexUpdate { deleted, added };
Comment on lines +355 to +357
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These structures seem to be unused.


let canonical_path_buf = CanonicalPathBuf::canonicalize(path)?;
let canonical_path = canonical_path_buf.as_canonical_path();

Expand All @@ -365,33 +369,42 @@ impl ResourceIndex {
Err(_) => {
// updating the index after resource removal is a correct
// scenario
self.forget_path(canonical_path, old_id)
return self.forget_path(canonical_path, old_id);
}
Ok(metadata) => {
match scan_entry(canonical_path, metadata) {
Err(_) => {
// a directory or empty file exists by the path
self.forget_path(canonical_path, old_id)
return self.forget_path(canonical_path, old_id);
}
Ok(new_entry) => {
// valid resource exists by the path
let curr_entry = &self.path2id[canonical_path];

if curr_entry.id == new_entry.id {
// in rare cases we are here due to hash collision
// in rare cases we are here due to hash collision
funnyVOLT marked this conversation as resolved.
Show resolved Hide resolved
if let Some(curr_entry) =
self.path2id.get(canonical_path)
{
if curr_entry.id == new_entry.id {
if curr_entry.modified == new_entry.modified {
log::warn!("path {:?} does not modified but not its content", &canonical_path);
return Ok(update);
}

if curr_entry.modified == new_entry.modified {
log::warn!("path {:?} was modified but not its content", &canonical_path);
//in cases Resourceid' didn't modify and 'SystemTime' was modified
// updated
log::warn!("Resourceid didn't modify but SystemTime was modified");
// update.added.insert(
// canonical_path_buf.clone(),
// new_entry.id,
// );
// self.insert_entry(
// canonical_path_buf.clone(),
// new_entry,
// );
// update.deleted.insert(old_id);
Comment on lines +394 to +402
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Commented-out code should be removed before merging.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

return Ok(update);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


// the caller must have ensured that the path was
// updated
return Err(ArklibError::Collision(
"New content has the same id".into(),
));
}

// new resource exists by the path
//new resource exists by the path
self.forget_path(canonical_path, old_id).map(
|mut update| {
update.added.insert(
Expand Down Expand Up @@ -605,7 +618,7 @@ fn is_hidden(entry: &DirEntry) -> bool {
#[cfg(test)]
mod tests {
use crate::id::ResourceId;
use crate::index::{discover_paths, IndexEntry};
use crate::index::{discover_paths, scan_entries, IndexEntry, IndexUpdate};
use crate::initialize;
use crate::ResourceIndex;
use canonical_path::CanonicalPathBuf;
Expand All @@ -619,6 +632,10 @@ mod tests {
use std::time::SystemTime;
use uuid::Uuid;

use std::collections::{HashMap, HashSet};
type Paths = HashSet<CanonicalPathBuf>;
use walkdir::DirEntry;

const FILE_SIZE_1: u64 = 10;
const FILE_SIZE_2: u64 = 11;

Expand All @@ -629,6 +646,8 @@ mod tests {
const CRC32_1: u32 = 3817498742;
const CRC32_2: u32 = 1804055020;

use crate::{ArklibError, Result};

fn get_temp_dir() -> PathBuf {
create_dir_at(std::env::temp_dir())
}
Expand Down Expand Up @@ -984,4 +1003,198 @@ mod tests {
assert!(new2 > old2);
assert!(new2 > new1);
}

#[test]
fn should_correctly_update_one_resource() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

run_test_and_clean_up(|path| {
create_file_at(path.clone(), Some(FILE_SIZE_2), Some(FILE_NAME_2));
let mut actual = ResourceIndex::build(path.clone());

println!("==🌹🌹=update_one unit tests start==");

for (key, value) in &actual.path2id {
println!(
"===❤❤========update_one_new_entry Resource ID: {}",
&actual.path2id[key].id
);
}

create_file_at(path.clone(), Some(FILE_SIZE_1), Some(FILE_NAME_1));
let mut file_path = path.clone();
file_path.push(FILE_NAME_1);
let old_id = ResourceId {
data_size: FILE_SIZE_1,
crc32: CRC32_1,
};
let path_buf: CanonicalPathBuf =
CanonicalPathBuf::canonicalize(&file_path).unwrap();
let update = actual
.update_one(&path_buf.clone(), old_id)
.expect("Should update one resource correctly");

for (key, value) in &actual.path2id {
println!(
"===😂😂========update_after_entry Resource ID: {}",
&actual.path2id[key].id
);
}

println!(
"===✔✔==added=:{} deleted={}",
update.added.len().to_string(),
update.deleted.len().to_string()
);

assert_eq!(update.added.len(), 1);
assert_eq!(actual.path2id.len(), 2);
})
}

#[test]
fn should_correctly_update_all_resource() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test case is a duplicate. See fn should_update_resource_index_adding_1_additional_file_successfully.

run_test_and_clean_up(|path| {
create_file_at(path.clone(), Some(FILE_SIZE_2), Some(FILE_NAME_2));
let mut actual = ResourceIndex::build(path.clone());

println!("==🌹🌹=update_all unit tests start==");
for (key, value) in &actual.path2id {
println!(
"===❤❤========update_one_new_entry Resource ID: {}",
&actual.path2id[key].id
);
}

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

let update = actual
.update_all()
.expect("Should update index correctly");

for (key, value) in &actual.path2id {
println!(
"===😂😂========update_after_entry Resource ID: {}",
&actual.path2id[key].id
);
}

println!(
"===✔✔==added=:{} deleted={}",
update.added.len().to_string(),
update.deleted.len().to_string()
);

assert_eq!(update.deleted.len(), 0);
assert_eq!(update.added.len(), 1);
})
}

#[test]
fn should_correctly_update_all_using_update_one_resource() {
run_test_and_clean_up(|path| {
create_file_at(path.clone(), Some(FILE_SIZE_2), Some(FILE_NAME_2));
let mut actual = ResourceIndex::build(path.clone());

let mut all_deleted: HashSet<ResourceId> = HashSet::new();
let mut all_added: HashMap<CanonicalPathBuf, ResourceId> =
HashMap::new();

println!("==🌹🌹=update_all using update_one unit tests start==");
for (key, value) in &actual.path2id {
println!(
"===❤❤========update_one_new_entry Resource ID: {}",
value.id
);
}

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is just 1 iteration, at this moment.

let one_index_update = actual
.clone()
.update_one(&old_path.clone(), old_index_enry.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);
}
}

let curr_paths: HashMap<CanonicalPathBuf, DirEntry> =
discover_paths(path.clone());
let curr_entries = scan_entries(curr_paths);

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);
}
}
Comment on lines +1130 to +1142
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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


for (key, entry) in &actual.path2id {
println!(
"===😂😂========update_after_entry Resource ID: {}",
entry.id
);
}

println!(
"===✔✔==added=:{} deleted={}",
all_added.len().to_string(),
all_deleted.len().to_string()
);
})
}

#[test]
fn should_correctly_update_all_compare_update_one_resource() {
run_test_and_clean_up(|path| {
create_file_at(path.clone(), Some(FILE_SIZE_2), Some(FILE_NAME_2));
let mut actual = ResourceIndex::build(path.clone());
create_file_at(path.clone(), Some(FILE_SIZE_1), Some(FILE_NAME_1));

let updated_using_update_all_result = actual.clone().update_all();

let mut all_deleted: HashSet<ResourceId> = HashSet::new();
let mut all_added: HashMap<CanonicalPathBuf, ResourceId> =
HashMap::new();

let curr_paths: HashMap<CanonicalPathBuf, DirEntry> =
discover_paths(path.clone());
let curr_entries = scan_entries(curr_paths);

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);
}
}

let updated_using_update_one = IndexUpdate {
deleted: all_deleted,
added: all_added,
};
Comment on lines +1190 to +1193
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);
        })
    }

let updated_using_update_all =
updated_using_update_all_result.unwrap();

assert_eq!(updated_using_update_all, updated_using_update_one);
})
}
}