From 2d1bbcff01756cfd8fc1033d2b0a02139f91865a Mon Sep 17 00:00:00 2001 From: John Starich Date: Mon, 13 Feb 2023 23:31:14 -0600 Subject: [PATCH] Add functional tests, fix unset exit codes on panics (#42) * Refactor to support functional tests * Fix unset exit code from uncaught exception * Fix unset exit code for panic in next tick of event loop * Fix data race on shadowed "err" * Poke CI * Increase test timeout for Windows race detector to complete * Move test types and helpers to the bottom * Refactor into table-driven tests * Remove single-use helper * Handle errors with testing.T in testParse helper * Return errors instead of exit codes * Extract HTTP server, use ctx for startup/shutdown * Increase test timeout for Windows again * Handle testParseOther error as assertion failure * Rename shutdownNoWait to startShutdown for clarity --- .github/workflows/ci.yml | 2 +- http_server.go | 51 ++++++++++ index.html | 7 +- main.go | 102 ++++++++----------- main_test.go | 212 +++++++++++++++++++++++++++++++++++++++ parse.go | 11 +- parse_test.go | 46 +++++---- 7 files changed, 342 insertions(+), 89 deletions(-) create mode 100644 http_server.go create mode 100644 main_test.go diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 02ae1c3..e64c78b 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -21,6 +21,6 @@ jobs: - name: Install cleanenv run: go install ./cmd/cleanenv - name: Test - run: cleanenv -remove-prefix GITHUB_ -remove-prefix JAVA_ -- go test -v -race ./... + run: cleanenv -remove-prefix GITHUB_ -remove-prefix JAVA_ -- go test -v -race -timeout 5m ./... - name: Install run: go install diff --git a/http_server.go b/http_server.go new file mode 100644 index 0000000..c9f6602 --- /dev/null +++ b/http_server.go @@ -0,0 +1,51 @@ +package main + +import ( + "context" + "log" + "net" + "net/http" + neturl "net/url" + "time" +) + +func startHTTPServer(ctx context.Context, handler http.Handler, logger *log.Logger) (url string, shutdown context.CancelFunc, err error) { + // Need to generate a random port every time for tests in parallel to run. + l, err := net.Listen("tcp", "localhost:") + if err != nil { + return "", nil, err + } + + server := &http.Server{ + Handler: handler, + } + go func() { // serves HTTP + err := server.Serve(l) + if err != http.ErrServerClosed { + logger.Println(err) + } + }() + + shutdownCtx, startShutdown := context.WithCancel(ctx) + shutdownComplete := make(chan struct{}, 1) + go func() { // waits for canceled ctx or triggered shutdown, then shuts down HTTP + <-shutdownCtx.Done() + shutdownTimeoutCtx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + err := server.Shutdown(shutdownTimeoutCtx) + if err != nil { + logger.Println(err) + } + shutdownComplete <- struct{}{} + }() + + shutdown = func() { + startShutdown() + <-shutdownComplete + } + url = (&neturl.URL{ + Scheme: "http", + Host: l.Addr().String(), + }).String() + return url, shutdown, nil +} diff --git a/index.html b/index.html index 38cadcc..f2d7834 100644 --- a/index.html +++ b/index.html @@ -89,7 +89,12 @@ }).catch((err) => { console.error(err); }); - await go.run(inst); + try { + await go.run(inst); + } catch(e) { + exitCode = 1 + console.error(e) + } document.getElementById("doneButton").disabled = false; })(); diff --git a/main.go b/main.go index 71b3f54..fb1992d 100644 --- a/main.go +++ b/main.go @@ -3,19 +3,18 @@ package main import ( "bytes" "context" + "errors" "flag" "fmt" "io" "io/ioutil" "log" - "net" - "net/http" "os" + "os/signal" "path" "runtime" "strconv" "strings" - "time" "github.com/chromedp/cdproto/inspector" "github.com/chromedp/cdproto/profiler" @@ -24,71 +23,64 @@ import ( "github.com/chromedp/chromedp" ) -var ( - cpuProfile *string - coverageProfile *string -) - func main() { - // NOTE: Since `os.Exit` will cause the process to exit, this defer - // must be at the bottom of the defer stack to allow all other defer calls to - // be called first. - exitCode := 0 + ctx, cancel := signal.NotifyContext(context.Background(), os.Interrupt) + defer cancel() + err := run(ctx, os.Args, os.Stderr, flag.CommandLine) + if err != nil { + fmt.Fprintln(os.Stderr, err) + os.Exit(1) + } +} + +func run(ctx context.Context, args []string, errOutput io.Writer, flagSet *flag.FlagSet) (returnedErr error) { + logger := log.New(errOutput, "[wasmbrowsertest]: ", log.LstdFlags|log.Lshortfile) defer func() { - if exitCode != 0 { - os.Exit(exitCode) + r := recover() + if r != nil { + returnedErr = fmt.Errorf("Panicked: %+v", r) } }() - logger := log.New(os.Stderr, "[wasmbrowsertest]: ", log.LstdFlags|log.Lshortfile) - if len(os.Args) < 2 { - logger.Fatal("Please pass a wasm file as a parameter") + if len(args) < 2 { + return errors.New("Please pass a wasm file as a parameter") } - cpuProfile = flag.String("test.cpuprofile", "", "") - coverageProfile = flag.String("test.coverprofile", "", "") + cpuProfile := flagSet.String("test.cpuprofile", "", "") + coverageProfile := flagSet.String("test.coverprofile", "", "") - wasmFile := os.Args[1] + wasmFile := args[1] ext := path.Ext(wasmFile) // net/http code does not take js/wasm path if it is a .test binary. if ext == ".test" { wasmFile = strings.Replace(wasmFile, ext, ".wasm", -1) - err := copyFile(os.Args[1], wasmFile) + err := copyFile(args[1], wasmFile) if err != nil { - logger.Fatal(err) + return err } defer os.Remove(wasmFile) - os.Args[1] = wasmFile + args[1] = wasmFile } - passon := gentleParse(flag.CommandLine, os.Args[2:]) + passon, err := gentleParse(flagSet, args[2:]) + if err != nil { + return err + } passon = append([]string{wasmFile}, passon...) if *coverageProfile != "" { passon = append(passon, "-test.coverprofile="+*coverageProfile) } - // Need to generate a random port every time for tests in parallel to run. - l, err := net.Listen("tcp", "localhost:") - if err != nil { - logger.Fatal(err) - } - tcpL, ok := l.(*net.TCPListener) - if !ok { - logger.Fatal("net.Listen did not return a TCPListener") - } - _, port, err := net.SplitHostPort(tcpL.Addr().String()) - if err != nil { - logger.Fatal(err) - } - // Setup web server. handler, err := NewWASMServer(wasmFile, passon, *coverageProfile, logger) if err != nil { - logger.Fatal(err) + return err } - httpServer := &http.Server{ - Handler: handler, + url, shutdownHTTPServer, err := startHTTPServer(ctx, handler, logger) + if err != nil { + return err } + defer shutdownHTTPServer() opts := chromedp.DefaultExecAllocatorOptions[:] if os.Getenv("WASM_HEADLESS") == "off" { @@ -105,7 +97,7 @@ func main() { } // create chrome instance - allocCtx, cancelAllocCtx := chromedp.NewExecAllocator(context.Background(), opts...) + allocCtx, cancelAllocCtx := chromedp.NewExecAllocator(ctx, opts...) defer cancelAllocCtx() ctx, cancelCtx := chromedp.NewContext(allocCtx) defer cancelCtx() @@ -114,17 +106,10 @@ func main() { handleEvent(ctx, ev, logger) }) - done := make(chan struct{}) - go func() { - err = httpServer.Serve(l) - if err != http.ErrServerClosed { - logger.Println(err) - } - done <- struct{}{} - }() + var exitCode int var coverageProfileContents string tasks := []chromedp.Action{ - chromedp.Navigate(`http://localhost:` + port), + chromedp.Navigate(url), chromedp.WaitEnabled(`#doneButton`), chromedp.Evaluate(`exitCode;`, &exitCode), chromedp.Evaluate(`coverageProfileContents;`, &coverageProfileContents), @@ -167,20 +152,13 @@ func main() { err = chromedp.Run(ctx, tasks...) if err != nil { - logger.Println(err) + // Browser did not exit cleanly. Likely failed with an uncaught error. + return err } if exitCode != 0 { - exitCode = 1 + return fmt.Errorf("exit with status %d", exitCode) } - // create a timeout - ctx, cancelHTTPCtx := context.WithTimeout(ctx, 5*time.Second) - defer cancelHTTPCtx() - // Close shop - err = httpServer.Shutdown(ctx) - if err != nil { - logger.Println(err) - } - <-done + return nil } func copyFile(src, dst string) error { diff --git a/main_test.go b/main_test.go new file mode 100644 index 0000000..9cf4acf --- /dev/null +++ b/main_test.go @@ -0,0 +1,212 @@ +package main + +import ( + "bytes" + "context" + "flag" + "io" + "os" + "os/exec" + "path/filepath" + "testing" +) + +func TestRun(t *testing.T) { + t.Parallel() + + for _, tc := range []struct { + description string + files map[string]string + args []string + expectErr string + }{ + { + description: "pass", + files: map[string]string{ + "go.mod": ` +module foo +`, + "foo_test.go": ` +package foo + +import "testing" + +func TestFoo(t *testing.T) { + if false { + t.Errorf("foo failed") + } +} +`, + }, + }, + { + description: "fails", + files: map[string]string{ + "go.mod": ` +module foo +`, + "foo_test.go": ` +package foo + +import "testing" + +func TestFooFails(t *testing.T) { + t.Errorf("foo failed") +} +`, + }, + expectErr: "exit with status 1", + }, + { + description: "panic fails", + files: map[string]string{ + "go.mod": ` +module foo +`, + "foo_test.go": ` +package foo + +import "testing" + +func TestFooPanic(t *testing.T) { + panic("failed") +} +`, + }, + expectErr: "exit with status 2", + }, + { + description: "panic in goroutine fails", + files: map[string]string{ + "go.mod": ` +module foo +`, + "foo_test.go": ` +package foo + +import "testing" + +func TestFooGoroutinePanic(t *testing.T) { + go panic("foo failed") +} +`, + }, + expectErr: "exit with status 1", + }, + { + description: "panic in next run of event loop fails", + files: map[string]string{ + "go.mod": ` +module foo +`, + "foo_test.go": ` +package foo + +import ( + "syscall/js" + "testing" +) + +func TestFooNextEventLoopPanic(t *testing.T) { + js.Global().Call("setTimeout", js.FuncOf(func(js.Value, []js.Value) interface{} { + panic("bad") + return nil + }), 0) +} +`, + }, + expectErr: "context canceled", + }, + } { + tc := tc // enable parallel sub-tests + t.Run(tc.description, func(t *testing.T) { + t.Parallel() + dir := t.TempDir() + for fileName, contents := range tc.files { + writeFile(t, dir, fileName, contents) + } + wasmFile := buildTestWasm(t, dir) + _, err := testRun(t, wasmFile, tc.args...) + assertEqualError(t, tc.expectErr, err) + }) + } +} + +type testWriter struct { + testingT *testing.T +} + +func testLogger(t *testing.T) io.Writer { + return &testWriter{t} +} + +func (w *testWriter) Write(b []byte) (int, error) { + w.testingT.Helper() + w.testingT.Log(string(b)) + return len(b), nil +} + +func testRun(t *testing.T, wasmFile string, flags ...string) ([]byte, error) { + var logs bytes.Buffer + output := io.MultiWriter(testLogger(t), &logs) + flagSet := flag.NewFlagSet("wasmbrowsertest", flag.ContinueOnError) + ctx, cancel := context.WithCancel(context.Background()) + t.Cleanup(cancel) + + err := run(ctx, append([]string{"go_js_wasm_exec", wasmFile, "-test.v"}, flags...), output, flagSet) + return logs.Bytes(), err +} + +// writeFile creates a file at $baseDir/$path with the given contents, where 'path' is slash separated +func writeFile(t *testing.T, baseDir, path, contents string) { + t.Helper() + path = filepath.FromSlash(path) + fullPath := filepath.Join(baseDir, path) + err := os.MkdirAll(filepath.Dir(fullPath), 0755) + if err != nil { + t.Fatal("Failed to create file's base directory:", err) + } + err = os.WriteFile(fullPath, []byte(contents), 0600) + if err != nil { + t.Fatal("Failed to create file:", err) + } +} + +// buildTestWasm builds the given Go package's test binary and returns the output Wasm file +func buildTestWasm(t *testing.T, path string) string { + t.Helper() + outputFile := filepath.Join(t.TempDir(), "out.wasm") + cmd := exec.Command("go", "test", "-c", "-o", outputFile, ".") + cmd.Dir = path + cmd.Env = append(os.Environ(), + "GOOS=js", + "GOARCH=wasm", + ) + output, err := cmd.CombinedOutput() + if len(output) > 0 { + t.Log(string(output)) + } + if err != nil { + t.Fatal("Failed to build Wasm binary:", err) + } + return outputFile +} + +func assertEqualError(t *testing.T, expected string, err error) { + t.Helper() + if expected == "" { + if err != nil { + t.Error("Unexpected error:", err) + } + return + } + + if err == nil { + t.Error("Expected error, got nil") + return + } + message := err.Error() + if expected != message { + t.Errorf("Unexpected error message: %q != %q", expected, message) + } +} diff --git a/parse.go b/parse.go index 20dfaf9..93c3dc7 100644 --- a/parse.go +++ b/parse.go @@ -4,16 +4,15 @@ import ( "flag" "fmt" "io/ioutil" - "os" "strings" ) // gentleParse takes a flag.FlagSet, calls Parse to get its flags parsed, // and collects the arguments the FlagSet does not recognize, returning // the collected list. -func gentleParse(flagset *flag.FlagSet, args []string) []string { +func gentleParse(flagset *flag.FlagSet, args []string) ([]string, error) { if len(args) == 0 { - return nil + return nil, nil } const prefix = "flag provided but not defined: " @@ -48,7 +47,7 @@ func gentleParse(flagset *flag.FlagSet, args []string) []string { fmt.Fprintf(w, "%s\n", err) flagset.SetOutput(w) flagset.Usage() - os.Exit(1) + return nil, err } // Check if the call to flagset.Parse ate a "--". If so, we're done @@ -58,10 +57,10 @@ func gentleParse(flagset *flag.FlagSet, args []string) []string { lastabsorbed := next[lastabsorbedpos] if lastabsorbed == "--" { r = append(r, "--") // return the "--" too. - return append(r, flagset.Args()...) + return append(r, flagset.Args()...), nil } } next = flagset.Args() } - return r + return r, nil } diff --git a/parse_test.go b/parse_test.go index f35f0ac..e9c243f 100644 --- a/parse_test.go +++ b/parse_test.go @@ -16,7 +16,7 @@ import ( func TestParse(t *testing.T) { t.Run("Empty", func(t *testing.T) { // Empty in, empty out. - testParse().Expect(t, + testParse(t).Expect(t, `cpuProfile: ""`, `passon : []`, ) @@ -24,14 +24,14 @@ func TestParse(t *testing.T) { t.Run("Other", func(t *testing.T) { // Empty in, empty out, with an extra `other` variable that has a default. - testParseOther().Expect(t, + testParseOther(t).Expect(t, `cpuProfile: ""`, `other : "default-other-value"`, `passon : []`, ) // Test parsing of a custom flag with a custom value - testParseOther("-test.v", "-test.cpuprofile", "cpu1.out", "-other=another").Expect(t, + testParseOther(t, "-test.v", "-test.cpuprofile", "cpu1.out", "-other=another").Expect(t, `cpuProfile: "cpu1.out"`, `other : "another"`, `passon : ["-test.v"]`, @@ -40,7 +40,7 @@ func TestParse(t *testing.T) { t.Run("Verbose", func(t *testing.T) { // One unrecognized in, same out. - testParse("-test.v").Expect(t, + testParse(t, "-test.v").Expect(t, `cpuProfile: ""`, `passon : ["-test.v"]`, ) @@ -48,7 +48,7 @@ func TestParse(t *testing.T) { t.Run("CPU1", func(t *testing.T) { // One unrecognized followed by ours. - testParse("-test.v", "-test.cpuprofile", "cpu1.out").Expect(t, + testParse(t, "-test.v", "-test.cpuprofile", "cpu1.out").Expect(t, `cpuProfile: "cpu1.out"`, `passon : ["-test.v"]`, ) @@ -56,7 +56,7 @@ func TestParse(t *testing.T) { t.Run("CPU2", func(t *testing.T) { // Ours followed by one unrecognized. - testParse("-test.cpuprofile", "cpu2.out", "-test.v").Expect(t, + testParse(t, "-test.cpuprofile", "cpu2.out", "-test.v").Expect(t, `cpuProfile: "cpu2.out"`, `passon : ["-test.v"]`, ) @@ -64,7 +64,7 @@ func TestParse(t *testing.T) { t.Run("CPU3", func(t *testing.T) { // Ours followed by one unrecognized that uses "=". - testParse("-test.cpuprofile", "cpu3.out", "-test.v=true").Expect(t, + testParse(t, "-test.cpuprofile", "cpu3.out", "-test.v=true").Expect(t, `cpuProfile: "cpu3.out"`, `passon : ["-test.v=true"]`, ) @@ -72,7 +72,7 @@ func TestParse(t *testing.T) { t.Run("EqualCPU4", func(t *testing.T) { // Swapping order from Cpu3 test, the unrecognized first, followed by ours. - testParse("-test.v=true", "-test.cpuprofile", "cpu4.out").Expect(t, + testParse(t, "-test.v=true", "-test.cpuprofile", "cpu4.out").Expect(t, `cpuProfile: "cpu4.out"`, `passon : ["-test.v=true"]`, ) @@ -80,7 +80,7 @@ func TestParse(t *testing.T) { t.Run("ExtraBool1", func(t *testing.T) { // Ours followed by two unrecognized. - testParse("-test.cpuprofile", "cpu.out", "-test.v", "-bool").Expect(t, + testParse(t, "-test.cpuprofile", "cpu.out", "-test.v", "-bool").Expect(t, `cpuProfile: "cpu.out"`, `passon : ["-test.v" "-bool"]`, ) @@ -88,7 +88,7 @@ func TestParse(t *testing.T) { t.Run("ExtraBool2", func(t *testing.T) { // Ours between two unrecognized. - testParse("-bool", "-test.cpuprofile", "cpu.out", "-test.v").Expect(t, + testParse(t, "-bool", "-test.cpuprofile", "cpu.out", "-test.v").Expect(t, `cpuProfile: "cpu.out"`, `passon : ["-bool" "-test.v"]`, ) @@ -96,7 +96,7 @@ func TestParse(t *testing.T) { t.Run("ExtraStringNoDDash1", func(t *testing.T) { // Ours pulled out from front. - testParse("-test.cpuprofile", "cpu.out", "-test.v", "-bool", "-string", "last").Expect(t, + testParse(t, "-test.cpuprofile", "cpu.out", "-test.v", "-bool", "-string", "last").Expect(t, `cpuProfile: "cpu.out"`, `passon : ["-test.v" "-bool" "-string" "last"]`, ) @@ -104,7 +104,7 @@ func TestParse(t *testing.T) { t.Run("ExtraStringNoDDash2", func(t *testing.T) { // Ours pulled out from middle. - testParse("-string", "first", "-test.cpuprofile", "cpu.out", "-test.v", "-bool").Expect(t, + testParse(t, "-string", "first", "-test.cpuprofile", "cpu.out", "-test.v", "-bool").Expect(t, `cpuProfile: "cpu.out"`, `passon : ["-string" "first" "-test.v" "-bool"]`, ) @@ -112,7 +112,7 @@ func TestParse(t *testing.T) { t.Run("DDash1ExtraString", func(t *testing.T) { // Ours pulled out from front and the -- appears afterwards. - testParse("-test.cpuprofile", "cpu.out", "-test.v", "--", "-bool", "-string", "abc").Expect(t, + testParse(t, "-test.cpuprofile", "cpu.out", "-test.v", "--", "-bool", "-string", "abc").Expect(t, `cpuProfile: "cpu.out"`, `passon : ["-test.v" "--" "-bool" "-string" "abc"]`, ) @@ -120,7 +120,7 @@ func TestParse(t *testing.T) { t.Run("DDash2ExtraString", func(t *testing.T) { // Ours pulled out from front and the -- appears afterwards. - testParse("-test.cpuprofile", "cpu.out", "--", "-test.v", "-bool", "-string", "abc").Expect(t, + testParse(t, "-test.cpuprofile", "cpu.out", "--", "-test.v", "-bool", "-string", "abc").Expect(t, `cpuProfile: "cpu.out"`, `passon : ["--" "-test.v" "-bool" "-string" "abc"]`, ) @@ -128,7 +128,7 @@ func TestParse(t *testing.T) { t.Run("DDash3UnprocessedProfile", func(t *testing.T) { // Ours *not* pulled out because it appears after a --, just as "go test" would handle it. - testParse("--", "-test.cpuprofile", "cpu.other", "-test.v", "-bool", "-string", "abc").Expect(t, + testParse(t, "--", "-test.cpuprofile", "cpu.other", "-test.v", "-bool", "-string", "abc").Expect(t, `cpuProfile: ""`, `passon : ["--" "-test.cpuprofile" "cpu.other" "-test.v" "-bool" "-string" "abc"]`, ) @@ -174,7 +174,8 @@ func (g testParseGot) Expect(t testing.TB, expect ...string) { } } -func testParse(args ...string) testParseGot { +func testParse(t *testing.T, args ...string) testParseGot { + t.Helper() var ( cpuProfile string ) @@ -184,7 +185,10 @@ func testParse(args ...string) testParseGot { flagset.StringVar(&cpuProfile, "test.cpuprofile", "", "") - passon := gentleParse(flagset, args) + passon, err := gentleParse(flagset, args) + if err != nil { + t.Error(err) + } return makeParseGot( fmt.Sprintf("cpuProfile: %q", cpuProfile), @@ -195,7 +199,8 @@ func testParse(args ...string) testParseGot { // This one acts more like an example of how to perform a different type of test. // It was perhaps useful in early stages of building unit tests but then seems // to have gone unused except for the default, empty, case. -func testParseOther(args ...string) testParseGot { +func testParseOther(t *testing.T, args ...string) testParseGot { + t.Helper() var ( cpuProfile string other string @@ -207,7 +212,10 @@ func testParseOther(args ...string) testParseGot { flagset.StringVar(&cpuProfile, "test.cpuprofile", "", "") flagset.StringVar(&other, "other", "default-other-value", "") - passon := gentleParse(flagset, args) + passon, err := gentleParse(flagset, args) + if err != nil { + t.Error(err) + } return makeParseGot( fmt.Sprintf("cpuProfile: %q", cpuProfile),