Add repo cloning to check-diff crate #6187

benluiwj · 2024-06-06T13:47:17Z

TLDR: Added repo cloning to check-diff. Included sample execution as screenshot.

This PR adds git functionalities to the check-diff crate. The current script has a init_submodules function. However, it seems that the repos used does not require initialising of submodules. Hence, this function was not converted to Rust.
Sample execution attached. Parameters to the program is currently not in used.

The screenshot shows the following things:

runs the script to clone rustfmt into a tmp directory created
show that tmp contains the entire rustfmt repo

ytmimi

Great first pass, though I think we can make some changes to take better advantage of the type system, and keep functions more focused on a single task. There are also a few other suggestions that I've made. Take a look and let me know if you've got any questions on the feedback.

check_diff/src/main.rs

benluiwj · 2024-06-19T03:16:23Z

@ytmimi Sorry for the big PR. Things updated based on the previous review:

Added unit tests
Refactored clone_github_repo function and updated its return type
Replaced println in main functions with tracing instead

Some questions though:

tracing has the concept of a span. I was thinking where should this span be, if it exists. There are 2 places I can think of, 1) in the function itself or 2) in main.
The tests have a common cleanup function that removes the tmp directory created. The reason its called twice is because I am unsure of the order of execution of the tests. Is there a better way to do this?

Let me know what do you think of the updated implementation!

ps the PR is huge because I added tracing. Most changes are from the Cargo.lock file actually. The actual code is really about 100 lines (including tests)

ytmimi

Great to see that you've added some test cases to cover the functionality that you're adding. I left a few more comments about returning Results from functions instead of panicing and also I think the tempfile crate would be a nice addition to the test suite so that we don't need to roll our own temporary file and directory cleanup.

check_diff/tests/common/mod.rs

check_diff/tests/git.rs

check_diff/src/lib.rs

benluiwj · 2024-06-28T08:44:35Z

Changes made from last review:

Added tempfile crate for unit tests
Polished Result of git (clone) and unix (cd) functionalities

Questions:

Since errors are possible when creating the directories using tempfile, I made an assertion that is guaranteed to fail in the testcase. Not sure if there are better ways to do this. What do you think?
tempfile creates directories with arbitrary names (even using prefix and suffix does not seem to mitigate it). Thus for cd_test, I have an assertion that says the current directory name is not the home directory (check_diff). This is a little odd as well since we should be testing whether we actually made it into a path that we want, not that we're not in the home directory. What do you feel should be the way we resolve this (if it needs to be resolved)?

ytmimi

I left some additional comments on the cd_test and clone_repo_test functions. When testing I think it's totally fine to call unwrap, and I think doing so would simplify the test cases. I also left some suggestions for more direct assertions. Overall I think the addition of the tempfile crate helped improve the tests.

ytmimi · 2024-07-06T18:25:27Z

check_diff/tests/bash_commands.rs

