Skip to content

Isolate tests from local machine's auth and git configuration - #14128

Merged
babakks merged 6 commits into
trunkfrom
bagtoad/probable-funicular
Aug 11, 2026
Merged

Isolate tests from local machine's auth and git configuration#14128
babakks merged 6 commits into
trunkfrom
bagtoad/probable-funicular

Conversation

@BagToad

@BagToad BagToad commented Aug 10, 2026

Copy link
Copy Markdown
Member

Description

This fixes some test failures that are a result of a not-fully-isolated testing setup.

There are two leaks:

  • Auth. The existing NewIsolatedTestConfig isolates the config file, but go-gh reads GH_TOKEN and friends first. Anyone with a token exported gets an authenticated config from a helper meant to return a blank one.
  • Git. The existing git tests redirect GIT_CONFIG_GLOBAL, which is not enough. GIT_CONFIG_COUNT and GIT_CONFIG_PARAMETERS are command line scope and outrank it.

This also fixes Test_mightBeGHESUser, whose cases named "no env vars" never cleared any.

How did you test this change?

One shell throughout, exporting GH_TOKEN, GH_HOST, and safe.bareRepository=explicit via GIT_CONFIG_COUNT:

  • Before, on trunk: 11 packages fail.
  • After, on this branch: all pass.

The expectation is that the suite ignores its environment entirely, so I ran the same command in three more: a normal shell, a stripped CI-like one, and one with a ~/.gitconfig setting commit.gpgsign, core.autocrlf, and a credential.helper.

Key points

  • I added a parameter to NewIsolatedTestConfig, that felt like the best solution with the goal of funneling test configs, rather than a second helper you have to know to reach for.
  • The mock got some love too with a rename to be more clear about its purpose.
  • Isolation clears the auth variables, so tests setting their own must do it after.

Notes for reviewers

  • a4ed81c Rename config stub file to test.go. Rename only, to match git/test.go.
  • 5e8ec5d Isolate git config in tests that shell out to git. Fixes the git leak. Two copies of a leaky local helper become one exported git.IsolateConfig.
  • e5b9c16 Clear auth environment variables in isolated test config. Fixes the auth leak.
  • 1e5e85c Accept config content in NewIsolatedTestConfig. The new parameter lets tests that need populated hosts use it, which is how Test_mightBeGHESUser gets fixed.
  • efd568e Document when to use the config mock or the isolated config. Comments only.
  • 5dfc6b0 Rename config mock constructors to match repo convention. Mechanical, 73 files, skippable.

Commits 2 to 4 are the substance, and most of that is call sites.

Authorship and follow-up

Who wrote this:

  • A human wrote it.
  • An agent wrote it under close human direction.
  • An agent wrote it independently, and no human has guided the implementation beyond the initial prompt.

Who answers review comments:

  • @BagToad will read and reply directly.
  • An agent will draft replies and @username will read them before they are posted.
  • Nobody has explicitly committed to replying.

BagToad and others added 6 commits August 10, 2026 12:44
@BagToad
Rename config stub file to test.go
a4ed81c 
Matches the convention already used by internal/prompter, pkg/cmd/run/shared,
and pkg/cmd/workflow/shared for their test helper files.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@BagToad
Isolate git config in tests that shell out to git
5e8ec5d 
Config from GIT_CONFIG_COUNT and GIT_CONFIG_PARAMETERS is command line scope,
which outranks both the global and system files, so pointing GIT_CONFIG_GLOBAL
at an empty file was not enough on its own. An inherited safe.bareRepository=explicit
made git refuse to open the bare fixtures at all.

Replaces the copy of this helper in each of the two packages that needed it.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@BagToad
Clear auth environment variables in isolated test config
e5b9c16 
go-gh reads GH_TOKEN and friends ahead of any stored config, so isolating the
config file left tests authenticated for anyone with a token exported. Nine
packages failed as a result.

The token test set its own env vars before building the config, which the
clearing would undo, so it now sets them after.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@BagToad
Accept config content in NewIsolatedTestConfig
1e5e85c 
The helper only ever produced an empty config, so tests needing populated hosts
had to reach for the mock instead and lost the env isolation that comes with it.

