Skip to content

Executing TestDeleteRun alias test does not safe guard pre-existing configuration #10590

Closed
#10683
Closed
Executing TestDeleteRun alias test does not safe guard pre-existing configuration#10590
#10683
Assignees
andyfeller
Labels
bugSomething isn't working

Description

@andyfeller
andyfeller
opened on Mar 12, 2025

Describe the bug

Collaborating with @BagToad after raising awareness of his aliases disappearing unpredictably, we have been periodically checking in whether my aliases have disappeared. This morning, I believe I found the cause due to a missing safeguard within the gh alias delete test below, which does not mock Config.WriteFunc() function the same as gh alias set test does:

cli/pkg/cmd/alias/delete/delete_test.go

Lines 87 to 185 in 7924274

func TestDeleteRun(t *testing.T) {
tests := []struct {
name string
config string
isTTY bool
opts *DeleteOptions
wantAliases map[string]string
wantStdout string
wantStderr string
wantErrMsg string
}{
{
name: "delete alias",
config: heredoc.Doc(`
aliases:
il: issue list
co: pr checkout
`),
isTTY: true,
opts: &DeleteOptions{
Name: "co",
All: false,
},
wantAliases: map[string]string{
"il": "issue list",
},
wantStderr: "✓ Deleted alias co; was pr checkout\n",
},
{
name: "delete all aliases",
config: heredoc.Doc(`
aliases:
il: issue list
co: pr checkout
`),
isTTY: true,
opts: &DeleteOptions{
All: true,
},
wantAliases: map[string]string{},
wantStderr: "✓ Deleted alias co; was pr checkout\n✓ Deleted alias il; was issue list\n",
},
{
name: "delete alias that does not exist",
config: heredoc.Doc(`
aliases:
il: issue list
co: pr checkout
`),
isTTY: true,
opts: &DeleteOptions{
Name: "unknown",
},
wantAliases: map[string]string{
"il": "issue list",
"co": "pr checkout",
},
wantErrMsg: "no such alias unknown",
},
{
name: "delete all aliases when none exist",
isTTY: true,
opts: &DeleteOptions{
All: true,
},
wantAliases: map[string]string{},
wantErrMsg: "no aliases configured",
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
ios, _, stdout, stderr := iostreams.Test()
ios.SetStdinTTY(tt.isTTY)
ios.SetStdoutTTY(tt.isTTY)
ios.SetStderrTTY(tt.isTTY)
tt.opts.IO = ios
cfg := config.NewFromString(tt.config)
tt.opts.Config = func() (gh.Config, error) {
return cfg, nil
}
err := deleteRun(tt.opts)
if tt.wantErrMsg != "" {
assert.EqualError(t, err, tt.wantErrMsg)
writeCalls := cfg.WriteCalls()
assert.Equal(t, 0, len(writeCalls))
} else {
assert.NoError(t, err)
writeCalls := cfg.WriteCalls()
assert.Equal(t, 1, len(writeCalls))
}
assert.Equal(t, tt.wantStdout, stdout.String())
assert.Equal(t, tt.wantStderr, stderr.String())
assert.Equal(t, tt.wantAliases, cfg.Aliases().All())
})
}
}

cli/pkg/cmd/alias/set/set_test.go

Lines 98 to 290 in 7924274

func TestSetRun(t *testing.T) {
tests := []struct {
name string
tty bool
opts *SetOptions
stdin string
wantExpansion string
wantStdout string
wantStderr string
wantErrMsg string
}{
{
name: "creates alias tty",
tty: true,
opts: &SetOptions{
Name: "foo",
Expansion: "bar",
},
wantExpansion: "bar",
wantStderr: "- Creating alias for foo: bar\n✓ Added alias foo\n",
},
{
name: "creates alias",
opts: &SetOptions{
Name: "foo",
Expansion: "bar",
},
wantExpansion: "bar",
},
{
name: "creates shell alias tty",
tty: true,
opts: &SetOptions{
Name: "igrep",
Expansion: "!gh issue list | grep",
},
wantExpansion: "!gh issue list | grep",
wantStderr: "- Creating alias for igrep: !gh issue list | grep\n✓ Added alias igrep\n",
},
{
name: "creates shell alias",
opts: &SetOptions{
Name: "igrep",
Expansion: "!gh issue list | grep",
},
wantExpansion: "!gh issue list | grep",
},
{
name: "creates shell alias using flag tty",
tty: true,
opts: &SetOptions{
Name: "igrep",
Expansion: "gh issue list | grep",
IsShell: true,
},
wantExpansion: "!gh issue list | grep",
wantStderr: "- Creating alias for igrep: !gh issue list | grep\n✓ Added alias igrep\n",
},
{
name: "creates shell alias using flag",
opts: &SetOptions{
Name: "igrep",
Expansion: "gh issue list | grep",
IsShell: true,
},
wantExpansion: "!gh issue list | grep",
},
{
name: "creates alias where expansion has args tty",
tty: true,
opts: &SetOptions{
Name: "foo",
Expansion: "bar baz --author='$1' --label='$2'",
},
wantExpansion: "bar baz --author='$1' --label='$2'",
wantStderr: "- Creating alias for foo: bar baz --author='$1' --label='$2'\n✓ Added alias foo\n",
},
{
name: "creates alias where expansion has args",
opts: &SetOptions{
Name: "foo",
Expansion: "bar baz --author='$1' --label='$2'",
},
wantExpansion: "bar baz --author='$1' --label='$2'",
},
{
name: "creates alias from stdin tty",
tty: true,
opts: &SetOptions{
Name: "foo",
Expansion: "-",
},
stdin: `bar baz --author="$1" --label="$2"`,
wantExpansion: `bar baz --author="$1" --label="$2"`,
wantStderr: "- Creating alias for foo: bar baz --author=\"$1\" --label=\"$2\"\n✓ Added alias foo\n",
},
{
name: "creates alias from stdin",
opts: &SetOptions{
Name: "foo",
Expansion: "-",
},
stdin: `bar baz --author="$1" --label="$2"`,
wantExpansion: `bar baz --author="$1" --label="$2"`,
},
{
name: "overwrites existing alias tty",
tty: true,
opts: &SetOptions{
Name: "co",
Expansion: "bar",
OverwriteExisting: true,
},
wantExpansion: "bar",
wantStderr: "- Creating alias for co: bar\n! Changed alias co\n",
},
{
name: "overwrites existing alias",
opts: &SetOptions{
Name: "co",
Expansion: "bar",
OverwriteExisting: true,
},
wantExpansion: "bar",
},
{
name: "fails when alias name is an existing alias tty",
tty: true,
opts: &SetOptions{
Name: "co",
Expansion: "bar",
},
wantExpansion: "pr checkout",
wantErrMsg: "X Could not create alias co: name already taken, use the --clobber flag to overwrite it",
wantStderr: "- Creating alias for co: bar\n",
},
{
name: "fails when alias name is an existing alias",
opts: &SetOptions{
Name: "co",
Expansion: "bar",
},
wantExpansion: "pr checkout",
wantErrMsg: "X Could not create alias co: name already taken, use the --clobber flag to overwrite it",
},
{
name: "fails when alias expansion is not an existing command tty",
tty: true,
opts: &SetOptions{
Name: "foo",
Expansion: "baz",
},
wantErrMsg: "X Could not create alias foo: expansion does not correspond to a gh command, extension, or alias",
wantStderr: "- Creating alias for foo: baz\n",
},
{
name: "fails when alias expansion is not an existing command",
opts: &SetOptions{
Name: "foo",
Expansion: "baz",
},
wantErrMsg: "X Could not create alias foo: expansion does not correspond to a gh command, extension, or alias",
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
rootCmd := &cobra.Command{}
barCmd := &cobra.Command{Use: "bar"}
barCmd.AddCommand(&cobra.Command{Use: "baz"})
rootCmd.AddCommand(barCmd)
coCmd := &cobra.Command{Use: "co"}
rootCmd.AddCommand(coCmd)
tt.opts.validAliasName = shared.ValidAliasNameFunc(rootCmd)
tt.opts.validAliasExpansion = shared.ValidAliasExpansionFunc(rootCmd)
ios, stdin, stdout, stderr := iostreams.Test()
ios.SetStdinTTY(tt.tty)
ios.SetStdoutTTY(tt.tty)
ios.SetStderrTTY(tt.tty)
tt.opts.IO = ios
if tt.stdin != "" {
fmt.Fprint(stdin, tt.stdin)
}
cfg := config.NewBlankConfig()
cfg.WriteFunc = func() error {
return nil
}
tt.opts.Config = func() (gh.Config, error) {
return cfg, nil
}

Stepping through the debugger shows the deleteRun() call will overwrite the test executor's config.yaml (all of it!) with the final test scenario wiping out all aliases.

I believe this is due to the test missing the following safeguard:

cli/pkg/cmd/alias/set/set_test.go

Lines 285 to 287 in 7924274

cfg.WriteFunc = func() error {
return nil
}

Affected version

N/A

Steps to reproduce the behavior

$ go clean -testcache 

$ cat ~/.config/gh/config.yml                                                 
aliases: {slackd: slack read -d}
version: "1"

$ go test -run "^TestDeleteRun$" github.com/cli/cli/v2/pkg/cmd/alias/delete     
ok  	github.com/cli/cli/v2/pkg/cmd/alias/delete	0.253s

$ cat ~/.config/gh/config.yml                                              
aliases: {}

Expected vs actual behavior

gh alias tests do not affect the test executor's configuration file.

Logs

Unsure how to get better logs here as part of the testing suite and being related to non-HTTP behavior.

Activity

  1. andyfeller
    added
    bugSomething isn't working
    on Mar 12, 2025

  2. andyfeller commented on Mar 12, 2025

    @andyfeller
    andyfeller
    on Mar 12, 2025
    ContributorAuthor

    Thanks to @williammartin for calling out pre-existing test helper for setting up an isolated, mocked Config to use; perhaps this is the more appropriate fix for this bug. 🤔

    cli/internal/config/stub.go

    Lines 85 to 114 in 7924274

    // NewIsolatedTestConfig sets up a Mock keyring, creates a blank config
    // overwrites the ghConfig.Read function that returns a singleton config
    // in the real implementation, sets the GH_CONFIG_DIR env var so that
    // any call to Write goes to a different location on disk, and then returns
    // the blank config and a function that reads any data written to disk.
    func NewIsolatedTestConfig(t *testing.T) (*cfg, func(io.Writer, io.Writer)) {
    keyring.MockInit()
    c := ghConfig.ReadFromString("")
    cfg := cfg{c}
    // The real implementation of config.Read uses a sync.Once
    // to read config files and initialise package level variables
    // that are used from then on.
    //
    // This means that tests can't be isolated from each other, so
    // we swap out the function here to return a new config each time.
    ghConfig.Read = func(_ *ghConfig.Config) (*ghConfig.Config, error) {
    return c, nil
    }
    // The config.Write method isn't defined in the same way as Read to allow
    // the function to be swapped out and it does try to write to disk.
    //
    // We should consider whether it makes sense to change that but in the meantime
    // we can use GH_CONFIG_DIR env var to ensure the tests remain isolated.
    readConfigs := StubWriteConfig(t)
    return &cfg, readConfigs
    }

  3. RyanHecht
    assigned
    BagToad
    on Mar 19, 2025
  4. BagToad
    removed their assignment
    on Mar 24, 2025

  5. mxie commented on Mar 26, 2025

    @mxie
    mxie
    on Mar 26, 2025
    Member

    From IPM:

    • impactful work for us as maintainers because it's wiping our local configurations
    • should be quick
  6. mxie
    assigned
    andyfeller
    on Mar 26, 2025
  7. andyfeller
    added a commit that references this issue on Mar 26, 2025

    Fixes #10590

    86ad62a
  8. andyfeller
    closed this as completedin 86ad62aon Mar 26, 2025
  9. andyfeller
    closed this as completedin #10683on Mar 26, 2025

  10. shindere commented on Mar 30, 2025

    @shindere
    shindere
    on Mar 30, 2025 via email

  11. BagToad commented on Apr 30, 2025

    @BagToad
    BagToad
    on Apr 30, 2025
    Member

    👋 @shindere Sorry we missed this comment! I just found it again today.

    IPM is our internal "iteration planning meeting". We will occasionally add some notes to the issues we discuss regarding prioritization, blockers, technical decisions etc.

  12. shindere commented on Apr 30, 2025

    @shindere
    shindere
    on Apr 30, 2025 via email
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

bugSomething isn't working

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions