test/2024-06-08-connect-remote-shims #22
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @ackkerman, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request significantly enhances the test coverage for the gh-sync connect command. It introduces specialized Git shims to simulate various remote configurations, enabling robust testing of how gh-sync interacts with Git when remotes are either absent or have differing URLs. This ensures the command correctly performs git remote add or git remote set-url operations as needed.
Highlights
- Test Infrastructure: Added two new Git shims (
fake_git_remote_missingandfake_git_remote_mismatch) to simulate specificgit remote get-urlbehaviors (failure for missing, different URL for mismatch). These shims allow for controlled testing ofgh-sync's interaction with Git remotes. - New Tests: Introduced new integration tests (
connect_adds_remote_when_missingandconnect_updates_remote_url) to verify that thegh-sync connectcommand correctly handles adding missing remotes and updating mismatched remote URLs, leveraging the newly created Git shims.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments or fill out our survey to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request introduces helper shims for git remote add/set-url cases and covers connect scenarios for missing or mismatched remotes. The review suggests improvements to the maintainability and portability of the test code by addressing code duplication and hardcoded values. Refactoring the duplicated shim creation and test setup logic into helper functions, and replacing hardcoded paths and platform-specific separators with more robust alternatives are recommended.
| fn fake_git_remote_missing(dir: &tempfile::TempDir) -> std::path::PathBuf { | ||
| let shim = dir.path().join("git"); | ||
| fs::write( | ||
| &shim, | ||
| r#"#!/usr/bin/env sh | ||
| if [ "$1" = "config" ]; then | ||
| /usr/bin/git "$@" | ||
| elif [ "$1" = "remote" ] && [ "$2" = "get-url" ]; then | ||
| exit 1 | ||
| else | ||
| echo git "$@" | ||
| exit 0 | ||
| fi | ||
| "#, | ||
| ) | ||
| .unwrap(); | ||
| #[cfg(unix)] | ||
| { | ||
| use std::os::unix::fs::PermissionsExt; | ||
| fs::set_permissions(&shim, fs::Permissions::from_mode(0o755)).unwrap(); | ||
| } | ||
| shim | ||
| } | ||
|
|
||
| /// `git remote get-url` を成功させつつ異なる URL を返し、set-url を誘発するシム | ||
| fn fake_git_remote_mismatch(dir: &tempfile::TempDir) -> std::path::PathBuf { | ||
| let shim = dir.path().join("git"); | ||
| fs::write( | ||
| &shim, | ||
| r#"#!/usr/bin/env sh | ||
| if [ "$1" = "config" ]; then | ||
| /usr/bin/git "$@" | ||
| elif [ "$1" = "remote" ] && [ "$2" = "get-url" ]; then | ||
| echo https://example.com/other.git | ||
| exit 0 | ||
| else | ||
| echo git "$@" | ||
| exit 0 | ||
| fi | ||
| "#, | ||
| ) | ||
| .unwrap(); | ||
| #[cfg(unix)] | ||
| { | ||
| use std::os::unix::fs::PermissionsExt; | ||
| fs::set_permissions(&shim, fs::Permissions::from_mode(0o755)).unwrap(); | ||
| } | ||
| shim |
There was a problem hiding this comment.
There's significant code duplication between fake_git_remote_missing and fake_git_remote_mismatch. The logic for creating the file, writing the script content, and setting permissions is nearly identical. To improve maintainability and reduce redundancy, consider extracting this repeated logic into a single helper function that accepts the script content as an argument.
| &shim, | ||
| r#"#!/usr/bin/env sh | ||
| if [ "$1" = "config" ]; then | ||
| /usr/bin/git "$@" |
There was a problem hiding this comment.
The path to the git executable is hardcoded as /usr/bin/git. This can make the tests less portable if git is installed in a different location on other systems. Consider dynamically finding the path to the git executable during test setup and injecting it into the shim script.
| &shim, | ||
| r#"#!/usr/bin/env sh | ||
| if [ "$1" = "config" ]; then | ||
| /usr/bin/git "$@" |
There was a problem hiding this comment.
Similar to line 68, the path to the git executable is hardcoded as /usr/bin/git. Consider dynamically finding the path to the git executable during test setup and injecting it into the shim script to improve portability.
tests/cli.rs
Outdated
| fn connect_adds_remote_when_missing() { | ||
| let repo = setup_repo(); | ||
| let git_shim = fake_git_remote_missing(&repo); | ||
|
|
||
| let path_env = format!( | ||
| "{}:{}", | ||
| git_shim.parent().unwrap().display(), | ||
| std::env::var("PATH").unwrap() | ||
| ); | ||
|
|
||
| Command::cargo_bin("gh-sync") | ||
| .unwrap() | ||
| .current_dir(repo.path()) | ||
| .env("PATH", &path_env) | ||
| .args(&["connect", "web", "git@github.com:a/b.git"]) | ||
| .assert() | ||
| .success() | ||
| .stdout(predicate::str::contains("git remote add")); | ||
| } |
There was a problem hiding this comment.
The setup logic within connect_adds_remote_when_missing and connect_updates_remote_url is largely identical. This includes creating the temporary repository, creating the git shim, and constructing the PATH environment variable. Extracting this common setup into a helper function would improve maintainability.
| let path_env = format!( | ||
| "{}:{}", | ||
| git_shim.parent().unwrap().display(), | ||
| std::env::var("PATH").unwrap() | ||
| ); | ||
|
|
||
| Command::cargo_bin("gh-sync") | ||
| .unwrap() | ||
| .current_dir(repo.path()) | ||
| .env("PATH", &path_env) | ||
| .args(&["connect", "web", "git@github.com:a/b.git"]) | ||
| .assert() | ||
| .success() | ||
| .stdout(predicate::str::contains("git remote set-url")); | ||
| } |
There was a problem hiding this comment.
The setup logic within connect_adds_remote_when_missing and connect_updates_remote_url is largely identical. This includes creating the temporary repository, creating the git shim, and constructing the PATH environment variable. Extracting this common setup into a helper function would improve maintainability.
| "{}:{}", | ||
| git_shim.parent().unwrap().display(), | ||
| std::env::var("PATH").unwrap() | ||
| ); |
There was a problem hiding this comment.
The PATH variable is constructed using a hardcoded colon (:), which is specific to Unix-like systems. Use the platform-agnostic std::env::consts::PATH_SEPARATOR for better portability. Also, using .unwrap() on std::env::var("PATH") can cause the test to panic if the PATH variable is not set. Using .expect() with a descriptive message is more robust.
| "{}:{}", | |
| git_shim.parent().unwrap().display(), | |
| std::env::var("PATH").unwrap() | |
| ); | |
| let path_env = format!( | |
| "{}{}{}", | |
| git_shim.parent().unwrap().display(), | |
| std::env::consts::PATH_SEPARATOR, | |
| std::env::var("PATH").expect("PATH environment variable must be set") | |
| ); |
| "{}:{}", | ||
| git_shim.parent().unwrap().display(), | ||
| std::env::var("PATH").unwrap() | ||
| ); |
There was a problem hiding this comment.
The PATH variable is constructed using a hardcoded colon (:), which is not portable. Use std::env::consts::PATH_SEPARATOR. Also, unwrap() on an environment variable can lead to fragile tests. Use expect() with a clear error message to improve diagnostics if the test fails.
| "{}:{}", | |
| git_shim.parent().unwrap().display(), | |
| std::env::var("PATH").unwrap() | |
| ); | |
| let path_env = format!( | |
| "{}{}{}", | |
| git_shim.parent().unwrap().display(), | |
| std::env::consts::PATH_SEPARATOR, | |
| std::env::var("PATH").expect("PATH environment variable must be set") | |
| ); |
Summary
Testing
cargo test -- --nocapturehttps://chatgpt.com/codex/tasks/task_e_6880affbbf8483308b8b1fc21850281b