Test_mightBeGHESUser was one of those. Its cases named "no env vars" never
cleared any, so they failed whenever GH_ENTERPRISE_TOKEN or GH_HOST was set.
Its table now holds config strings rather than prebuilt configs.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@BagToad
Document when to use the config mock or the isolated config
efd568e 
Neither mock constructor had a doc comment, so nothing signalled which of the
three to reach for. Each now says what it does and points at the other.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@BagToad
Rename config mock constructors to match repo convention
5dfc6b0 
NewBlankConfig and NewFromString gave no hint that what came back was a mock,
which made them the default choice even where the real config was wanted. Every
other mock constructor in the repo already announces itself, as in NewMockPrompter
and NewMockFinder.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@BagToad
BagToad requested a review from a team as a code owner August 10, 2026 23:12
Copilot AI balanced review requested due to automatic review settings August 10, 2026 23:12
@BagToad
BagToad requested review from a team as code owners August 10, 2026 23:12
@BagToad
BagToad requested a review from babakks August 10, 2026 23:12
@BagToad BagToad changed the title Isolate tests from ambient auth and git configuration Isolate tests from local machine's auth and git configuration Aug 10, 2026
Copilot AI reviewed Aug 10, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Hardens tests against local authentication and Git configuration leakage.

Changes:

  • Isolates auth and Git environment settings.
  • Adds configurable isolated configs and fixes GHES-user tests.
  • Renames config mock constructors and migrates callers.
Show a summary per file
File Description
pkg/cmdutil/auth_check_test.go Updates isolated-config invocation.
pkg/cmd/variable/set/set_test.go Migrates config mocks.
pkg/cmd/variable/list/list_test.go Migrates config mocks.
pkg/cmd/variable/get/get_test.go Migrates config mocks.
pkg/cmd/variable/delete/delete_test.go Migrates config mocks.
pkg/cmd/status/status_test.go Migrates config mock.
pkg/cmd/ssh-key/list/list_test.go Migrates config mock.
pkg/cmd/ssh-key/delete/delete_test.go Migrates config mock.
pkg/cmd/ssh-key/add/add_test.go Migrates config mock.
pkg/cmd/skills/update/update_test.go Migrates config mocks.
pkg/cmd/skills/search/search_test.go Migrates config mocks.
pkg/cmd/send-telemetry/send_telemetry_test.go Migrates config mock.
pkg/cmd/secret/set/set_test.go Migrates config mocks.
pkg/cmd/secret/list/list_test.go Migrates config mocks.
pkg/cmd/secret/delete/delete_test.go Migrates config mocks.
pkg/cmd/search/shared/shared_test.go Migrates config mock.
pkg/cmd/search/code/code_test.go Migrates config mocks.
pkg/cmd/run/view/view_test.go Migrates config mock.
pkg/cmd/root/help_test.go Migrates config mock.
pkg/cmd/root/extension_registration_test.go Migrates config mock.
pkg/cmd/repo/view/view_test.go Migrates config mock.
pkg/cmd/repo/rename/rename_test.go Migrates config mock.
pkg/cmd/repo/list/list_test.go Migrates config mocks.
pkg/cmd/repo/license/view/view_test.go Migrates config mock.
pkg/cmd/repo/license/list/list_test.go Migrates config mock.
pkg/cmd/repo/gitignore/view/view_test.go Migrates config mock.
pkg/cmd/repo/gitignore/list/list_test.go Migrates config mock.
pkg/cmd/repo/fork/fork_test.go Updates isolated-config invocation.
pkg/cmd/repo/create/create_test.go Migrates config mock.
pkg/cmd/repo/clone/clone_test.go Migrates config mock.
pkg/cmd/release/create/create_test.go Migrates config mock.
pkg/cmd/project/unlink/unlink_test.go Migrates config mocks.
pkg/cmd/project/link/link_test.go Migrates config mocks.
pkg/cmd/pr/status/status_test.go Migrates config mock.
pkg/cmd/pr/review/review_test.go Migrates config mock.
pkg/cmd/pr/create/create_test.go Migrates config mocks.
pkg/cmd/pr/checkout/checkout_test.go Migrates config mocks.
pkg/cmd/org/list/list_test.go Migrates config mock.
pkg/cmd/issue/view/view_test.go Migrates config mock.
pkg/cmd/issue/unpin/unpin_test.go Migrates config mock.
pkg/cmd/issue/transfer/transfer_test.go Migrates config mock.
pkg/cmd/issue/status/status_test.go Migrates config mock.
pkg/cmd/issue/reopen/reopen_test.go Migrates config mock.
pkg/cmd/issue/pin/pin_test.go Migrates config mock.
pkg/cmd/issue/list/list_test.go Migrates config mock.
pkg/cmd/issue/delete/delete_test.go Migrates config mock.
pkg/cmd/issue/create/create_test.go Migrates config mocks.
pkg/cmd/gpg-key/list/list_test.go Migrates config mock.
pkg/cmd/gpg-key/delete/delete_test.go Migrates config mock.
pkg/cmd/gpg-key/add/add_test.go Migrates config mock.
pkg/cmd/gist/view/view_test.go Migrates config mock.
pkg/cmd/gist/rename/rename_test.go Migrates config mock.
pkg/cmd/gist/list/list_test.go Migrates config mock.
pkg/cmd/gist/edit/edit_test.go Migrates config mock.
pkg/cmd/gist/delete/delete_test.go Migrates config mock.
pkg/cmd/gist/create/create_test.go Migrates config mock.
pkg/cmd/gist/clone/clone_test.go Migrates config mock.
pkg/cmd/factory/default_test.go Migrates config mocks.
pkg/cmd/extension/manager_test.go Migrates config mock.
pkg/cmd/extension/command_test.go Migrates config mock.
pkg/cmd/extension/browse/browse_test.go Migrates config mock.
pkg/cmd/config/set/set_test.go Migrates config mocks.
pkg/cmd/config/list/list_test.go Migrates config mocks.
pkg/cmd/config/get/get_test.go Migrates config mocks.
pkg/cmd/auth/token/token_test.go Isolates auth before setting test variables.
pkg/cmd/auth/switch/switch_test.go Updates isolated-config invocation.
pkg/cmd/auth/status/status_test.go Updates isolated-config invocation.
pkg/cmd/auth/shared/gitcredentials/updater_test.go Uses shared Git isolation.
pkg/cmd/auth/shared/gitcredentials/helper_config_test.go Replaces local Git isolation helper.
pkg/cmd/auth/setupgit/setupgit_test.go Updates isolated-config invocation.
pkg/cmd/auth/refresh/refresh_test.go Updates isolated-config invocation.
pkg/cmd/auth/logout/logout_test.go Updates isolated-config invocations.
pkg/cmd/auth/login/login_test.go Updates isolated-config invocations.
pkg/cmd/attestation/verify/verify_integration_test.go Migrates config mocks.
pkg/cmd/api/api_test.go Migrates config mocks.
pkg/cmd/alias/set/set_test.go Migrates config mock.
pkg/cmd/alias/list/list_test.go Migrates string-backed config mock.
pkg/cmd/alias/imports/import_test.go Migrates string-backed config mock.
pkg/cmd/alias/delete/delete_test.go Migrates string-backed config mock.
pkg/cmd/agent-task/agent_task_test.go Migrates config mocks.
internal/ghcmd/cmd_test.go Isolates GHES tests and migrates mocks.
internal/config/test.go Defines renamed and isolated config helpers.
internal/config/auth_config_test.go Updates isolated-config invocation.
git/test.go Adds shared Git isolation helper.
git/client_test.go Isolates real Git invocations.
cmd/gen-docs/main.go Migrates documentation generator config.

