test: Tidy up creation and removal of test directories

We have many places in our tests where we create temporary directories,
which we later remove (most of the time). We have at least 3 helpers to
do this, and various places where it's done ad-hoc (and the cleanup is
not always present).

To try to reduce the clutter, and make the tests more uniform and
readable, this patch introduces two helpers in a new "testutil" package:
one for creating and one for removing temporary directories.

These new functions are safer, better tested, and make the tests more
consistent.  All the tests are updated to use them.

internal/config/config_test.go

@@ -4,16 +4,14 @@ import (
 	"io/ioutil"
 	"os"
 	"testing"
+
+	"blitiri.com.ar/go/chasquid/internal/testlib"
 )
 
 func mustCreateConfig(t *testing.T, contents string) (string, string) {
-	tmpDir, err := ioutil.TempDir("", "chasquid_config_test:")
+	tmpDir := testlib.MustTempDir(t)
-	if err != nil {
-		t.Fatalf("Failed to create temp dir: %v\n", tmpDir)
-	}
-
 	confStr := []byte(contents)
-	err = ioutil.WriteFile(tmpDir+"/chasquid.conf", confStr, 0600)
+	err := ioutil.WriteFile(tmpDir+"/chasquid.conf", confStr, 0600)
 	if err != nil {
 		t.Fatalf("Failed to write tmp config: %v", err)
 	}
@@ -23,7 +21,7 @@ func mustCreateConfig(t *testing.T, contents string) (string, string) {
 
 func TestEmptyConfig(t *testing.T) {
 	tmpDir, path := mustCreateConfig(t, "")
-	defer os.RemoveAll(tmpDir)
+	defer testlib.RemoveIfOk(t, tmpDir)
 	c, err := Load(path)
 	if err != nil {
 		t.Fatalf("error loading empty config: %v", err)
@@ -64,7 +62,7 @@ func TestFullConfig(t *testing.T) {
 	`
 
 	tmpDir, path := mustCreateConfig(t, confStr)
-	defer os.RemoveAll(tmpDir)
+	defer testlib.RemoveIfOk(t, tmpDir)
 
 	c, err := Load(path)
 	if err != nil {
@@ -99,7 +97,7 @@ func TestErrorLoading(t *testing.T) {
 func TestBrokenConfig(t *testing.T) {
 	tmpDir, path := mustCreateConfig(
 		t, "<invalid> this is not a valid protobuf")
-	defer os.RemoveAll(tmpDir)
+	defer testlib.RemoveIfOk(t, tmpDir)
 
 	c, err := Load(path)
 	if err == nil {

internal/courier/procmail_test.go

@@ -6,14 +6,13 @@ import (
 	"os"
 	"testing"
 	"time"
+
+	"blitiri.com.ar/go/chasquid/internal/testlib"
 )
 
 func TestProcmail(t *testing.T) {
-	dir, err := ioutil.TempDir("", "test-chasquid-courier")
+	dir := testlib.MustTempDir(t)
-	if err != nil {
+	defer testlib.RemoveIfOk(t, dir)
-		t.Fatalf("Failed to create temp dir: %v", err)
-	}
-	defer os.RemoveAll(dir)
 
 	p := Procmail{
 		Binary:  "tee",
@@ -21,7 +20,7 @@ func TestProcmail(t *testing.T) {
 		Timeout: 1 * time.Minute,
 	}
 
-	err, _ = p.Deliver("from@x", "to@local", []byte("data"))
+	err, _ := p.Deliver("from@x", "to@local", []byte("data"))
 	if err != nil {
 		t.Fatalf("Deliver: %v", err)
 	}

internal/courier/smtp_test.go

@@ -2,22 +2,17 @@ package courier
 
 import (
 	"bufio"
-	"io/ioutil"
 	"net"
 	"net/textproto"
-	"os"
 	"testing"
 	"time"
 
 	"blitiri.com.ar/go/chasquid/internal/domaininfo"
+	"blitiri.com.ar/go/chasquid/internal/testlib"
 )
 
 func newSMTP(t *testing.T) (*SMTP, string) {
-	dir, err := ioutil.TempDir("", "smtp_test")
+	dir := testlib.MustTempDir(t)
-	if err != nil {
-		t.Fatal(err)
-	}
-
 	dinfo, err := domaininfo.New(dir)
 	if err != nil {
 		t.Fatal(err)
@@ -93,7 +88,7 @@ func TestSMTP(t *testing.T) {
 	*smtpPort = port
 
 	s, tmpDir := newSMTP(t)
-	defer os.Remove(tmpDir)
+	defer testlib.RemoveIfOk(t, tmpDir)
 	err, _ := s.Deliver("me@me", "to@to", []byte("data"))
 	if err != nil {
 		t.Errorf("deliver failed: %v", err)
@@ -154,7 +149,7 @@ func TestSMTPErrors(t *testing.T) {
 		*smtpPort = port
 
 		s, tmpDir := newSMTP(t)
-		defer os.Remove(tmpDir)
+		defer testlib.RemoveIfOk(t, tmpDir)
 		err, _ := s.Deliver("me@me", "to@to", []byte("data"))
 		if err == nil {
 			t.Errorf("deliver not failed in case %q: %v", rs["_welcome"], err)
@@ -167,7 +162,7 @@ func TestNoMXServer(t *testing.T) {
 	fakeMX["to"] = []string{}
 
 	s, tmpDir := newSMTP(t)
-	defer os.Remove(tmpDir)
+	defer testlib.RemoveIfOk(t, tmpDir)
 	err, permanent := s.Deliver("me@me", "to@to", []byte("data"))
 	if err == nil {
 		t.Errorf("delivery worked, expected failure")

internal/domaininfo/domaininfo_test.go

@@ -1,24 +1,15 @@
 package domaininfo
 
 import (
-	"io/ioutil"
-	"os"
 	"testing"
 	"time"
+
+	"blitiri.com.ar/go/chasquid/internal/testlib"
 )
 
-func mustTempDir(t *testing.T) string {
-	dir, err := ioutil.TempDir("", "greylisting_test")
-	if err != nil {
-		t.Fatal(err)
-	}
-
-	t.Logf("test directory: %q", dir)
-	return dir
-}
-
 func TestBasic(t *testing.T) {
-	dir := mustTempDir(t)
+	dir := testlib.MustTempDir(t)
+	defer testlib.RemoveIfOk(t, dir)
 	db, err := New(dir)
 	if err != nil {
 		t.Fatal(err)
@@ -59,14 +50,11 @@ func TestBasic(t *testing.T) {
 	if db2.IncomingSecLevel("d1", SecLevel_TLS_INSECURE) {
 		t.Errorf("decrement to tls-insecure was allowed in new DB")
 	}
-
-	if !t.Failed() {
-		os.RemoveAll(dir)
-	}
 }
 
 func TestNewDomain(t *testing.T) {
-	dir := mustTempDir(t)
+	dir := testlib.MustTempDir(t)
+	defer testlib.RemoveIfOk(t, dir)
 	db, err := New(dir)
 	if err != nil {
 		t.Fatal(err)
@@ -88,13 +76,11 @@ func TestNewDomain(t *testing.T) {
 			t.Errorf("domain %q not allowed (out) at %s", c.domain, c.level)
 		}
 	}
-	if !t.Failed() {
-		os.RemoveAll(dir)
-	}
 }
 
 func TestProgressions(t *testing.T) {
-	dir := mustTempDir(t)
+	dir := testlib.MustTempDir(t)
+	defer testlib.RemoveIfOk(t, dir)
 	db, err := New(dir)
 	if err != nil {
 		t.Fatal(err)
@@ -126,8 +112,4 @@ func TestProgressions(t *testing.T) {
 				i, c.domain, c.lvl, ok, c.ok)
 		}
 	}
-
-	if !t.Failed() {
-		os.RemoveAll(dir)
-	}
 }

internal/protoio/protoio_test.go

@@ -1,31 +1,15 @@
 package protoio
 
 import (
-	"io/ioutil"
-	"os"
 	"testing"
 
 	"blitiri.com.ar/go/chasquid/internal/protoio/testpb"
+	"blitiri.com.ar/go/chasquid/internal/testlib"
 )
 
-func mustTempDir(t *testing.T) string {
-	dir, err := ioutil.TempDir("", "safeio_test")
-	if err != nil {
-		t.Fatal(err)
-	}
-
-	err = os.Chdir(dir)
-	if err != nil {
-		t.Fatal(err)
-	}
-
-	t.Logf("test directory: %q", dir)
-
-	return dir
-}
-
 func TestBin(t *testing.T) {
-	dir := mustTempDir(t)
+	dir := testlib.MustTempDir(t)
+	defer testlib.RemoveIfOk(t, dir)
 	pb := &testpb.M{"hola"}
 
 	if err := WriteMessage("f", pb, 0600); err != nil {
@@ -39,14 +23,11 @@ func TestBin(t *testing.T) {
 	if pb.Content != pb2.Content {
 		t.Errorf("content mismatch, got %q, expected %q", pb2.Content, pb.Content)
 	}
-
-	if !t.Failed() {
-		os.RemoveAll(dir)
-	}
 }
 
 func TestText(t *testing.T) {
-	dir := mustTempDir(t)
+	dir := testlib.MustTempDir(t)
+	defer testlib.RemoveIfOk(t, dir)
 	pb := &testpb.M{"hola"}
 
 	if err := WriteTextMessage("f", pb, 0600); err != nil {
@@ -60,14 +41,11 @@ func TestText(t *testing.T) {
 	if pb.Content != pb2.Content {
 		t.Errorf("content mismatch, got %q, expected %q", pb2.Content, pb.Content)
 	}
-
-	if !t.Failed() {
-		os.RemoveAll(dir)
-	}
 }
 
 func TestStore(t *testing.T) {
-	dir := mustTempDir(t)
+	dir := testlib.MustTempDir(t)
+	defer testlib.RemoveIfOk(t, dir)
 	st, err := NewStore(dir + "/store")
 	if err != nil {
 		t.Fatalf("failed to create store: %v", err)
@@ -98,8 +76,4 @@ func TestStore(t *testing.T) {
 	if ids, err := st.ListIDs(); len(ids) != 1 || ids[0] != "f" || err != nil {
 		t.Errorf("expected [f], got %v - %v", ids, err)
 	}
-
-	if !t.Failed() {
-		os.RemoveAll(dir)
-	}
 }

internal/queue/queue_test.go

@@ -10,6 +10,7 @@ import (
 
 	"blitiri.com.ar/go/chasquid/internal/aliases"
 	"blitiri.com.ar/go/chasquid/internal/set"
+	"blitiri.com.ar/go/chasquid/internal/testlib"
 )
 
 // Test courier. Delivery is done by sending on a channel, so users have fine
@@ -61,9 +62,11 @@ func newTestCourier() *TestCourier {
 }
 
 func TestBasic(t *testing.T) {
+	dir := testlib.MustTempDir(t)
+	defer testlib.RemoveIfOk(t, dir)
 	localC := newTestCourier()
 	remoteC := newTestCourier()
-	q := New("/tmp/queue_test", set.NewString("loco"), aliases.NewResolver(),
+	q := New(dir, set.NewString("loco"), aliases.NewResolver(),
 		localC, remoteC)
 
 	localC.wg.Add(2)
@@ -116,7 +119,9 @@ func TestBasic(t *testing.T) {
 func TestDSNOnTimeout(t *testing.T) {
 	localC := newTestCourier()
 	remoteC := newTestCourier()
-	q := New("/tmp/queue_test", set.NewString("loco"), aliases.NewResolver(),
+	dir := testlib.MustTempDir(t)
+	defer testlib.RemoveIfOk(t, dir)
+	q := New(dir, set.NewString("loco"), aliases.NewResolver(),
 		localC, remoteC)
 
 	// Insert an expired item in the queue.
@@ -155,7 +160,9 @@ func TestDSNOnTimeout(t *testing.T) {
 func TestAliases(t *testing.T) {
 	localC := newTestCourier()
 	remoteC := newTestCourier()
-	q := New("/tmp/queue_test", set.NewString("loco"), aliases.NewResolver(),
+	dir := testlib.MustTempDir(t)
+	defer testlib.RemoveIfOk(t, dir)
+	q := New(dir, set.NewString("loco"), aliases.NewResolver(),
 		localC, remoteC)
 
 	q.aliases.AddDomain("loco")
@@ -206,7 +213,9 @@ func (c DumbCourier) Deliver(from string, to string, data []byte) (error, bool)
 var dumbCourier = DumbCourier{}
 
 func TestFullQueue(t *testing.T) {
-	q := New("/tmp/queue_test", set.NewString(), aliases.NewResolver(),
+	dir := testlib.MustTempDir(t)
+	defer testlib.RemoveIfOk(t, dir)
+	q := New(dir, set.NewString(), aliases.NewResolver(),
 		dumbCourier, dumbCourier)
 
 	// Force-insert maxQueueSize items in the queue.
@@ -246,7 +255,9 @@ func TestFullQueue(t *testing.T) {
 }
 
 func TestPipes(t *testing.T) {
-	q := New("/tmp/queue_test", set.NewString("loco"), aliases.NewResolver(),
+	dir := testlib.MustTempDir(t)
+	defer testlib.RemoveIfOk(t, dir)
+	q := New(dir, set.NewString("loco"), aliases.NewResolver(),
 		dumbCourier, dumbCourier)
 	item := &Item{
 		Message: Message{

internal/safeio/safeio_test.go

@@ -8,24 +8,10 @@ import (
 	"os"
 	"strings"
 	"testing"
+
+	"blitiri.com.ar/go/chasquid/internal/testlib"
 )
 
-func mustTempDir(t *testing.T) string {
-	dir, err := ioutil.TempDir("", "safeio_test")
-	if err != nil {
-		t.Fatal(err)
-	}
-
-	err = os.Chdir(dir)
-	if err != nil {
-		t.Fatal(err)
-	}
-
-	t.Logf("test directory: %q", dir)
-
-	return dir
-}
-
 func testWriteFile(fname string, data []byte, perm os.FileMode, ops ...FileOp) error {
 	err := WriteFile("file1", data, perm, ops...)
 	if err != nil {
@@ -56,7 +42,8 @@ func testWriteFile(fname string, data []byte, perm os.FileMode, ops ...FileOp) e
 }
 
 func TestWriteFile(t *testing.T) {
-	dir := mustTempDir(t)
+	dir := testlib.MustTempDir(t)
+	defer testlib.RemoveIfOk(t, dir)
 
 	// Write a new file.
 	content := []byte("content 1")
@@ -75,16 +62,11 @@ func TestWriteFile(t *testing.T) {
 	if err := testWriteFile("file1", content, 0600); err != nil {
 		t.Error(err)
 	}
-
-	// Remove the test directory, but only if we have not failed. We want to
-	// keep the failed structure for debugging.
-	if !t.Failed() {
-		os.RemoveAll(dir)
-	}
 }
 
 func TestWriteFileWithOp(t *testing.T) {
-	dir := mustTempDir(t)
+	dir := testlib.MustTempDir(t)
+	defer testlib.RemoveIfOk(t, dir)
 
 	var opFile string
 	op := func(f string) error {
@@ -103,16 +85,11 @@ func TestWriteFileWithOp(t *testing.T) {
 	if !strings.Contains(opFile, "file1") {
 		t.Errorf("operation called with suspicious file: %s", opFile)
 	}
-
-	// Remove the test directory, but only if we have not failed. We want to
-	// keep the failed structure for debugging.
-	if !t.Failed() {
-		os.RemoveAll(dir)
-	}
 }
 
 func TestWriteFileWithFailingOp(t *testing.T) {
-	dir := mustTempDir(t)
+	dir := testlib.MustTempDir(t)
+	defer testlib.RemoveIfOk(t, dir)
 
 	var opFile string
 	opOK := func(f string) error {
@@ -134,12 +111,6 @@ func TestWriteFileWithFailingOp(t *testing.T) {
 	if _, err := os.Stat(opFile); err == nil {
 		t.Errorf("temporary file was not removed after failure (%v)", opFile)
 	}
-
-	// Remove the test directory, but only if we have not failed. We want to
-	// keep the failed structure for debugging.
-	if !t.Failed() {
-		os.RemoveAll(dir)
-	}
 }
 
 // TODO: We should test the possible failure scenarios for WriteFile, but it

internal/smtpsrv/conn_test.go

@@ -1,25 +1,21 @@
 package smtpsrv
 
 import (
-	"io/ioutil"
-	"os"
 	"testing"
 
 	"blitiri.com.ar/go/chasquid/internal/domaininfo"
 	"blitiri.com.ar/go/chasquid/internal/spf"
+	"blitiri.com.ar/go/chasquid/internal/testlib"
 	"blitiri.com.ar/go/chasquid/internal/trace"
 )
 
 func TestSecLevel(t *testing.T) {
 	// We can't simulate this externally because of the SPF record
 	// requirement, so do a narrow test on Conn.secLevelCheck.
-	tmpDir, err := ioutil.TempDir("", "chasquid_test:")
+	dir := testlib.MustTempDir(t)
-	if err != nil {
+	defer testlib.RemoveIfOk(t, dir)
-		t.Fatalf("Failed to create temp dir: %v", err)
-	}
-	defer os.RemoveAll(tmpDir)
 
-	dinfo, err := domaininfo.New(tmpDir)
+	dinfo, err := domaininfo.New(dir)
 	if err != nil {
 		t.Fatalf("Failed to create domain info: %v", err)
 	}

internal/testlib/testlib.go

@@ -0,0 +1,39 @@
+// Package testlib provides common test utilities.
+package testlib
+
+import (
+	"io/ioutil"
+	"os"
+	"strings"
+	"testing"
+)
+
+// MustTempDir creates a temporary directory, or dies trying.
+func MustTempDir(t *testing.T) string {
+	dir, err := ioutil.TempDir("", "testlib_")
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	err = os.Chdir(dir)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	t.Logf("test directory: %q", dir)
+	return dir
+}
+
+// RemoveIfOk removes the given directory, but only if we have not failed. We
+// want to keep the failed directories for debugging.
+func RemoveIfOk(t *testing.T, dir string) {
+	// Safeguard, to make sure we only remove test directories.
+	// This should help prevent accidental deletions.
+	if !strings.Contains(dir, "testlib_") {
+		panic("invalid/dangerous directory")
+	}
+
+	if !t.Failed() {
+		os.RemoveAll(dir)
+	}
+}

internal/testlib/testlib_test.go

@@ -0,0 +1,54 @@
+package testlib
+
+import (
+	"io/ioutil"
+	"os"
+	"testing"
+)
+
+func TestBasic(t *testing.T) {
+	dir := MustTempDir(t)
+	if err := ioutil.WriteFile(dir+"/file", nil, 0660); err != nil {
+		t.Fatalf("could not create file in %s: %v", dir, err)
+	}
+
+	wd, err := os.Getwd()
+	if err != nil {
+		t.Fatalf("could not get working directory: %v", err)
+	}
+	if wd != dir {
+		t.Errorf("MustTempDir did not change directory")
+		t.Errorf("  expected %q, got %q", dir, wd)
+	}
+
+	RemoveIfOk(t, dir)
+	if _, err := os.Stat(dir); !os.IsNotExist(err) {
+		t.Fatalf("%s existed, should have been deleted: %v", dir, err)
+	}
+}
+
+func TestRemoveCheck(t *testing.T) {
+	defer func() {
+		if r := recover(); r != nil {
+			t.Logf("recovered: %v", r)
+		} else {
+			t.Fatalf("check did not panic as expected")
+		}
+	}()
+
+	RemoveIfOk(t, "/tmp/something")
+}
+
+func TestLeaveDirOnError(t *testing.T) {
+	myt := &testing.T{}
+	dir := MustTempDir(myt)
+	myt.Errorf("something bad happened")
+
+	RemoveIfOk(myt, dir)
+	if _, err := os.Stat(dir); os.IsNotExist(err) {
+		t.Fatalf("%s was removed, should have been kept", dir)
+	}
+
+	// Remove the directory for real this time.
+	RemoveIfOk(t, dir)
+}

internal/userdb/userdb_test.go

@@ -160,6 +160,7 @@ func TestWrite(t *testing.T) {
 
 func TestNew(t *testing.T) {
 	fname := fmt.Sprintf("%s/userdb_test-%d", os.TempDir(), os.Getpid())
+	defer os.Remove(fname)
 	db1 := New(fname)
 	db1.AddUser("user", "passwd")
 	db1.Write()