check_diff function rewrite

[benluiwj]

No description provided.

[ytmimi]

We're on the right track here, but I think there are a few things we need to double check.

1. The Display output of diffy::Patch under a few different scenarios:

• when the two &str inputs to diffy::create_patch are the same
• when the two &str inputs to diffy::create_patch are different
• The best way I can think to test this would be to create a new trait CodeFormatter with a single function format_code that matches the definition we've already defined for RustfmtRunner::format_code. Then you can implement the trait for RustfmtRunner using the current definition, and make CheckDiffRunners generic over T: CodeFormatter. Should be simple enough at that point to create a unit test where you initialize a CheckDiffRunners with mock implementation of CodeFormatter to test the output of calling CheckDiffRunners::create_diff

2. Invoking rustfmt correctly. We probably should write a unit test where we at least compile rustfmt from source and run it on a simple misformatted input and make sure we get back the expected output. We should also review how rust-analyzer calls rustfmt and follow a similar approach, but adapt it for the arguments we want to pass when calling rustfmt.

3. Abstracting the .rs files search and unit testing it to make sure that we're picking up all .rs files in the current directory and any nested directories.

[ytmimi]

Thanks for making some updates! Take a look at the inline comments below.

[ytmimi]

This is a good start, and definitely helps demonstrate what we can expect to get back from create_path when the input is the same and when it's different, but I don't think we should be directly testing a 3rd party API in this way. We should be calling our own functions e.g CheckDiffRunners::create_diff, and making assertions based on what they return.

[ytmimi]

Instead of making these fields public let's add getter methods.

[ytmimi]

Taking a look at on of the test failures, I think CI is failing because the code isn't formatted using rustfmt built from source.

[ytmimi]

why not use diffy;?

[ytmimi]

Instead of needing to compile both the src and feature binaries to test this can we simply just call build_rustfmt_from_src to build the local version of rustfmt? You may need to modify the function signature to take the current working directly so that we can set it for any of the subcommands with Command::current_dir. You'll probably also need to add the current_working directory as an argument to get_ld_library_path, and update the call to std::fs::copy("target/release/rustfmt", &binary_path)?; to be relative to the current working directory.

[ytmimi]

I think this is a cleaver way to test check_diff. But, the downside is that it requires network access to actually test the check_diff function. I think it would be great if we didn't need to build either of the rustfmt binaries in order to do so. As I mentioned in an earlier comment we could create a trait named something like CodeFormatter, that matches the format_code signature we've already defined for RustfmtRunner. Then the CheckDiffRunner could be made generic over the CodeFormatter, which will make testing check_diff a lot simpler.

pub struct CheckDiffRunner<F, S> {
    feature_runner: F,
    src_runner: S,
}

impl<F, S> CheckDiffRunner<F, S> {
    pub fn new(feature_runner: F, src_runner: S) -> Self {
        Self { feature_runner, src_runner }
    }
}

impl<F, S> CheckDiffRunner<F, S>
Where
    F: CodeFormatter,
    S: CodeFormatter,
{
    /// Creates a diff generated by running the source and feature binaries on the same file path
    pub fn create_diff(
        &self,
        path: &Path,
        additional_configs: &Option<Vec<String>>,
    ) -> Result<Diff, CheckDiffError> {
        let code = std::fs::read_to_string(path)?;
        let src_format = self.src_runner.format_code(&code, additional_configs)?;
        let feature_format = self.feature_runner.format_code(&code, additional_configs)?;
        Ok(Diff {
            src_format,
            feature_format,
        })
    }
}

Then the test(s) could be something like this:

#[test]
fn check_diff_test_no_formatting_differences() {
    let runner = CheckDiffRunner::new(
        /* `CodeFormatter` implementations that agree on the formatting */
    );
    let errors = check_diff(None, runner, dir.path());
    assert!(errors == 0);
}

#[test]
fn check_diff_test_with_formatting_differences() {
    let runner = CheckDiffRunner::new(
        /* `CodeFormatter` implementations that don't agree on the formatting */
    );
    let errors = check_diff(None, runner, dir.path());
    assert!(errors >= 1);
}

[ytmimi]

This comment isn't 100% accurate anymore. We aren't calling the rustfmt binary with the --check flag, and the arguments to the method are different.

[ytmimi]

Would be nice to add a doc comment for create_config_arg.

[ytmimi]

Would be nice to add a doc comment for search_for_rs_files, especially because it's marked as pub.

[ytmimi]

Would be nice to add a doc comment for check_diff, especially because it's marked as pub.