Fix acceptance test failures: git identity, headRepository JSON, obsolete traversal test#13037
Conversation
The sandbox overrides HOME so git cannot find the user's global config, causing 'Author identity unknown' errors when acceptance test scripts make commits. Write a minimal .gitconfig with user.name and user.email into the sandbox working directory during sharedSetup. Co-Authored-By: Copilot <223556219+Copilot@users.noreply.github.com>
GitHub's Artifact API now rejects artifact names like '..' server-side with a 400 Bad Request, making it impossible to create artifacts with path traversal names. This means the scenario this test was verifying (that gh run download catches traversal names) can no longer be reproduced through normal artifact creation. The client-side traversal check in gh run download remains in place as a defense-in-depth measure. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Commit dd424d8 added NameWithOwner to PRRepository for agent-task listings but didn't update the headRepository GraphQL query to fetch it. This caused gh pr view --json headRepository to emit an empty "nameWithOwner":"" field, breaking the pr-create-respects-simple- pushdefault acceptance test. Fetch nameWithOwner in the query and update the test assertion to expect it. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Fixes multiple acceptance test failures by ensuring git commits work inside the testscript sandbox, correcting a GraphQL selection regression for headRepository.nameWithOwner, and removing an acceptance test scenario that can no longer be reproduced due to upstream API validation.
Changes:
- Write a minimal
.gitconfiginto the acceptance test sandbox sogit commitdoesn’t fail due to missing identity. - Fetch
nameWithOwnerforheadRepositoryin the PR GraphQL fragment and update the affected acceptance assertion. - Remove the obsolete
run-download-traversalacceptance test.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
acceptance/acceptance_test.go |
Sets up sandbox git identity via .gitconfig so commit-making scripts succeed. |
api/query_builder.go |
Extends headRepository GraphQL fragment to include nameWithOwner. |
acceptance/testdata/pr/pr-create-respects-simple-pushdefault.txtar |
Updates expected JSON output to include headRepository.nameWithOwner. |
acceptance/testdata/workflow/run-download-traversal.txtar |
Removes an acceptance test that’s no longer viable due to server-side artifact name validation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "github.com/cli/cli/v2/internal/ghcmd" | ||
| "github.com/cli/go-internal/testscript" | ||
| "github.com/MakeNowJust/heredoc" | ||
| ) |
There was a problem hiding this comment.
Import block isn’t gofmt-sorted (third-party imports should be lexicographically ordered). As-is, the gofmt formatter (enabled in golangci-lint) will report this file; please run gofmt/goimports to normalize the import order.
There was a problem hiding this comment.
Well that's just not true. Our linter seems happy.
| case "headRepository": | ||
| q = append(q, `headRepository{id,name}`) | ||
| q = append(q, `headRepository{id,name,nameWithOwner}`) | ||
| case "assignees": |
There was a problem hiding this comment.
This change to the headRepository fragment isn’t currently covered by api/query_builder_test.go. Since there are already unit tests asserting the generated fragments for other fields, consider adding a case that asserts IssueGraphQL([]string{"headRepository"}) (or via PullRequestGraphQL) includes nameWithOwner to prevent regressions.
There was a problem hiding this comment.
Not super concerned with this personally.
There was a problem hiding this comment.
Fix 2 here looks like it breaks a lot of other tests, which are skipped by default right now:
- pr-create-push-default-upstream-no-merge-ref-fork.txtar:50
- pr-create-respects-push-destination.txtar:53
- pr-create-remote-ref-with-branch-name-slash.txtar:46
- pr-create-respects-remote-pushdefault.txtar:49
- pr-create-guesses-remote-from-sha.txtar:48
- pr-create-respects-user-colon-branch-syntax.txtar:47
- pr-create-guesses-remote-from-sha-with-branch-name-slash.txtar:50
- pr-create-respects-branch-pushremote.txtar:49
Or at least, maybe they were already broken? In any case, we should fix them.
|
I fixed these failures in 971be97 |
Description
Fixes several acceptance test failures found while running the suite locally. Each fix is in its own commit.
Fix 1: Git identity in testscript sandbox
The testscript sandbox overrides
HOME, so git inside the sandbox can't find the user's globaluser.name/user.email. This causes "Author identity unknown" errors in any acceptance test that makes commits (e.g. extensions, PRs). The fix writes a minimal.gitconfiginto the sandbox duringsharedSetup.Fix 2: Fetch
nameWithOwnerinheadRepositoryGraphQL querydd424d8 (agent-task listings) added
NameWithOwnerto thePRRepositorystruct but didn't update theheadRepositoryGraphQL query to fetch it. Sinceexport_pr.godumps the raw struct,gh pr view --json headRepositorystarted emitting"nameWithOwner":""- breaking thepr-create-respects-simple-pushdefaultacceptance test assertion.The fix adds
nameWithOwnerto the GraphQL query so the field is actually populated, and updates the test assertion to expect it.Fix 3: Remove
run-download-traversalacceptance testGitHub's Artifact API now rejects artifact names like
..server-side (400 Bad Request), making it impossible to create artifacts with path traversal names. The test scenario can no longer be reproduced through normal artifact creation. The client-side traversal check ingh run downloadremains as defense-in-depth.Known failures not addressed
These were observed during the acceptance run but are not bugs in
gh:TestExtensions/extensionTestRepo/repo-list-renameTestRepo/repo-archive-unarchiveTestRulesets/rulesetgh ruleset checkTestSecrets/secret-orgCommits
nameWithOwnerinheadRepositoryGraphQL query to fix regression from dd424d8 that brokepr-create-respects-simple-pushdefaultrun-download-traversaltest since GitHub's Artifact API now prevents the scenario server-side