Unit tests for PNGme steganography exercise in Rust

Question

I want to become proficient in Rust and I've followed the PNGme exercise. I've completed it up until Chapter 4 included, where I've written a lot of tests myself for verifying that the command line arguments work as expected and the involved files are manipulated correctly.

Unfortunately, the tests have gotten quite messy and I'm looking for suggestions on how to refactor them or perhaps move them in a different file/module. In addition, by following the proper TDD etiquette I've always written the minimum amount of code to make tests pass, so there might be room for adding even more specialized tests.

I also want to ask about my approach in error handling all over the project, with the use of unwrap() mostly for laziness or for not knowing how/if I should deal with a certain issue in a certain way. Speaking about code coverage, I have yet to try it because I'm not familiar with tools used for Rust, so there might be cases which are not currently covered by tests and they would result in uncaught errors.

Anyway, the place where I'm mostly looking for improvements is src/args.rs. All the tests found there involve side effects with files: I've already looked for better ways to test that kind of behavior and the README of the project provides a very detailed explanation of the possible solutions.

I've included a test as an example of the kind of code I've written, but be mindful that many other tests are actually shorter and simpler. Please excuse the lack of full code here, it would be quite impractical since we're talking about a thousand or more lines of code. Even the src/args.rs file from which this test is taken is more than 600 lines long, so I feel it would be useless to just dump it all here with no context nor relation to other parts and modules of the project.

Here's the repo.

#[test]
fn test_encode_existing_file_with_separate_output() {
    prepare_file(FILE_NAME);

    let new_chunk = test_chunk().unwrap();
    let args = parse_args(&[ENCODE, FILE_NAME, "TeSt", "I am a test chunk", OUTPUT_NAME]).unwrap();

    if let CommandType::Encode(encode_args) = args.command_type {
        encode_args.encode().unwrap();

        let png_from_input_file = Png::try_from(&read_file(FILE_NAME)[..]).unwrap();
        let png_from_output_file = Png::try_from(&read_file(OUTPUT_NAME)[..]).unwrap();

        assert_eq!(
            png_from_input_file.as_bytes(),
            testing_png_full().as_bytes()
        );
        assert_eq!(
            png_from_output_file.as_bytes(),
            testing_png_full()
                .as_bytes()
                .iter()
                .chain(new_chunk.as_bytes().iter())
                .cloned()
                .collect::<Vec<u8>>()
        );
        delete_file(FILE_NAME);
        delete_file(OUTPUT_NAME);
    }
}

fn prepare_file(file_name: &str) {
    create_file(file_name);

    let png = testing_png_full();
    let mut file = File::options().write(true).open(file_name).unwrap();

    file.write_all(&png.as_bytes()).unwrap();
}

fn create_file(file_name: &str) {
    File::create(file_name).unwrap();
}

fn testing_png_full() -> Png {
    let chunks = vec![
        chunk_from_strings("FrSt", "I am the first chunk").unwrap(),
        chunk_from_strings("miDl", "I am another chunk").unwrap(),
        chunk_from_strings("LASt", "I am the last chunk").unwrap(),
    ];

    Png::from_chunks(chunks)
}

// this helper function was already present as part of the tests provided
// by the exercise and it could be made to always return a valid Chunk
fn chunk_from_strings(chunk_type: &str, data: &str) -> Result<Chunk> {
    let chunk_type = ChunkType::from_str(chunk_type)?;
    let data: Vec<u8> = data.bytes().collect();

    Ok(Chunk::new(chunk_type, data))
}

fn test_chunk() -> Result<Chunk> {
    chunk_from_strings("TeSt", "I am a test chunk")
}

fn parse_args(args: &[&str]) -> clap::Result<PngMeArgs> {
    PngMeArgs::try_parse_from(std::iter::once("pngme").chain(args.iter().cloned()))
}

fn read_file(file_name: &str) -> Vec<u8> {
    let mut buffer = Vec::<u8>::new();
    let mut file = File::open(file_name).unwrap();

    file.read_to_end(&mut buffer).unwrap();
    buffer
}

fn delete_file(file_name: &str) {
    fs::remove_file(file_name).unwrap();
}

