webhook: rework flaky TestDump due to file races

webhook/dump.go

@@ -51,13 +51,22 @@ func (r *recorder) WriteHeader(status int) {
 	r.ResponseWriter.WriteHeader(status)
 }
 
-type dumper struct {
-	dir     string
-	log     *log.Logger
-	handler http.Handler
+// Dumper is a helper handler, which wraps other http.Handler and dumps its
+// requests' bodies to files, when serving them was successful.
+type Dumper struct {
+	Handler http.Handler // underlying handler
+	Dir     string       // directory where files are written
+
+	// ErrorLog specifies an optional logger for errors serving requests.
+	// If nil, logging goes to os.Stderr via the log package's standard logger.
+	ErrorLog *log.Logger
+
+	// WriteFile specifies an optional file writer.
+	// If nil, ioutil.WriteFile is used instead.
+	WriteFile func(string, []byte, os.FileMode) error
 }
 
-// Dump is a helper handler, which wraps a webhook handler and dumps each
+// Dump creates new Dumper handler, which wraps a webhook handler and dumps each
 // request's body to a file when response was served successfully. It was
 // added for *webhook.Handler in mind, but works on every generic http.Handler.
 //
@@ -67,7 +76,7 @@ type dumper struct {
 // If either of the above functions fails, Dump panics.
 // If handler is a *webhook Handler and its ErrorLog field is non-nil, Dump uses
 // it for logging.
-func Dump(dir string, handler http.Handler) http.Handler {
+func Dump(dir string, handler http.Handler) *Dumper {
 	switch {
 	case dir == "":
 		name, err := ioutil.TempDir("", "webhook")
@@ -85,45 +94,51 @@ func Dump(dir string, handler http.Handler) http.Handler {
 			panic(err)
 		}
 	}
-	d := &dumper{
-		dir:     dir,
-		handler: handler,
+	d := &Dumper{
+		Dir:     dir,
+		Handler: handler,
 	}
 	if handler, ok := handler.(*Handler); ok {
-		d.log = handler.ErrorLog
+		d.ErrorLog = handler.ErrorLog
 	}
 	return d
 }
 
 // ServeHTTP implements the http.Handler interface.
-func (d dumper) ServeHTTP(w http.ResponseWriter, req *http.Request) {
+func (d *Dumper) ServeHTTP(w http.ResponseWriter, req *http.Request) {
 	buf := &bytes.Buffer{}
 	rec := record(w)
 	req.Body = ioutil.NopCloser(io.TeeReader(req.Body, buf))
-	d.handler.ServeHTTP(rec, req)
+	d.Handler.ServeHTTP(rec, req)
 	if rec.status == 200 {
 		go d.dump(req.Header.Get("X-GitHub-Event"), buf)
 	}
 }
 
-func (d dumper) dump(event string, buf *bytes.Buffer) {
+func (d *Dumper) dump(event string, buf *bytes.Buffer) {
 	var name string
 	if event != "" {
-		name = filepath.Join(d.dir, fmt.Sprintf("%s-%s.json", event, now()))
+		name = filepath.Join(d.Dir, fmt.Sprintf("%s-%s.json", event, now()))
+	} else {
+		name = filepath.Join(d.Dir, now())
+	}
+	var err error
+	if d.WriteFile != nil {
+		err = d.WriteFile(name, buf.Bytes(), 0644)
 	} else {
-		name = filepath.Join(d.dir, now())
+		err = ioutil.WriteFile(name, buf.Bytes(), 0644)
 	}
-	switch err := writefile(name, buf.Bytes(), 0644); err {
+	switch err {
 	case nil:
 		d.logf("INFO %q: written file", name)
 	default:
 		d.logf("ERROR %q: error writing file: %v", name, err)
 	}
 }
 
-func (d dumper) logf(format string, args ...interface{}) {
-	if d.log != nil {
-		d.log.Printf(format, args...)
+func (d *Dumper) logf(format string, args ...interface{}) {
+	if d.ErrorLog != nil {
+		d.ErrorLog.Printf(format, args...)
 	} else {
 		log.Printf(format, args...)
 	}

webhook/dump_test.go

@@ -4,7 +4,6 @@ import (
 	"bytes"
 	"crypto/sha1"
 	"io"
-	"io/ioutil"
 	"os"
 	"path/filepath"
 	"strings"
@@ -19,63 +18,43 @@ func hash(r io.Reader) ([]byte, error) {
 	return h.Sum(nil), nil
 }
 
-func testFiles(t *testing.T, files map[string]string) {
-	for orig, dump := range files {
-		forig, err := os.Open(orig)
-		if err != nil {
-			t.Errorf("os.Open(%q)=%v", orig, err)
-			continue
+func TestDump(t *testing.T) {
+	unique := make(map[string]struct{})
+	test := func(name string, p []byte, _ os.FileMode) error {
+		name = filepath.Base(name)
+		n := strings.IndexRune(name, '-')
+		if n == -1 {
+			t.Fatalf("unexpected file name: %s", name)
+		}
+		event := name[:n]
+		if _, ok := payloads[event]; !ok {
+			t.Fatalf("Dump written a file for a non-existing event: %s", event)
+		}
+		if _, ok := unique[event]; ok {
+			t.Fatalf("duplicate file written for the %s event", event)
 		}
-		fdump, err := os.Open(dump)
+		unique[event] = struct{}{}
+		f, err := os.Open(filepath.Join("testdata", event+".json"))
 		if err != nil {
-			t.Errorf("os.Open(%q)=%v", orig, nonil(err, forig.Close()))
-			continue
+			t.Fatalf("os.Open(%q)=%v", f.Name(), err)
 		}
-		horig, err := hash(forig)
+		defer f.Close()
+		hexpected, err := hash(f)
 		if err != nil {
-			t.Errorf("hashing %s failed: %v", orig, nonil(err, forig.Close(), fdump.Close()))
-			continue
+			t.Fatalf("hashing %s failed: %v", f.Name(), err)
 		}
-		hdump, err := hash(fdump)
+		h, err := hash(bytes.NewReader(p))
 		if err != nil {
-			t.Errorf("hashing %s failed: %v", dump, nonil(err, forig.Close(), fdump.Close()))
-			continue
+			t.Errorf("hashing dumped file failed: %v", err)
 		}
-		if !bytes.Equal(horig, hdump) {
-			t.Errorf("files %q and %q are not equal", orig, dump)
+		if !bytes.Equal(h, hexpected) {
+			t.Errorf("files %q and %q are not equal", name, f.Name())
 		}
+		return nil
 	}
-}
-
-func TestDump(t *testing.T) {
-	tmp, err := ioutil.TempDir("", "testdump")
-	if err != nil {
-		t.Fatalf("ioutil.TempDir()=%v", err)
-	}
-	defer os.RemoveAll(tmp)
-	testHandler(t, Dump(tmp, New(secret, BlanketHandler{})))
-	fis, err := ioutil.ReadDir(tmp)
-	if err != nil {
-		t.Fatalf("ioutil.ReadDir(%q)=%v", tmp, err)
-	}
-	if len(fis) != len(payloads) {
-		t.Fatalf("want number of dumped files be %d; got %d", len(payloads), len(fis))
-	}
-	files := make(map[string]string)
-	for _, fi := range fis {
-		n := strings.IndexRune(fi.Name(), '-')
-		if n == -1 {
-			t.Fatalf("unexpected file name: %s", fi.Name())
-		}
-		event := fi.Name()[:n]
-		if _, ok := payloads[event]; !ok {
-			t.Fatalf("Dump written a file for a non-existing event: %s", event)
-		}
-		orig := filepath.Join("testdata", event+".json")
-		if _, ok := files[orig]; ok {
-			t.Fatalf("duplicated files for the %s event", event)
-		}
-		files[orig] = filepath.Join(tmp, fi.Name())
+	h := &Dumper{
+		Handler:   New(secret, BlanketHandler{}),
+		WriteFile: test,
 	}
-	testFiles(t, files)
+	testHandler(t, h)
 }