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
@andyfeller

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.

Metadata

Metadata

Assignees

Labels

bugSomething isn't working

Type

No type

Fields

No fields configured for issues without a type.

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions