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]

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]

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]

Can you double check that the FailedFindingRustFiles and FailedWritingToFile variants are being used. I couldn't find any references to them. If they're not in use then I'd suggest removing them.

[ytmimi]

Took another look, I think we're good on this one. Thanks!