testscript: phase out func() int in RunMain

mvdan · mvdan · commit a5dc8ff20d0a · 2024-12-26T12:14:12.000Z
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.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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/gotooltest/testdata/cover.txt b/gotooltest/testdata/cover.txt
@@ -51,6 +51,8 @@ import (
 )
 
 func TestMain(m *testing.M) {
+	// Note that here we still use RunMain, which is the older deprecated API,
+	// to sanity check that it works OK.
 	os.Exit(testscript.RunMain(m, map[string] func() int{
 		"foo": foo1,
 	}))
diff --git 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
@@ -23,13 +23,12 @@ 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.
+// Main always calls [os.Exit], so it does not return back to the caller.
 //
 // 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 +37,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".
@@ -57,19 +54,16 @@ func RunMain(m TestingM, commands map[string]func() int) (exitCode int) {
 		// Set up all commands in a directory, added in $PATH.
 		tmpdir, err := os.MkdirTemp("", "testscript-main")
 		if err != nil {
-			log.Printf("could not set up temporary directory: %v", err)
-			return 2
+			log.Fatalf("could not set up temporary directory: %v", err)
 		}
 		defer func() {
 			if err := os.RemoveAll(tmpdir); err != nil {
-				log.Printf("cannot delete temporary directory: %v", err)
-				exitCode = 2
+				log.Fatalf("cannot delete temporary directory: %v", err)
 			}
 		}()
 		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
+			log.Fatalf("could not set up PATH binary directory: %v", err)
 		}
 		os.Setenv("PATH", bindir+string(filepath.ListSeparator)+os.Getenv("PATH"))
 
@@ -85,8 +79,7 @@ func RunMain(m TestingM, commands map[string]func() int) (exitCode int) {
 				err = copyBinary(binpath, binfile)
 			}
 			if err != nil {
-				log.Printf("could not set up %s in $PATH: %v", name, err)
-				return 2
+				log.Fatalf("could not set up %s in $PATH: %v", name, err)
 			}
 			scriptCmds[name] = func(ts *TestScript, neg bool, args []string) {
 				if ts.params.RequireExplicitExec {
@@ -95,11 +88,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
@@ -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

Original file line number	Diff line number	Diff line change
`@@ -62,13 +62,9 @@ func fatalf(format string, args ...interface{}) {`
`62`	`62`
`63`	`63`	`const goCmd = "go"`
`64`	`64`
`65`		`-func main() {`
`66`		`- os.Exit(main1())`
`67`		`-}`
`68`		`-`
`69`	`65`	`var allFiles = flag.Bool("all", false, "include all source files")`
`70`	`66`
`71`		`-func main1() int {`
	`67`	`+func main() {`
`72`	`68`	`flag.Usage = usage`
`73`	`69`	`flag.Parse()`
`74`	`70`	`if flag.NArg() < 2 {`
`@@ -211,5 +207,5 @@ func main1() int {`
`211`	`207`	`}`
`212`	`208`	`}`
`213`	`209`	`os.RemoveAll(tmpdir)`
`214`		`- return exitCode`
	`210`	`+ os.Exit(exitCode)`
`215`	`211`	`}`
Original file line number	Diff line number	Diff line change
`@@ -13,9 +13,9 @@ import (`
`13`	`13`	`)`
`14`	`14`
`15`	`15`	`func TestMain(m *testing.M) {`
`16`		`- os.Exit(testscript.RunMain(m, map[string]func() int{`
`17`		`- "txtar-x": main1,`
`18`		`- }))`
	`16`	`+ testscript.Main(m, map[string]func(){`
	`17`	`+ "txtar-x": main,`
	`18`	`+ })`
`19`	`19`	`}`
`20`	`20`
`21`	`21`	`func TestScripts(t *testing.T) {`
Original file line number	Diff line number	Diff line change
`@@ -51,6 +51,8 @@ import (`
`51`	`51`	`)`
`52`	`52`
`53`	`53`	`func TestMain(m *testing.M) {`
	`54`	`+ // Note that here we still use RunMain, which is the older deprecated API,`
	`55`	`+ // to sanity check that it works OK.`
`54`	`56`	`os.Exit(testscript.RunMain(m, map[string] func() int{`
`55`	`57`	`"foo": foo1,`
`56`	`58`	`}))`