Review details

Tip

Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Suppressed comments (3)

git/test.go:23

  • 🛑 Requirement: GIT_CONFIG_COUNT must not be set to an empty string. Git parses any present value as an integer, so this causes fatal: bogus count in GIT_CONFIG_COUNT and every real Git invocation using this helper fails even in a clean environment. Use 0 to disable inherited key/value pairs while preserving t.Setenv cleanup.
	t.Setenv("GIT_CONFIG_COUNT", "")

internal/config/test.go:26

  • The claim that this mock never writes is incorrect: its WriteFunc delegates to cfg.Write, which calls the real go-gh filesystem writer. Tests avoid writes only when they do not call Write or override/isolate it, so document that behavior explicitly to prevent callers from relying on a false safety guarantee.
    internal/config/test.go:139
  • 🛑 Requirement: Add a deterministic regression test that sets each auth/host variable before calling this helper and verifies the returned real config cannot observe it. The current callers set their test variables only after isolation, so CI with a clean environment would still pass if one of these newly added clearing rules were removed.
  • Files reviewed: 86/86 changed files
  • Comments generated: 1
  • Review effort level: Balanced

Comment thread git/test.go
babakks
babakks approved these changes Aug 11, 2026
View reviewed changes

@babakks babakks left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM! Thanks for fixing this! 🙌

@babakks
babakks merged commit a526307 into trunk Aug 11, 2026
26 checks passed
@babakks
babakks deleted the bagtoad/probable-funicular branch August 11, 2026 09:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot
Copilot review effort
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments
@babakks babakks babakks approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@BagToad @babakks