Skip to content
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

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

Merged
merged 10 commits into from
Feb 11, 2025

Conversation

bsdinis
Copy link
Contributor

@bsdinis bsdinis commented Feb 10, 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:

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/git.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

This PR was originally #5575 but Github exists to make my life miserable and I unwittingly removed the branch the PR was based on

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 4 times, most recently from 93c3b4e to 69ad9c8 Compare February 10, 2025 22:30
@bsdinis bsdinis force-pushed the bsdinis/srruzkxpyysz branch from 69ad9c8 to c7b4f17 Compare February 11, 2025 03:05
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.

Thanks!

cli/Cargo.toml Show resolved Hide resolved
cli/tests/test_git_import_export.rs Outdated Show resolved Hide resolved
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 c7b4f17 to b4b9915 Compare February 11, 2025 13:31
@bsdinis bsdinis enabled auto-merge February 11, 2025 13:43
@bsdinis bsdinis added this pull request to the merge queue Feb 11, 2025
Merged via the queue into main with commit 39fe9aa Feb 11, 2025
39 checks passed
@bsdinis bsdinis deleted the bsdinis/srruzkxpyysz branch February 11, 2025 14:00
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.

2 participants