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.

check_diff/tests/git.rs

check_diff/tests/bash_commands.rs

check_diff/tests/git.rs

ytmimi · 2024-07-16T00:23:46Z

From what I can tell I don't think we're actually running the check-diff test suite in our CI. We'll need to update build_and_test.bat and build_and_test.sh to cd into the check_diff directory and run the tests after we've run the config_proc_macro tests.

benluiwj · 2024-07-16T02:47:18Z

I will update the respective files - doing something similar to config_proc_macro

ci/build_and_test.bat

ci/build_and_test.sh

ytmimi · 2024-07-17T23:34:21Z

@benluiwj thanks for the updates. It's good to see CI passing. This PR has gone through several rounds of review and there are a lot of intermedia commits that could be squashed down. I think it might make sense to squash the implementation and unit test commits into a single commit and then have another commit that updates CI. let me know if that's something you'd want to do, otherwise I can make that change.

ytmimi

Things are looking good here!

Included unit tests for the new functionality as well.

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 force-pushed the check-diff-add-git-funcs branch from e428b98 to 7ff0250 Compare June 18, 2024 07:23

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

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 requested a review from ytmimi June 28, 2024 08:44

ytmimi requested changes Jul 6, 2024

View reviewed changes

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

ytmimi requested changes Jul 10, 2024

View reviewed changes

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

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

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

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

benluiwj requested a review from ytmimi July 11, 2024 02:02

ytmimi reviewed Jul 16, 2024

View reviewed changes

ci/build_and_test.bat Outdated Show resolved Hide resolved

ytmimi reviewed Jul 16, 2024

View reviewed changes

ci/build_and_test.sh Outdated Show resolved Hide resolved

benluiwj requested a review from ytmimi July 17, 2024 02:31

benluiwj force-pushed the check-diff-add-git-funcs branch 5 times, most recently from ff4c37c to 66ea607 Compare July 24, 2024 09:37

ytmimi approved these changes Jul 24, 2024

View reviewed changes

ytmimi added pr-ready-to-merge and removed pr-waiting-on-author labels Jul 24, 2024

benluiwj force-pushed the check-diff-add-git-funcs branch from 66ea607 to fe1eb36 Compare July 24, 2024 10:14

benluiwj added 2 commits August 3, 2024 06:52

Add cd and git clone functionality to check-diff crate

e0b8237

Included unit tests for the new functionality as well.

Run check-diff tests in ci

6e7f0fb

ytmimi force-pushed the check-diff-add-git-funcs branch from fe1eb36 to 6e7f0fb Compare August 3, 2024 10:55

ytmimi merged commit a1361bd into rust-lang:master Aug 3, 2024
26 checks passed

benluiwj deleted the check-diff-add-git-funcs branch August 12, 2024 03:21

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 commented Jul 16, 2024

benluiwj commented Jul 16, 2024

ytmimi commented Jul 17, 2024 •

edited

Loading

ytmimi left a comment •

edited

Loading

Add repo cloning to check-diff crate #6187

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

ytmimi commented Jul 16, 2024

benluiwj commented Jul 16, 2024

ytmimi commented Jul 17, 2024 • edited Loading

ytmimi left a comment • edited Loading

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

ytmimi commented Jul 17, 2024 •

edited

Loading

ytmimi left a comment •

edited

Loading