Answer 1

#[test]
fn test_encode_existing_file_with_separate_output() {
    prepare_file(FILE_NAME);

Your strategy of reusing a single file name in all of these tests is not great. As you note in your comments, it prevents running the tests in parallel. It also concerns me what'll happen if a crash happens and a file doesn't get cleaned up.

Instead, use the tempfile crate. Probably what you'll find best is tempfile::tempdir() which will give you a temporary directory which will automatically be cleaned up. Simple generate any paths you need under the temporary directory and everything will be cleaned up nicely.

    let new_chunk = test_chunk().unwrap();
    let args = parse_args(&[ENCODE, FILE_NAME, "TeSt", "I am a test chunk", OUTPUT_NAME]).unwrap();

    if let CommandType::Encode(encode_args) = args.command_type {
        encode_args.encode().unwrap();

This code only run is the args was encode. If there was a bug somewhere causing it to parse as something else, this test would seem to pass, even though it checked nothing.

What you want to do is actually run your program with a specific command line. One way is to use an integration test which actually runs your binary as a subprocess. Another way would be to put your main logic in a function that you can call with the arguments. Then you can invoke it in a unit test or in your main function with the actual arguments.

        let png_from_input_file = Png::try_from(&read_file(FILE_NAME)[..]).unwrap();
        let png_from_output_file = Png::try_from(&read_file(OUTPUT_NAME)[..]).unwrap();

        assert_eq!(
            png_from_input_file.as_bytes(),
            testing_png_full().as_bytes()
        );
        assert_eq!(
            png_from_output_file.as_bytes(),
            testing_png_full()
                .as_bytes()
                .iter()
                .chain(new_chunk.as_bytes().iter())
                .cloned()
                .collect::<Vec<u8>>()
        );

These tests are odd on a couple of counts. Firstly, you parse the output files with Png::try_from only to convert them back into bytes. Why not just compare the bytes that were directly read from the file? Secondly, you test your code against itself. You assert the file contains the same contents as generated by your code. But if there is a bug in your code, you'll miss because both of those will agree. I think you should be asserting against a literal byte string to verify you got the correct output instead.

        delete_file(FILE_NAME);
        delete_file(OUTPUT_NAME);
    }
}

fn prepare_file(file_name: &str) {
    create_file(file_name);

    let png = testing_png_full();
    let mut file = File::options().write(true).open(file_name).unwrap();

    file.write_all(&png.as_bytes()).unwrap();

Why are you creating the file, closing it, and then reopening it? Just create and write. Furthermore, rust has a function std::fs::write which given a path and contents will open, write, and close in one function call.

}



fn create_file(file_name: &str) {
    File::create(file_name).unwrap();
}

fn testing_png_full() -> Png {
    let chunks = vec![
        chunk_from_strings("FrSt", "I am the first chunk").unwrap(),
        chunk_from_strings("miDl", "I am another chunk").unwrap(),
        chunk_from_strings("LASt", "I am the last chunk").unwrap(),
    ];

    Png::from_chunks(chunks)
}

// this helper function was already present as part of the tests provided
// by the exercise and it could be made to always return a valid Chunk
fn chunk_from_strings(chunk_type: &str, data: &str) -> Result<Chunk> {
    let chunk_type = ChunkType::from_str(chunk_type)?;
    let data: Vec<u8> = data.bytes().collect();

    Ok(Chunk::new(chunk_type, data))
}

fn test_chunk() -> Result<Chunk> {
    chunk_from_strings("TeSt", "I am a test chunk")
}

fn parse_args(args: &[&str]) -> clap::Result<PngMeArgs> {
    PngMeArgs::try_parse_from(std::iter::once("pngme").chain(args.iter().cloned()))
}

fn read_file(file_name: &str) -> Vec<u8> {
    let mut buffer = Vec::<u8>::new();
    let mut file = File::open(file_name).unwrap();

    file.read_to_end(&mut buffer).unwrap();
    buffer
}

Rust has a std::fs::read which does exactly this for you.

fn delete_file(file_name: &str) {
    fs::remove_file(file_name).unwrap();
}