git » chasquid » commit 9864f40

diff --git a/internal/config/config_test.go b/internal/config/config_test.go
index 790c2a0..c871457 100644
--- a/internal/config/config_test.go
+++ b/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:")
-	if err != nil {
-		t.Fatalf("Failed to create temp dir: %v\n", tmpDir)
-	}
-
+	tmpDir := testlib.MustTempDir(t)
 	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 {
diff --git a/internal/courier/procmail_test.go b/internal/courier/procmail_test.go
index 0668a36..1729215 100644
--- a/internal/courier/procmail_test.go
+++ b/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")
-	if err != nil {
-		t.Fatalf("Failed to create temp dir: %v", err)
-	}
-	defer os.RemoveAll(dir)
+	dir := testlib.MustTempDir(t)
+	defer testlib.RemoveIfOk(t, 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)
 	}
diff --git a/internal/courier/smtp_test.go b/internal/courier/smtp_test.go
index 72fbe10..7edef1e 100644
--- a/internal/courier/smtp_test.go
+++ b/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")
-	if err != nil {
-		t.Fatal(err)
-	}
-
+	dir := testlib.MustTempDir(t)
 	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")
diff --git a/internal/domaininfo/domaininfo_test.go b/internal/domaininfo/domaininfo_test.go
index 266dd19..6aa2790 100644
--- a/internal/domaininfo/domaininfo_test.go
+++ b/internal/domaininfo/domaininfo_test.go
@@ -1,24 +1,15 @@
 package domaininfo
 
 import (
-	"io/ioutil"
-	"os"
 	"testing"
 	"time"
-)
-
-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
-}
+	"blitiri.com.ar/go/chasquid/internal/testlib"
+)
 
 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)
-	}
 }
diff --git a/internal/protoio/protoio_test.go b/internal/protoio/protoio_test.go
index e8ad037..32e79e6 100644
--- a/internal/protoio/protoio_test.go
+++ b/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)
-	}
 }
diff --git a/internal/queue/queue_test.go b/internal/queue/queue_test.go
index 1b485d2..bd9839a 100644
--- a/internal/queue/queue_test.go
+++ b/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{
diff --git a/internal/safeio/safeio_test.go b/internal/safeio/safeio_test.go
index f5dc81d..2d4150c 100644
--- a/internal/safeio/safeio_test.go
+++ b/internal/safeio/safeio_test.go
@@ -8,23 +8,9 @@ import (
 	"os"
 	"strings"
 	"testing"
-)
-
-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
-}
+	"blitiri.com.ar/go/chasquid/internal/testlib"
+)
 
 func testWriteFile(fname string, data []byte, perm os.FileMode, ops ...FileOp) error {
 	err := WriteFile("file1", data, perm, ops...)
@@ -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
diff --git a/internal/smtpsrv/conn_test.go b/internal/smtpsrv/conn_test.go
index ed5b0c7..c58ad29 100644
--- a/internal/smtpsrv/conn_test.go
+++ b/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:")
-	if err != nil {
-		t.Fatalf("Failed to create temp dir: %v", err)
-	}
-	defer os.RemoveAll(tmpDir)
+	dir := testlib.MustTempDir(t)
+	defer testlib.RemoveIfOk(t, dir)
 
-	dinfo, err := domaininfo.New(tmpDir)
+	dinfo, err := domaininfo.New(dir)
 	if err != nil {
 		t.Fatalf("Failed to create domain info: %v", err)
 	}
diff --git a/internal/testlib/testlib.go b/internal/testlib/testlib.go
new file mode 100644
index 0000000..7a96cbd
--- /dev/null
+++ b/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)
+	}
+}
diff --git a/internal/testlib/testlib_test.go b/internal/testlib/testlib_test.go
new file mode 100644
index 0000000..fad4b65
--- /dev/null
+++ b/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)
+}
diff --git a/internal/userdb/userdb_test.go b/internal/userdb/userdb_test.go
index f132933..f0a2953 100644
--- a/internal/userdb/userdb_test.go
+++ b/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()