+#[test]
+fn cd_test() {
+    // create directory in current directory
+    let dir = Builder::new().tempdir_in("");


I don't think we need to use the builder here. Probably simpler to use the tempdir function and just unwrap since we're in a test case. If we fail to create the temporary directory then there's not much more we can do.

ytmimi · 2024-07-06T18:27:01Z

check_diff/tests/bash_commands.rs

+            match d.close() {
+                Ok(_) => {}
+                Err(e) => {
+                    error!("Error from closing: {}", e);
+                    assert_eq!(1, 2);
+                }
+            }


Can you explain why we need to call close here.

ytmimi · 2024-07-06T18:30:33Z

check_diff/tests/bash_commands.rs

+    match dir {
+        Ok(d) => {
+            let dest_path = d.path();
+            let _ = change_directory_to_path(dest_path);


Probably simpler to just unwrap in the test instead of silently ignoring the returned Result.

the Result here is actually ignored because we are actually changing the env variable.

ytmimi · 2024-07-06T18:31:15Z

check_diff/tests/bash_commands.rs

+        Ok(d) => {
+            let dest_path = d.path();
+            let _ = change_directory_to_path(dest_path);
+            assert_eq!(env::current_dir().is_ok(), true);


I feel like it would be a more direct test if you asserted that the current working directory is the same as the dest_path

ytmimi · 2024-07-06T18:36:38Z

check_diff/tests/bash_commands.rs

+            assert_ne!(
+                env::current_dir().unwrap().file_name(),
+                Some(OsStr::new("check_diff"))
+            );


This is the code you were referring to in #6187 (comment), right? My feeling is that this isn't a necessary check and if we assert that we've correctly changed the working directory to the tempdir then I don't think this is necessary.

ytmimi · 2024-07-06T18:39:50Z

check_diff/tests/git.rs

+
+#[test]
+fn clone_repo_test() {
+    let dir = Builder::new().tempdir_in("");


Similar comment as before. It's probably simpler to use the tempdir function and just unwrap if we fail to create a tempdir.

ytmimi · 2024-07-06T18:41:04Z

check_diff/tests/git.rs

+            // check whether we can read this directory
+            assert_eq!(directory.is_err(), false);
+            // check that the directory is non-empty
+            assert_eq!(directory.iter().next().is_none(), false);


Since we've cloned a git repo into this repository we might want to double check that a .git directory exists with the tempdir. That might be a more direct assertion. Checking that we're able to reach the directly could be useful, but my assumption would be that if we're able to write to the tempdir then we'd be able to read from it as well.

ytmimi · 2024-07-06T18:42:04Z

check_diff/tests/git.rs

+            match d.close() {
+                Ok(_) => {}
+                Err(e) => {
+                    error!("Error from closing: {}", e);
+                    assert_eq!(1, 2);
+                }
+            }
+        }


Similar question as before. I'm not sure if we need to manually close the directory since it should be closed and cleaned up at the end of the test already.

benluiwj · 2024-07-08T03:23:37Z

Updated most of the changes that were requested as I agreed with them. The use of Builder is actually to force tempfile to create directory in the current crate directory instead of at std::env::temp_dir(). I suspect that if the testcase were to fail, the directory will not be deleted. Thus, having the temporary directory created in the same crate allows the user to manually delete the folder if that happens.

ytmimi

Thanks for continuing to work on this. Take a look at my suggestions for simplifying the assertions.

ytmimi · 2024-07-10T03:22:56Z

check_diff/tests/git.rs

+    let sample_repo = "https://github.com/rust-lang/rustfmt.git";
+    let dest_path = dir.path();
+    let result = clone_git_repo(sample_repo, dest_path);
+    assert_eq!(result.is_ok(), true);


Simpler to use assert! here since you're testing a boolean value.

Suggested change

assert_eq!(result.is_ok(), true);

assert!(result.is_ok());

ytmimi · 2024-07-10T03:25:00Z

check_diff/tests/bash_commands.rs

+    // Creates an empty directory in the current working directory
+    let dir = Builder::new().tempdir_in("").unwrap();
+    let dest_path = dir.path();
+    let _ = change_directory_to_path(dest_path).unwrap();


change_directory_to_path returns io::Result<()>. No need to bind this to anything if you're just unwrapping:

Suggested change

let _ = change_directory_to_path(dest_path).unwrap();

change_directory_to_path(dest_path).unwrap();

ytmimi · 2024-07-10T03:30:52Z

check_diff/tests/git.rs

+    let directory = fs::read_dir(dest_path);
+    // check that the directory is non-empty
+    assert_eq!(directory.iter().next().is_none(), false);


Probably better to do a positive assertion. Also, you can use .is_some() along with assert! to check the boolean.

Suggested change

let directory = fs::read_dir(dest_path);

// check that the directory is non-empty

assert_eq!(directory.iter().next().is_none(), false);

let directory = fs::read_dir(dest_path);

// check that the directory is non-empty

assert!(directory.iter().next().is_some());

This check is a little unnecessary since we're about to check that a .git folder exists, which would also prove that the directory isn't empty.

ytmimi · 2024-07-10T03:35:36Z

check_diff/tests/git.rs

+    // check whether a .git folder exists in directory
+    let mut is_git_present = false;
+    for path in directory.unwrap() {
+        if path.unwrap().file_name() == ".git" {
+            is_git_present = true;
+        }
+    }
+    assert_eq!(is_git_present, true);


Instead of looping we can test this more directly with PathBuf::exists:

Suggested change

// check whether a .git folder exists in directory

let mut is_git_present = false;

for path in directory.unwrap() {

if path.unwrap().file_name() == ".git" {

is_git_present = true;

}

}

assert_eq!(is_git_present, true);

// check whether a .git folder exists after cloning the repo

let git_repo = dest_path.join(".git");

assert!(git_repo.exists());

rustbot added the pr-not-reviewed label Jun 6, 2024

ytmimi requested changes Jun 12, 2024

View reviewed changes

check_diff/src/main.rs Outdated Show resolved Hide resolved

check_diff/src/main.rs Outdated Show resolved Hide resolved

check_diff/src/main.rs Outdated Show resolved Hide resolved

check_diff/src/main.rs Outdated Show resolved Hide resolved

ytmimi added pr-waiting-on-author and removed pr-not-reviewed labels Jun 12, 2024

benluiwj added 2 commits June 18, 2024 15:23

add repo cloning fn

5953546

improve error handling

7ff0250

benluiwj force-pushed the check-diff-add-git-funcs branch from e428b98 to 7ff0250 Compare June 18, 2024 07:23

benluiwj added 7 commits June 18, 2024 16:01

update param types

85c3afc

add unit tests

9c77ff2

add cd functionality

0b7d2e9

fix env vars

1914b8e

update tests

d9d77fe

fix return type of cloning git

8b4a781

refactor tests

e24503a

benluiwj requested a review from ytmimi June 19, 2024 03:12

benluiwj added 3 commits June 19, 2024 11:16

cleanup common functions

90259f2

add tracing

c2fd13b

update logs

c0555aa

ytmimi requested changes Jun 25, 2024

View reviewed changes

check_diff/tests/common/mod.rs Outdated Show resolved Hide resolved

check_diff/tests/git.rs Outdated Show resolved Hide resolved

check_diff/src/lib.rs Outdated Show resolved Hide resolved

check_diff/src/lib.rs Outdated Show resolved Hide resolved

benluiwj added 3 commits June 28, 2024 12:23

fix results

cd441a5

add tempfile to tests

49b49da

add logging for test errors

7bc1b8e

benluiwj requested a review from ytmimi June 28, 2024 08:44

ytmimi requested changes Jul 6, 2024

View reviewed changes

refine tests

aab6178

benluiwj requested a review from ytmimi July 8, 2024 03:24

improve tests

611fc47

ytmimi requested changes Jul 10, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add repo cloning to check-diff crate #6187

Add repo cloning to check-diff crate #6187

benluiwj commented Jun 6, 2024 •

edited

Loading

ytmimi left a comment

benluiwj commented Jun 19, 2024 •

edited

Loading

ytmimi left a comment

benluiwj commented Jun 28, 2024 •

edited

Loading

ytmimi left a comment •

edited

Loading

ytmimi Jul 6, 2024

ytmimi Jul 6, 2024

ytmimi Jul 6, 2024

benluiwj Jul 8, 2024

ytmimi Jul 6, 2024

ytmimi Jul 6, 2024

ytmimi Jul 6, 2024

ytmimi Jul 6, 2024

ytmimi Jul 6, 2024

benluiwj commented Jul 8, 2024

ytmimi left a comment

ytmimi Jul 10, 2024

ytmimi Jul 10, 2024

ytmimi Jul 10, 2024

ytmimi Jul 10, 2024

ytmimi Jul 10, 2024

	let _ = change_directory_to_path(dest_path).unwrap();
	change_directory_to_path(dest_path).unwrap();

Add repo cloning to check-diff crate #6187

Are you sure you want to change the base?

Add repo cloning to check-diff crate #6187

Conversation

benluiwj commented Jun 6, 2024 • edited Loading

ytmimi left a comment

Choose a reason for hiding this comment

benluiwj commented Jun 19, 2024 • edited Loading

ytmimi left a comment

Choose a reason for hiding this comment

benluiwj commented Jun 28, 2024 • edited Loading

ytmimi left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benluiwj commented Jul 8, 2024

ytmimi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benluiwj commented Jun 6, 2024 •

edited

Loading

benluiwj commented Jun 19, 2024 •

edited

Loading

benluiwj commented Jun 28, 2024 •

edited

Loading

ytmimi left a comment •

edited

Loading