Skip to content

cli/tests: move a lot of the cli tests to gitoxide #5575

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 13 commits into from

Conversation

bsdinis
Copy link
Contributor

@bsdinis bsdinis commented Feb 4, 2025

In keeping with #5548 , this moves a lot of the current cli tests to gitoxide.
This doesn't do it all. but thought it best to send this as is, as there is a lot of code to review already (even though most of it is relatively straightforward). Nevertheless, this is what is missing:

  • push/fetch/clone/remote require adding git remotes, which is not easy. Maybe when git: port remote management to gix #5553 is merged we can use that machinery directly here
  • test_git_colocated: i have it mostly done locally, but not quite
  • test_git_init: ditto

This PR also consolidates a lot of what was copy pasted code (namely creating new repos and adding commits to it). This, plus gitoxide's idiosyncrasies make most of the git hashes in the tests change, hence cargo-insta was run in a separate commit to avoid the noise.

The new gitoxide helpers in cli/tests/common/gitoxide.rs help centralize a lot of common functionality, and make sure we don't run into GitoxideLabs/gitoxide#1829.

Overall, this PR has a lot of commits to help reviewing, but at the end of the day, I think the commit structure should be:

  1. comment code commit
  2. move test environment to its own file
  3. add gitoxide helpers
  4. cli/tests: migrate (some) tests to gitoxide

@bsdinis bsdinis force-pushed the bsdinis/srruzkxpyysz branch from 57ad301 to 230f526 Compare February 4, 2025 06:34
@bsdinis bsdinis force-pushed the bsdinis/srruzkxpyysz branch 10 times, most recently from 523dc68 to a4632fe Compare February 6, 2025 07:30
@bsdinis
Copy link
Contributor Author

bsdinis commented Feb 6, 2025

Managed to port another test (test_git_init).
This required adding quite a bit of machinery to the gitoxide helpers (namely ways to look at the status and to add stuff to the index).

@bsdinis bsdinis force-pushed the bsdinis/srruzkxpyysz branch 6 times, most recently from 93d73fe to c9d7d11 Compare February 6, 2025 21:29
@bsdinis
Copy link
Contributor Author

bsdinis commented Feb 6, 2025

Ported test_gitignore and test_git_colocated

@bsdinis bsdinis force-pushed the bsdinis/srruzkxpyysz branch from c9d7d11 to a56c843 Compare February 6, 2025 21:31
Copy link
Contributor

@yuja yuja left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed the first two patches.

I wouldn't make this PR grow indefinitely because GitHub doesn't support per-patch approval.

@bsdinis bsdinis force-pushed the bsdinis/srruzkxpyysz branch 2 times, most recently from 0ffe93a to ac5aabf Compare February 7, 2025 17:47
Copy link
Contributor

@yuja yuja left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Commits up to "cli/tests: port test_git_init to gitoxide" look good, thanks.

@@ -145,7 +120,6 @@ fn test_git_colocated_unborn_bookmark() {
◆ 0000000000000000000000000000000000000000
"###);
// Staged change shouldn't persist.
checkout_index();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to either check out the index content to working copy or assert the index contents to ensure that the Git index is reset by jj new root().

let git_repo = init_git_repo(&workspace_root, false);
git_repo.config().unwrap().remove("core.bare").unwrap();
let _git_repo = init_git_repo(&workspace_root, false);
//git_repo.config().unwrap().remove("core.bare").unwrap();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you insert a snapshot of .git/config to ensure that core.bare key doesn't exist?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is don

The `mod.rs` was becoming crowded and this makes way for unrelated
additions to the test utilities module
@bsdinis bsdinis force-pushed the bsdinis/srruzkxpyysz branch 2 times, most recently from a19ec73 to ddc9f55 Compare February 9, 2025 20:28
@bsdinis bsdinis force-pushed the bsdinis/srruzkxpyysz branch from ddc9f55 to fc2a52b Compare February 9, 2025 22:27
cargo-insta was re-run.
This is due to centralizing git interaction code to use a particular
different signature, which changes the commit hashes
cargo-insta was re-run.
This is due to centralizing git interaction code to use a particular
different signature, which changes the commit hashes
cargo-insta was re-run.
This is due to centralizing git interaction code to use a particular
different signature, which changes the commit hashes
cargo-insta was re-run.
This is due to centralizing git interaction code to use a particular
different signature, which changes the commit hashes
@bsdinis bsdinis force-pushed the bsdinis/srruzkxpyysz branch from fc2a52b to 0a97779 Compare February 10, 2025 03:28
@bsdinis bsdinis closed this Feb 10, 2025
@bsdinis bsdinis deleted the bsdinis/srruzkxpyysz branch February 10, 2025 04:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants