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

#41: Add unit tests, fix update_one #72

Closed
wants to merge 1 commit into from
Closed

Conversation

kirillt
Copy link
Member

@kirillt kirillt commented Jan 20, 2024

Here are couple more tests.

update_one was handling absent paths incorrectly, fixed here.

Closing:

src/index.rs Outdated Show resolved Hide resolved
@kirillt
Copy link
Member Author

kirillt commented Jan 20, 2024

src/index.rs Outdated Show resolved Hide resolved
@funnyVOLT
Copy link

funnyVOLT commented Jan 23, 2024

@kirillt
I would like to inform you of my opinion.
The problem occurred while comparing 'update_all' and 'track_methods'.

  1. When a new file is created.
    When files with the same ResourceID are added, update_all and track_addition operate differently.
  2. When a new file is modified
    After the file is changed, update_all does not seem to reflect the changed information.
    The handling of collisions in the 'track_update' function is not reflected clearly.
  3. Overall, I think we need to check 'collisions' again.

@kirillt
Copy link
Member Author

kirillt commented Jan 24, 2024

@funnyVOLT thank you for the test case. I'll look into it deeper soon. It's great that you've discovered corner cases I've missed!

Could you create small isolated test cases reproducing these 2 problems?

  1. When files with the same ResourceID are added, update_all and track_addition operate differently.
  1. After the file is changed, update_all does not seem to reflect the changed information.

@funnyVOLT
Copy link

funnyVOLT commented Jan 24, 2024

@funnyVOLT thank you for the test case. I'll look into it deeper soon. It's great that you've discovered corner cases I've missed!

Could you create small isolated test cases reproducing these 2 problems?

  1. When files with the same ResourceID are added, update_all and track_addition operate differently.
  1. After the file is changed, update_all does not seem to reflect the changed information.

@kirillt , I've pushed test case codes.

@ARK-Builders ARK-Builders deleted a comment from Kirill Jan 25, 2024
@ARK-Builders ARK-Builders deleted a comment from funnyVOLT Jan 25, 2024
src/index.rs Outdated Show resolved Hide resolved
src/index.rs Outdated Show resolved Hide resolved
src/index.rs Outdated Show resolved Hide resolved
src/index.rs Outdated
// create
1 => {
let mut file_name_new = String::from(cur_file_name);
file_name_new.push_str("_new.txt");
Copy link
Member Author

Choose a reason for hiding this comment

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

Would be better to provide new random unique name, automatically.

Choose a reason for hiding this comment

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

Would be better to provide new random unique name, automatically.

I added const variable CREATE, UPDATE, DELETE, MOVE .

src/index.rs Outdated Show resolved Hide resolved
src/index.rs Outdated
Comment on lines 1149 to 1152
let mut folder_1 = root_path.clone();
let mut folder_2 = root_path.clone();
folder_1.push(FILE_DIR_1);
folder_2.push(FILE_DIR_2);
Copy link
Member Author

Choose a reason for hiding this comment

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

Do we really need 2 folders?

Choose a reason for hiding this comment

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

Do we really need 2 folders?

It is necessary for file movement function.

Copy link
Member Author

Choose a reason for hiding this comment

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

We can also just move to new random filename. This way we can apply move operation multiple times, no need to track in which of the two the file resides.

funnyVOLT added a commit to funnyVOLT/arklib that referenced this pull request Feb 1, 2024
src/index.rs Outdated Show resolved Hide resolved
src/index.rs Outdated Show resolved Hide resolved
src/index.rs Outdated Show resolved Hide resolved
src/index.rs Outdated Show resolved Hide resolved
src/index.rs Outdated
const FILE_NAME: &str = "test_";
const FILE_COUNT: i32 = 10;
const FILE_COUNT: i32 = 3;
Copy link
Member Author

@kirillt kirillt Feb 5, 2024

Choose a reason for hiding this comment

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

Isn't 3 too few for proper testing?

src/index.rs Outdated
@@ -1320,7 +1319,7 @@ mod tests {
.update_all()
.expect("Should update index correctly");

assert_eq!(index_track_addition, index_update_all);
assert_ne!(index_track_addition, index_update_all);
Copy link
Member Author

Choose a reason for hiding this comment

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

Must be equal, isn't it?

.update_all()
.expect("Should update index correctly");

assert_ne!(
Copy link
Member Author

Choose a reason for hiding this comment

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

After this assertion we should sleep for 1ms, perform update_all again and assert_eq new index.

Copy link
Collaborator

Choose a reason for hiding this comment

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

regarding this test,
Why should the result from track_addition and update_all be different?

Copy link
Collaborator

@tareknaser tareknaser left a comment

Choose a reason for hiding this comment

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

Great job!
Left some comments.

I think we can also add simple doc comments on the methods implemented for ResourceIndex

src/index.rs Outdated Show resolved Hide resolved
src/index.rs Outdated Show resolved Hide resolved
src/index.rs Outdated Show resolved Hide resolved
src/index.rs Outdated Show resolved Hide resolved
src/index.rs Outdated Show resolved Hide resolved
src/index.rs Outdated Show resolved Hide resolved
src/index.rs Outdated Show resolved Hide resolved
src/index.rs Show resolved Hide resolved
@kirillt kirillt assigned tareknaser and unassigned funnyVOLT Feb 6, 2024
@kirillt kirillt changed the title Add unit tests, fix update_one #41: Add unit tests, fix update_one Feb 6, 2024
Copy link
Collaborator

@tareknaser tareknaser left a comment

Choose a reason for hiding this comment

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

some more comments but we are getting there

src/index.rs Outdated Show resolved Hide resolved
src/index.rs Outdated Show resolved Hide resolved
crc32: CRC32_2,
}
}

fn run_test_and_clean_up(
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am not sure we need this function
we can use TempDir::new to achieve the same output

tempdir crate automatically cleans up the temporary directories when they go out of scope

Copy link
Collaborator

Choose a reason for hiding this comment

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

I will leave this one for a follow up PR

Comment on lines +1251 to +1261
match update_state.try_into() {
Ok(FileAction::CREATE) => {
let _ = index1.track_addition(&new_file_path);
}
Ok(FileAction::UPDATE) => {
let _ = index1.track_update(&file_path, old_id);
}
Ok(FileAction::DELETE) => {
let _ = index1.track_deletion(old_id);
}
Ok(FileAction::MOVE) => {
let _ = index1.track_deletion(old_id);
let _ = index1.track_addition(&new_file_path);
}
Err(_) => println!("rnd_num error"),
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are we intentionally ignoring errors here for testing purposes?

If not, we should panic with errors

Suggested change
match update_state.try_into() {
Ok(FileAction::CREATE) => {
let _ = index1.track_addition(&new_file_path);
}
Ok(FileAction::UPDATE) => {
let _ = index1.track_update(&file_path, old_id);
}
Ok(FileAction::DELETE) => {
let _ = index1.track_deletion(old_id);
}
Ok(FileAction::MOVE) => {
let _ = index1.track_deletion(old_id);
let _ = index1.track_addition(&new_file_path);
}
Err(_) => println!("rnd_num error"),
}
match update_state.try_into() {
Ok(FileAction::CREATE) => {
index1
.track_addition(&new_file_path)
.expect("Should track addition correctly");
}
Ok(FileAction::UPDATE) => {
index1
.track_update(&file_path, old_id)
.expect("Should track update correctly");
}
Ok(FileAction::DELETE) => {
index1
.track_deletion(old_id)
.expect("Should track deletion correctly");
}
Ok(FileAction::MOVE) => {
index1
.track_deletion(old_id)
.expect("Should track deletion correctly");
index1
.track_addition(&new_file_path)
.expect("Should track addition correctly");
}
Err(_) => println!("rnd_num error"),
}

Copy link
Member Author

Choose a reason for hiding this comment

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

@tareknaser yes, we must panic and not ignore the errors. But maybe simple unwrapping would be fine in test code?


let mut added_file_path = path.clone();
added_file_path.push(FILE_NAME_2);
let _ = index_track_addition.track_addition(&added_file_path);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Here as well

src/index.rs Outdated Show resolved Hide resolved
src/index.rs Outdated Show resolved Hide resolved
@tareknaser tareknaser mentioned this pull request Feb 7, 2024
Signed-off-by: Tarek <tareknaser360@gmail.com>
@kirillt
Copy link
Member Author

kirillt commented Sep 16, 2024

@kirillt kirillt closed this Sep 16, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants