Add graphite standalone command tests - #2112
Conversation
Converts the flat tests/integration/graphite.rs into a graphite/ module so
the gt runner can be shared with the upcoming remote-backed test suite.
- graphite/graphite_test_harness.rs: the gt() runner, git-shim helpers,
require_gt!, and the shared setup/assert helpers, lifted verbatim.
- graphite/local_ops.rs: the existing tests, unchanged.
main.rs needs no edit: `mod graphite;` resolves to graphite/mod.rs. Test
paths shift to graphite::local_ops::*, so TEST_FILTER=graphite still
selects the whole suite.
No behavior change; all 21 non-ignored tests pass as before.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| let number = output.trim().to_string(); | ||
| if number.is_empty() { | ||
| None | ||
| } else { | ||
| Some(number) | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 Pull-request check passes even when no pull request was created
The lookup for a branch's pull request treats the literal text "null" as a real result (.[0].number at tests/integration/graphite/graphite_test_harness.rs:475-476), so a branch with no pull request still reports one and the check that a pull request was opened can never fail.
Impact: The remote submit test reports success even if Graphite never opened a pull request, hiding real regressions.
Why gh emits the string "null" and how it defeats the assertion
gh pr list ... --json number --jq '.[0].number' on a repo with no matching PR evaluates [] | .[0].number, which is null; gh prints the literal null on stdout with exit status 0. pr_number_for_branch only treats an empty string as None (tests/integration/graphite/graphite_test_harness.rs:480-485), so it returns Some("null"), and assert!(remote.pr_number_for_branch(&branch).is_some()) in tests/integration/graphite/remote_ops.rs:51-54 is always true.
The cleanup script added in the same PR guards against exactly this: if [ -n "$PR_NUMBER" ] && [ "$PR_NUMBER" != "null" ] (tests/integration/graphite/scripts/cleanup-test-branches.sh:79).
Secondary nit in the same call: no --state open filter is passed even though the doc comment says "open pull request number" (gh pr list defaults to open, so this is only a documentation/robustness concern).
| let number = output.trim().to_string(); | |
| if number.is_empty() { | |
| None | |
| } else { | |
| Some(number) | |
| } | |
| } | |
| let number = output.trim().to_string(); | |
| if number.is_empty() || number == "null" { | |
| None | |
| } else { | |
| Some(number) | |
| } | |
| } |
Was this helpful? React with 👍 or 👎 to provide feedback.
| fn clone_test_repo(test_repo: &str, token: &str, destination: &Path) { | ||
| let url = format!("https://x-access-token:{token}@github.com/{test_repo}.git"); | ||
|
|
||
| let output = Command::new(real_git_executable()) | ||
| .args(["clone", &url, destination.to_str().unwrap()]) | ||
| // Fail fast on a bad token instead of blocking on a credential prompt, | ||
| // and ignore any system gitconfig that might rewrite the remote URL. | ||
| .env("GIT_TERMINAL_PROMPT", "0") | ||
| .env("GIT_CONFIG_NOSYSTEM", "1") | ||
| .output() | ||
| .unwrap_or_else(|error| panic!("failed to execute git clone: {error}")); | ||
|
|
||
| assert!( | ||
| output.status.success(), | ||
| "failed to clone {test_repo}:\n{}", | ||
| // Redact the token so a clone failure cannot leak it into test output. | ||
| String::from_utf8_lossy(&output.stderr).replace(token, "***"), | ||
| ); |
There was a problem hiding this comment.
🟨 GitHub token embedded in the clone's origin URL can leak into test output and git config
The clone URL embeds the GitHub PAT (https://x-access-token:{token}@github.com/... at tests/integration/graphite/graphite_test_harness.rs:682). While the clone failure message redacts the token (tests/integration/graphite/graphite_test_harness.rs:697), the token is persisted in .git/config of the temp clone and is included in the output of any later git remote -v/git push failure surfaced by TestRepo::git (e.g. the teardown error print at tests/integration/graphite/graphite_test_harness.rs:614-620) and by gt stdout/stderr returned from gt() (tests/integration/graphite/graphite_test_harness.rs:279-294), which tests print on expect panics.
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
Graphite's "sync" code paths perform some interesting logic in order to resolve drift between local and remote versions of branches. There are a number of combinations that can cause different behavior in Graphite. It would be nice to have test coverage for these combination to ensure Git AI attribution survives the different operations.
This PR add tests for those combinations and call out 2 test cases where attribution appears to be dropped (both appears to happen when pulling changes from the remote repo).
Note: This PR is stacked on (and depends on) the changes in #2111. But since stacks on not supported with forked repos, I'm just included the commit with those changes here. This may be easier to review by just viewing the top commit, or rebasing the changing after #2111 has merged.