From 0c51bdeb4491433dfc59a197d0ba2d83a0cd6349 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Mart=C3=AD?= Date: Sun, 24 Nov 2024 20:39:55 +0000 Subject: [PATCH] testscript: phase out `func() int` in RunMain We wanted the user's command functions to return an exit code as an int rather than calling os.Exit directly like a main function so that we could collect coverage profiles from subprocesses. This way, Go tests using testscript would still report the full code coverage information even when using nested processes. This all thankfully went away with Go 1.20, which introduced the same feature but built right into the toolchain for both `go test` and `go build`. As such, we were able to drop all of that code, including the bit that we ran before os.Exit. For more information, see: https://go.dev/blog/integration-test-coverage At this point, testscript users continue to use the `func() int` signature, via e.g. `func main1() int` out of inertia, but there's actually no good reason to keep doing that. It causes extra boilerplate and confuses new testscript users. Moreover, avoiding the use of os.Exit was rather tricky, for example see the former use of flag.ContinueOnExit in our tests. Add a new API, Main, which uses a `func()` signature just like `func main()`, meaning that no second function declaration is needed. Deprecate RunMain in favor of Main as well. --- cmd/testscript/main.go | 31 ++++++++++----------------- cmd/testscript/main_test.go | 7 +++---- cmd/txtar-addmod/addmod.go | 8 ++----- cmd/txtar-addmod/script_test.go | 6 +++--- cmd/txtar-c/savedir.go | 16 +++----------- cmd/txtar-c/script_test.go | 7 +++---- cmd/txtar-x/extract.go | 13 ++++-------- cmd/txtar-x/extract_test.go | 6 +++--- testscript/doc.go | 6 +++--- testscript/exe.go | 37 +++++++++++++++++++++------------ testscript/testscript.go | 2 +- testscript/testscript_test.go | 26 ++++++++++------------- 12 files changed, 71 insertions(+), 94 deletions(-) diff --git a/cmd/testscript/main.go b/cmd/testscript/main.go index 65b340ab..4e6832f2 100644 --- a/cmd/testscript/main.go +++ b/cmd/testscript/main.go @@ -41,37 +41,28 @@ func (e *envVarsFlag) Set(v string) error { } func main() { - os.Exit(main1()) -} - -func main1() int { switch err := mainerr(); err { case nil: - return 0 - case flag.ErrHelp: - return 2 default: fmt.Fprintln(os.Stderr, err) - return 1 + os.Exit(1) } } func mainerr() (retErr error) { - fs := flag.NewFlagSet(os.Args[0], flag.ContinueOnError) - fs.Usage = func() { + flag.Usage = func() { mainUsage(os.Stderr) + os.Exit(2) } var envVars envVarsFlag - fUpdate := fs.Bool("u", false, "update archive file if a cmp fails") - fWork := fs.Bool("work", false, "print temporary work directory and do not remove when done") - fContinue := fs.Bool("continue", false, "continue running the script if an error occurs") - fVerbose := fs.Bool("v", false, "run tests verbosely") - fs.Var(&envVars, "e", "pass through environment variable to script (can appear multiple times)") - if err := fs.Parse(os.Args[1:]); err != nil { - return err - } - - files := fs.Args() + fUpdate := flag.Bool("u", false, "update archive file if a cmp fails") + fWork := flag.Bool("work", false, "print temporary work directory and do not remove when done") + fContinue := flag.Bool("continue", false, "continue running the script if an error occurs") + fVerbose := flag.Bool("v", false, "run tests verbosely") + flag.Var(&envVars, "e", "pass through environment variable to script (can appear multiple times)") + flag.Parse() + + files := flag.Args() if len(files) == 0 { files = []string{"-"} } diff --git a/cmd/testscript/main_test.go b/cmd/testscript/main_test.go index 7bc39028..bb34022e 100644 --- a/cmd/testscript/main_test.go +++ b/cmd/testscript/main_test.go @@ -6,7 +6,6 @@ package main import ( "bytes" - "os" "os/exec" "path/filepath" "runtime" @@ -19,9 +18,9 @@ import ( ) func TestMain(m *testing.M) { - os.Exit(testscript.RunMain(m, map[string]func() int{ - "testscript": main1, - })) + testscript.Main(m, map[string]func(){ + "testscript": main, + }) } func TestScripts(t *testing.T) { diff --git a/cmd/txtar-addmod/addmod.go b/cmd/txtar-addmod/addmod.go index e58514a6..70714e73 100644 --- a/cmd/txtar-addmod/addmod.go +++ b/cmd/txtar-addmod/addmod.go @@ -62,13 +62,9 @@ func fatalf(format string, args ...interface{}) { const goCmd = "go" -func main() { - os.Exit(main1()) -} - var allFiles = flag.Bool("all", false, "include all source files") -func main1() int { +func main() { flag.Usage = usage flag.Parse() if flag.NArg() < 2 { @@ -211,5 +207,5 @@ func main1() int { } } os.RemoveAll(tmpdir) - return exitCode + os.Exit(exitCode) } diff --git a/cmd/txtar-addmod/script_test.go b/cmd/txtar-addmod/script_test.go index b070ef03..e3701bbc 100644 --- a/cmd/txtar-addmod/script_test.go +++ b/cmd/txtar-addmod/script_test.go @@ -17,9 +17,9 @@ import ( var proxyURL string func TestMain(m *testing.M) { - os.Exit(testscript.RunMain(gobinMain{m}, map[string]func() int{ - "txtar-addmod": main1, - })) + testscript.Main(gobinMain{m}, map[string]func(){ + "txtar-addmod": main, + }) } type gobinMain struct { diff --git a/cmd/txtar-c/savedir.go b/cmd/txtar-c/savedir.go index bc2f692b..c21bca93 100644 --- a/cmd/txtar-c/savedir.go +++ b/cmd/txtar-c/savedir.go @@ -14,7 +14,7 @@ package main import ( "bytes" - stdflag "flag" + "flag" "fmt" "log" "os" @@ -25,11 +25,10 @@ import ( "github.com/rogpeppe/go-internal/txtar" ) -var flag = stdflag.NewFlagSet(os.Args[0], stdflag.ContinueOnError) - func usage() { fmt.Fprintf(os.Stderr, "usage: txtar-c dir >saved.txtar\n") flag.PrintDefaults() + os.Exit(2) } var ( @@ -38,17 +37,10 @@ var ( ) func main() { - os.Exit(main1()) -} - -func main1() int { flag.Usage = usage - if flag.Parse(os.Args[1:]) != nil { - return 2 - } + flag.Parse() if flag.NArg() != 1 { usage() - return 2 } log.SetPrefix("txtar-c: ") @@ -111,6 +103,4 @@ func main1() int { data := txtar.Format(a) os.Stdout.Write(data) - - return 0 } diff --git a/cmd/txtar-c/script_test.go b/cmd/txtar-c/script_test.go index 94e77328..9760eb92 100644 --- a/cmd/txtar-c/script_test.go +++ b/cmd/txtar-c/script_test.go @@ -5,16 +5,15 @@ package main import ( - "os" "testing" "github.com/rogpeppe/go-internal/testscript" ) func TestMain(m *testing.M) { - os.Exit(testscript.RunMain(m, map[string]func() int{ - "txtar-c": main1, - })) + testscript.Main(m, map[string]func(){ + "txtar-c": main, + }) } func TestScripts(t *testing.T) { diff --git a/cmd/txtar-x/extract.go b/cmd/txtar-x/extract.go index 713026e7..289c0039 100644 --- a/cmd/txtar-x/extract.go +++ b/cmd/txtar-x/extract.go @@ -29,18 +29,14 @@ var ( func usage() { fmt.Fprintf(os.Stderr, "usage: txtar-x [flags] [file]\n") flag.PrintDefaults() + os.Exit(2) } func main() { - os.Exit(main1()) -} - -func main1() int { flag.Usage = usage flag.Parse() if flag.NArg() > 1 { usage() - return 2 } log.SetPrefix("txtar-x: ") log.SetFlags(0) @@ -50,20 +46,19 @@ func main1() int { data, err := io.ReadAll(os.Stdin) if err != nil { log.Printf("cannot read stdin: %v", err) - return 1 + os.Exit(1) } a = txtar.Parse(data) } else { a1, err := txtar.ParseFile(flag.Arg(0)) if err != nil { log.Print(err) - return 1 + os.Exit(1) } a = a1 } if err := txtar.Write(a, *extractDir); err != nil { log.Print(err) - return 1 + os.Exit(1) } - return 0 } diff --git a/cmd/txtar-x/extract_test.go b/cmd/txtar-x/extract_test.go index 97e8e7e3..2920bbd6 100644 --- a/cmd/txtar-x/extract_test.go +++ b/cmd/txtar-x/extract_test.go @@ -13,9 +13,9 @@ import ( ) func TestMain(m *testing.M) { - os.Exit(testscript.RunMain(m, map[string]func() int{ - "txtar-x": main1, - })) + testscript.Main(m, map[string]func(){ + "txtar-x": main, + }) } func TestScripts(t *testing.T) { diff --git a/testscript/doc.go b/testscript/doc.go index 8ccbb443..d05ce00b 100644 --- a/testscript/doc.go +++ b/testscript/doc.go @@ -24,14 +24,14 @@ To run a specific script foo.txtar or foo.txt, run where TestName is the name of the test that Run is called from. To define an executable command (or several) that can be run as part of the script, -call RunMain with the functions that implement the command's functionality. +call Main with the functions that implement the command's functionality. The command functions will be called in a separate process, so are free to mutate global variables without polluting the top level test binary. func TestMain(m *testing.M) { - os.Exit(testscript.RunMain(m, map[string] func() int{ + testscript.Main(m, map[string] func() { "testscript": testscriptMain, - })) + }) } In general script files should have short names: a few words, not whole sentences. diff --git a/testscript/exe.go b/testscript/exe.go index 9f9fdc14..108148cb 100644 --- a/testscript/exe.go +++ b/testscript/exe.go @@ -23,13 +23,11 @@ type TestingM interface { // Deprecated: this option is no longer used. func IgnoreMissedCoverage() {} -// RunMain should be called within a TestMain function to allow +// Main should be called within a TestMain function to allow // subcommands to be run in the testscript context. // // The commands map holds the set of command names, each -// with an associated run function which should return the -// code to pass to os.Exit. It's OK for a command function to -// exit itself, but this may result in loss of coverage information. +// with an associated run function which may call os.Exit. // // When Run is called, these commands are installed as regular commands in the shell // path, so can be invoked with "exec" or via any other command (for example a shell script). @@ -38,9 +36,7 @@ func IgnoreMissedCoverage() {} // without "exec" - that is, "foo" will behave like "exec foo". // This can be disabled with Params.RequireExplicitExec to keep consistency // across test scripts, and to keep separate process executions explicit. -// -// This function returns an exit code to pass to os.Exit, after calling m.Run. -func RunMain(m TestingM, commands map[string]func() int) (exitCode int) { +func Main(m TestingM, commands map[string]func()) { // Depending on os.Args[0], this is either the top-level execution of // the test binary by "go test", or the execution of one of the provided // commands via "foo" or "exec foo". @@ -58,18 +54,18 @@ func RunMain(m TestingM, commands map[string]func() int) (exitCode int) { tmpdir, err := os.MkdirTemp("", "testscript-main") if err != nil { log.Printf("could not set up temporary directory: %v", err) - return 2 + os.Exit(2) } defer func() { if err := os.RemoveAll(tmpdir); err != nil { log.Printf("cannot delete temporary directory: %v", err) - exitCode = 2 + os.Exit(2) } }() bindir := filepath.Join(tmpdir, "bin") if err := os.MkdirAll(bindir, 0o777); err != nil { log.Printf("could not set up PATH binary directory: %v", err) - return 2 + os.Exit(2) } os.Setenv("PATH", bindir+string(filepath.ListSeparator)+os.Getenv("PATH")) @@ -86,7 +82,7 @@ func RunMain(m TestingM, commands map[string]func() int) (exitCode int) { } if err != nil { log.Printf("could not set up %s in $PATH: %v", name, err) - return 2 + os.Exit(2) } scriptCmds[name] = func(ts *TestScript, neg bool, args []string) { if ts.params.RequireExplicitExec { @@ -95,11 +91,26 @@ func RunMain(m TestingM, commands map[string]func() int) (exitCode int) { ts.cmdExec(neg, append([]string{name}, args...)) } } - return m.Run() + os.Exit(m.Run()) } // The command being registered is being invoked, so run it, then exit. os.Args[0] = cmdName - return mainf() + mainf() + os.Exit(0) +} + +// Deprecated: use [Main], as the only reason for returning exit codes +// was to collect full code coverage, which Go does automatically now: +// https://go.dev/blog/integration-test-coverage +func RunMain(m TestingM, commands map[string]func() int) (exitCode int) { + commands2 := make(map[string]func(), len(commands)) + for name, fn := range commands { + commands2[name] = func() { os.Exit(fn()) } + } + Main(m, commands2) + // Main always calls os.Exit; we assume that all users of RunMain would have simply + // called os.Exit with the returned exitCode as well, following the documentation. + panic("unreachable") } // copyBinary makes a copy of a binary to a new location. It is used as part of diff --git a/testscript/testscript.go b/testscript/testscript.go index e4024837..cc72cc9a 100644 --- a/testscript/testscript.go +++ b/testscript/testscript.go @@ -181,7 +181,7 @@ type Params struct { // script. UpdateScripts bool - // RequireExplicitExec requires that commands passed to RunMain must be used + // RequireExplicitExec requires that commands passed to [Main] must be used // in test scripts via `exec cmd` and not simply `cmd`. This can help keep // consistency across test scripts as well as keep separate process // executions explicit. diff --git a/testscript/testscript_test.go b/testscript/testscript_test.go index 9cbab8fa..f5190857 100644 --- a/testscript/testscript_test.go +++ b/testscript/testscript_test.go @@ -21,12 +21,11 @@ import ( "time" ) -func printArgs() int { +func printArgs() { fmt.Printf("%q\n", os.Args) - return 0 } -func fprintArgs() int { +func fprintArgs() { s := strings.Join(os.Args[2:], " ") switch os.Args[1] { case "stdout": @@ -34,15 +33,14 @@ func fprintArgs() int { case "stderr": fmt.Fprintln(os.Stderr, s) } - return 0 } -func exitWithStatus() int { +func exitWithStatus() { n, _ := strconv.Atoi(os.Args[1]) - return n + os.Exit(n) } -func signalCatcher() int { +func signalCatcher() { // Note: won't work under Windows. c := make(chan os.Signal, 1) signal.Notify(c, os.Interrupt) @@ -50,27 +48,25 @@ func signalCatcher() int { // we will catch the signal. if err := os.WriteFile("catchsignal", nil, 0o666); err != nil { fmt.Println(err) - return 1 + os.Exit(1) } <-c fmt.Println("caught interrupt") - return 0 } -func terminalPrompt() int { +func terminalPrompt() { tty, err := os.OpenFile("/dev/tty", os.O_RDWR, 0) if err != nil { fmt.Println(err) - return 1 + os.Exit(1) } tty.WriteString("The magic words are: ") var words string fmt.Fscanln(tty, &words) if words != "SQUEAMISHOSSIFRAGE" { fmt.Println(words) - return 42 + os.Exit(42) } - return 0 } func TestMain(m *testing.M) { @@ -79,13 +75,13 @@ func TestMain(m *testing.M) { } showVerboseEnv = false - os.Exit(RunMain(m, map[string]func() int{ + Main(m, map[string]func(){ "printargs": printArgs, "fprintargs": fprintArgs, "status": exitWithStatus, "signalcatcher": signalCatcher, "terminalprompt": terminalPrompt, - })) + }) } func TestCRLFInput(t *testing.T) {