Skip to content
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

Unit test for filehash.rs #8

Merged
merged 2 commits into from Aug 12, 2021
Merged

Unit test for filehash.rs #8

merged 2 commits into from Aug 12, 2021

Conversation

kiavash-at-work
Copy link
Contributor

@kiavash-at-work kiavash-at-work commented Aug 11, 2021

This PR includes:

  • unit tests for filehash.rs functions.
    • tempfile crate was brought into scope only for unit tests [dev-dependencies]. It is used for generating temp folder in the system temp directory. This temp folder will be removed by OS as soon as its variable goes out of scope.
  • removed pub from get_filehash_local() and get_filehash_http_sftp().
    • They are called only by get_filehash() which is already in the scope.
  • refactored the error conversation to String from get_filehash_local() and get_filehash_http_sftp() to get_filehash()
    • This move makes it possible to implement unit tests for get_filehash_local() and get_filehash_http_sftp() that checks and verifies the complete error codes.
    • The code is easier to read and follow because now the Err to String conversation happens only once in each branch of get_filehash().
    • The unit test for get_filehash() only verifies the first 5 character of the error message because get_filehash() returns error in String and to avoid variation in error message implementations by the underlying OS.

Importing `tempfile` crate for unit testing.
1. Implementation of unit test
2. Refactored error parsing to the `pub` facing function.
@kiavash-at-work
Copy link
Contributor Author

kiavash-at-work commented Aug 11, 2021

While reviewing all the calls to get_filehash() which are located here and here and here, it seems all
of these references expect Err(Box<dyn Error>) while get_filehash() currently returns Err(String). I didn't change get_filehash() return value but I think it makes sense to change it to Err(Box<dyn Error>). If that is inline with the rest of code error handling, I will update it in the next PR. The change should be minimal

Replace function here with

use std::error::Error;

pub fn get_filehash(file: String, port: u16) -> Result<Vec<u8>, Box<dyn Error>> {
    if file.starts_with("https://") || file.starts_with("http://") || file.starts_with("sftp://") {
        get_filehash_http_sftp(file, port)
            .map_err(|e| e.into())
    } else {
        get_filehash_local(file)
            .map_err(|e| e.into())
    }
}

and unit test here with

                        e.to_string().truncate(truncate_size),

@ashuio
Copy link
Owner

ashuio commented Aug 12, 2021

Thanks for the PR, I'll merge it. Sorry for the delay.

@ashuio ashuio merged commit 047f0a5 into ashuio:0.1.4 Aug 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants