From d92b4cd0935fcb7be3fb30253426988aada78d32 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Sat, 5 Aug 2023 23:36:45 +0800 Subject: [PATCH] Fix incorrect CLI exit code and duplicate error message (#26346) Follow the CLI refactoring, and add tests. --- cmd/main.go | 22 +++++++++++- cmd/main_test.go | 79 +++++++++++++++++++++++++++++++++++-------- main.go | 18 ++++------ modules/test/utils.go | 6 ++++ 4 files changed, 98 insertions(+), 27 deletions(-) diff --git a/cmd/main.go b/cmd/main.go index 13f9bba01..feda41e68 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -6,6 +6,7 @@ package cmd import ( "fmt" "os" + "strings" "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/setting" @@ -117,8 +118,12 @@ func prepareWorkPathAndCustomConf(action cli.ActionFunc) func(ctx *cli.Context) } } -func NewMainApp() *cli.App { +func NewMainApp(version, versionExtra string) *cli.App { app := cli.NewApp() + app.Name = "Gitea" + app.Usage = "A painless self-hosted Git service" + app.Description = `By default, Gitea will start serving using the web-server with no argument, which can alternatively be run by running the subcommand "web".` + app.Version = version + versionExtra app.EnableBashCompletion = true // these sub-commands need to use config file @@ -166,3 +171,18 @@ func NewMainApp() *cli.App { return app } + +func RunMainApp(app *cli.App, args ...string) error { + err := app.Run(args) + if err == nil { + return nil + } + if strings.HasPrefix(err.Error(), "flag provided but not defined:") { + // the cli package should already have output the error message, so just exit + cli.OsExiter(1) + return err + } + _, _ = fmt.Fprintf(app.ErrWriter, "Command error: %v\n", err) + cli.OsExiter(1) + return err +} diff --git a/cmd/main_test.go b/cmd/main_test.go index e9204d09c..0e32dce56 100644 --- a/cmd/main_test.go +++ b/cmd/main_test.go @@ -5,6 +5,7 @@ package cmd import ( "fmt" + "io" "os" "path/filepath" "strings" @@ -12,6 +13,7 @@ import ( "code.gitea.io/gitea/models/unittest" "code.gitea.io/gitea/modules/setting" + "code.gitea.io/gitea/modules/test" "github.com/stretchr/testify/assert" "github.com/urfave/cli/v2" @@ -27,21 +29,38 @@ func makePathOutput(workPath, customPath, customConf string) string { return fmt.Sprintf("WorkPath=%s\nCustomPath=%s\nCustomConf=%s", workPath, customPath, customConf) } -func newTestApp() *cli.App { - app := NewMainApp() - testCmd := &cli.Command{ - Name: "test-cmd", - Action: func(ctx *cli.Context) error { - _, _ = fmt.Fprint(app.Writer, makePathOutput(setting.AppWorkPath, setting.CustomPath, setting.CustomConf)) - return nil - }, - } +func newTestApp(testCmdAction func(ctx *cli.Context) error) *cli.App { + app := NewMainApp("version", "version-extra") + testCmd := &cli.Command{Name: "test-cmd", Action: testCmdAction} prepareSubcommandWithConfig(testCmd, appGlobalFlags()) app.Commands = append(app.Commands, testCmd) app.DefaultCommand = testCmd.Name return app } +type runResult struct { + Stdout string + Stderr string + ExitCode int +} + +func runTestApp(app *cli.App, args ...string) (runResult, error) { + outBuf := new(strings.Builder) + errBuf := new(strings.Builder) + app.Writer = outBuf + app.ErrWriter = errBuf + exitCode := -1 + defer test.MockVariableValue(&cli.ErrWriter, app.ErrWriter)() + defer test.MockVariableValue(&cli.OsExiter, func(code int) { + if exitCode == -1 { + exitCode = code // save the exit code once and then reset the writer (to simulate the exit) + app.Writer, app.ErrWriter, cli.ErrWriter = io.Discard, io.Discard, io.Discard + } + })() + err := RunMainApp(app, args...) + return runResult{outBuf.String(), errBuf.String(), exitCode}, err +} + func TestCliCmd(t *testing.T) { defaultWorkPath := filepath.Dir(setting.AppPath) defaultCustomPath := filepath.Join(defaultWorkPath, "custom") @@ -92,7 +111,10 @@ func TestCliCmd(t *testing.T) { }, } - app := newTestApp() + app := newTestApp(func(ctx *cli.Context) error { + _, _ = fmt.Fprint(ctx.App.Writer, makePathOutput(setting.AppWorkPath, setting.CustomPath, setting.CustomConf)) + return nil + }) var envBackup []string for _, s := range os.Environ() { if strings.HasPrefix(s, "GITEA_") && strings.Contains(s, "=") { @@ -120,12 +142,39 @@ func TestCliCmd(t *testing.T) { _ = os.Setenv(k, v) } args := strings.Split(c.cmd, " ") // for test only, "split" is good enough - out := new(strings.Builder) - app.Writer = out - err := app.Run(args) + r, err := runTestApp(app, args...) assert.NoError(t, err, c.cmd) assert.NotEmpty(t, c.exp, c.cmd) - outStr := out.String() - assert.Contains(t, outStr, c.exp, c.cmd) + assert.Contains(t, r.Stdout, c.exp, c.cmd) } } + +func TestCliCmdError(t *testing.T) { + app := newTestApp(func(ctx *cli.Context) error { return fmt.Errorf("normal error") }) + r, err := runTestApp(app, "./gitea", "test-cmd") + assert.Error(t, err) + assert.Equal(t, 1, r.ExitCode) + assert.Equal(t, "", r.Stdout) + assert.Equal(t, "Command error: normal error\n", r.Stderr) + + app = newTestApp(func(ctx *cli.Context) error { return cli.Exit("exit error", 2) }) + r, err = runTestApp(app, "./gitea", "test-cmd") + assert.Error(t, err) + assert.Equal(t, 2, r.ExitCode) + assert.Equal(t, "", r.Stdout) + assert.Equal(t, "exit error\n", r.Stderr) + + app = newTestApp(func(ctx *cli.Context) error { return nil }) + r, err = runTestApp(app, "./gitea", "test-cmd", "--no-such") + assert.Error(t, err) + assert.Equal(t, 1, r.ExitCode) + assert.Equal(t, "Incorrect Usage: flag provided but not defined: -no-such\n\n", r.Stdout) + assert.Equal(t, "", r.Stderr) // the cli package's strange behavior, the error message is not in stderr .... + + app = newTestApp(func(ctx *cli.Context) error { return nil }) + r, err = runTestApp(app, "./gitea", "test-cmd") + assert.NoError(t, err) + assert.Equal(t, -1, r.ExitCode) // the cli.OsExiter is not called + assert.Equal(t, "", r.Stdout) + assert.Equal(t, "", r.Stderr) +} diff --git a/main.go b/main.go index 652a71195..775c729c5 100644 --- a/main.go +++ b/main.go @@ -5,7 +5,6 @@ package main import ( - "fmt" "os" "runtime" "strings" @@ -21,6 +20,8 @@ import ( _ "code.gitea.io/gitea/modules/markup/csv" _ "code.gitea.io/gitea/modules/markup/markdown" _ "code.gitea.io/gitea/modules/markup/orgmode" + + "github.com/urfave/cli/v2" ) // these flags will be set by the build flags @@ -37,17 +38,12 @@ func init() { } func main() { - app := cmd.NewMainApp() - app.Name = "Gitea" - app.Usage = "A painless self-hosted Git service" - app.Description = `By default, Gitea will start serving using the web-server with no argument, which can alternatively be run by running the subcommand "web".` - app.Version = Version + formatBuiltWith() - - err := app.Run(os.Args) - if err != nil { - _, _ = fmt.Fprintf(app.Writer, "\nFailed to run with %s: %v\n", os.Args, err) + cli.OsExiter = func(code int) { + log.GetManager().Close() + os.Exit(code) } - + app := cmd.NewMainApp(Version, formatBuiltWith()) + _ = cmd.RunMainApp(app, os.Args...) // all errors should have been handled by the RunMainApp log.GetManager().Close() } diff --git a/modules/test/utils.go b/modules/test/utils.go index 2917741c4..4a0c2f1b3 100644 --- a/modules/test/utils.go +++ b/modules/test/utils.go @@ -33,3 +33,9 @@ func RedirectURL(resp http.ResponseWriter) string { func IsNormalPageCompleted(s string) bool { return strings.Contains(s, `