From 1045268f012446675849cfb9ab89a667f167369a Mon Sep 17 00:00:00 2001 From: Seebs Date: Wed, 10 Mar 2021 13:37:48 -0600 Subject: [PATCH 1/2] use testhook to ensure temporary files and directories are cleaned up In nearly all cases, we can just switch ioutil.TempDir->testhook.TempDir and similarly for TempFile. There's one case where we can't because we need files to be removed before tests are over. Also in the process give identifiable names to a lot of temporary files and make sure they're being cleaned up, and don't use "/tmp/foo" as a file name in a test that could be running in more than one test process at once. :) --- attr_test.go | 25 +++++++++++++---------- boltdb/translate.go | 2 -- boltdb/translate_test.go | 41 +++++++++++++++++-------------------- cmd/root_test.go | 7 ++++--- cmd/server_test.go | 10 ++++----- ctl/check_test.go | 34 +++++++++++++++---------------- ctl/import_test.go | 33 +++++++++++++++--------------- ctl/inspect_test.go | 5 +++-- dbshard_internal_test.go | 6 +++--- etcd/leasedkv_test.go | 4 ++-- fragment_internal_test.go | 11 ++++++++-- idalloc_test.go | 9 ++------ logger/filewriter_test.go | 43 ++++++++++++++++++--------------------- rbf/db_test.go | 2 +- rbf/ingest_test.go | 6 ++++-- rbf/rbf_test.go | 8 ++++---- rbf/util_test.go | 4 ++-- server/cluster_test.go | 2 +- test/disco.go | 5 ++--- testhook/hook.go | 33 +++++++++++++++++++++++++++++- 20 files changed, 161 insertions(+), 129 deletions(-) diff --git a/attr_test.go b/attr_test.go index 38af27c5a..cc40daf3c 100644 --- a/attr_test.go +++ b/attr_test.go @@ -15,7 +15,6 @@ package pilosa_test import ( - "io/ioutil" "os" "reflect" "runtime" @@ -24,11 +23,12 @@ import ( "github.com/pilosa/pilosa/v2" "github.com/pilosa/pilosa/v2/boltdb" + "github.com/pilosa/pilosa/v2/testhook" ) // Ensure database can set and retrieve column attributes. func TestAttrStore_Attrs(t *testing.T) { - s := MustOpenAttrStore() + s := MustOpenAttrStore(t) defer s.Close() // Set attributes. @@ -57,7 +57,7 @@ func TestAttrStore_Attrs(t *testing.T) { // Ensure database returns a non-nil empty map if unset. func TestAttrStore_Attrs_Empty(t *testing.T) { - s := MustOpenAttrStore() + s := MustOpenAttrStore(t) defer s.Close() if m, err := s.Attrs(100); err != nil { @@ -69,7 +69,7 @@ func TestAttrStore_Attrs_Empty(t *testing.T) { // Ensure database can unset attributes if explicitly set to nil. func TestAttrStore_Attrs_Unset(t *testing.T) { - s := MustOpenAttrStore() + s := MustOpenAttrStore(t) defer s.Close() // Set attributes. @@ -89,7 +89,7 @@ func TestAttrStore_Attrs_Unset(t *testing.T) { // Ensure attribute block checksums can be returned. func TestAttrStore_Blocks(t *testing.T) { - s := MustOpenAttrStore() + s := MustOpenAttrStore(t) defer s.Close() // Set attributes. @@ -135,19 +135,22 @@ type AttrStore struct { } // NewAttrStore returns a new instance of AttrStore. -func NewAttrStore(string) pilosa.AttrStore { - f, err := ioutil.TempFile("", "pilosa-attr-") +func NewAttrStore(tb testing.TB) pilosa.AttrStore { + f, err := testhook.TempFile(tb, "pilosa-attr-") if err != nil { panic(err) } + // Note, even though the file is closed, TempFile will still avoid + // creating the same name again if we leave it existing. The boltdb + // code may already be deleting this, so the TestHook deletion + // may not matter but it's more reliable this way. f.Close() - os.Remove(f.Name()) return &AttrStore{boltdb.NewAttrStore(f.Name())} } func BenchmarkAttrStore_Duplicate(b *testing.B) { - s := MustOpenAttrStore() + s := MustOpenAttrStore(b) defer s.Close() // Set attributes. @@ -186,8 +189,8 @@ func BenchmarkAttrStore_Duplicate(b *testing.B) { } // MustOpenAttrStore returns a new, opened attribute store at a temporary path. Panic on error. -func MustOpenAttrStore() pilosa.AttrStore { - s := NewAttrStore("") +func MustOpenAttrStore(tb testing.TB) pilosa.AttrStore { + s := NewAttrStore(tb) if err := s.Open(); err != nil { panic(err) } diff --git a/boltdb/translate.go b/boltdb/translate.go index 00f0e657c..2df5114b2 100644 --- a/boltdb/translate.go +++ b/boltdb/translate.go @@ -19,7 +19,6 @@ import ( "context" "fmt" "io" - "io/ioutil" "os" "path/filepath" "sync" @@ -32,7 +31,6 @@ import ( "runtime/pprof" ) -var _ = ioutil.TempFile var _ = pprof.StartCPUProfile var ( diff --git a/boltdb/translate_test.go b/boltdb/translate_test.go index 6908da6ac..c016b0e63 100644 --- a/boltdb/translate_test.go +++ b/boltdb/translate_test.go @@ -17,8 +17,6 @@ import ( "bytes" "context" "fmt" - "io/ioutil" - "os" "reflect" "strconv" "testing" @@ -26,13 +24,14 @@ import ( "github.com/pilosa/pilosa/v2" "github.com/pilosa/pilosa/v2/boltdb" + "github.com/pilosa/pilosa/v2/testhook" "github.com/pilosa/pilosa/v2/topology" ) //var vv = pilosa.VV func TestTranslateStore_TranslateKey(t *testing.T) { - s := MustOpenNewTranslateStore() + s := MustOpenNewTranslateStore(t) defer MustCloseTranslateStore(s) // Ensure initial key translates to first ID for shard @@ -57,7 +56,7 @@ func TestTranslateStore_TranslateKey(t *testing.T) { } func TestTranslateStore_TranslateKeys(t *testing.T) { - s := MustOpenNewTranslateStore() + s := MustOpenNewTranslateStore(t) defer MustCloseTranslateStore(s) ids, err := s.TranslateKeys([]string{"abc", "abc"}, true) @@ -97,7 +96,7 @@ func TestTranslateStore_TranslateKeys(t *testing.T) { } func TestTranslateStore_CreateKeys(t *testing.T) { - s := MustOpenNewTranslateStore() + s := MustOpenNewTranslateStore(t) defer MustCloseTranslateStore(s) ids, err := s.CreateKeys("abc", "abc") @@ -137,7 +136,7 @@ func TestTranslateStore_CreateKeys(t *testing.T) { } func TestTranslateStore_ReadKey(t *testing.T) { - s := MustOpenNewTranslateStore() + s := MustOpenNewTranslateStore(t) defer MustCloseTranslateStore(s) id, err := s.TranslateKey("foo", false) @@ -172,7 +171,7 @@ func TestTranslateStore_ReadKey(t *testing.T) { } func TestTranslateStore_ReadKeys(t *testing.T) { - s := MustOpenNewTranslateStore() + s := MustOpenNewTranslateStore(t) defer MustCloseTranslateStore(s) ids, err := s.TranslateKeys([]string{"foo", "bar", "baz", "baz", "bar", "foo"}, false) @@ -200,7 +199,7 @@ func TestTranslateStore_ReadKeys(t *testing.T) { } } func TestTranslateStore_TranslateID(t *testing.T) { - s := MustOpenNewTranslateStore() + s := MustOpenNewTranslateStore(t) defer MustCloseTranslateStore(s) // Setup initial keys. @@ -239,7 +238,7 @@ func TestTranslateStore_TranslateID(t *testing.T) { } func TestTranslateStore_TranslateIDs(t *testing.T) { - s := MustOpenNewTranslateStore() + s := MustOpenNewTranslateStore(t) defer MustCloseTranslateStore(s) // Setup initial keys. @@ -293,7 +292,7 @@ func TestTranslateStore_FindKeys(t *testing.T) { for _, c := range cases { c := c t.Run(c.name, func(t *testing.T) { - s := MustOpenNewTranslateStore() + s := MustOpenNewTranslateStore(t) defer MustCloseTranslateStore(s) var naiveMap map[string]uint64 @@ -339,7 +338,7 @@ func TestTranslateStore_FindKeys(t *testing.T) { } func TestTranslateStore_MaxID(t *testing.T) { - s := MustOpenNewTranslateStore() + s := MustOpenNewTranslateStore(t) defer MustCloseTranslateStore(s) // Generate a bunch of keys. @@ -364,7 +363,7 @@ func TestTranslateStore_MaxID(t *testing.T) { func TestTranslateStore_EntryReader(t *testing.T) { t.Run("OK", func(t *testing.T) { - s := MustOpenNewTranslateStore() + s := MustOpenNewTranslateStore(t) defer MustCloseTranslateStore(s) // Create multiple new keys. @@ -422,7 +421,7 @@ func TestTranslateStore_EntryReader(t *testing.T) { // Ensure reader will read as soon as a new write comes in using WriteNotify(). t.Run("WriteNotify", func(t *testing.T) { - s := MustOpenNewTranslateStore() + s := MustOpenNewTranslateStore(t) defer MustCloseTranslateStore(s) // Start reader from initial position. @@ -465,7 +464,7 @@ func TestTranslateStore_EntryReader(t *testing.T) { // Ensure exits read on close. t.Run("Close", func(t *testing.T) { - s := MustOpenNewTranslateStore() + s := MustOpenNewTranslateStore(t) defer MustCloseTranslateStore(s) // Start reader from initial position. @@ -499,7 +498,7 @@ func TestTranslateStore_EntryReader(t *testing.T) { // Ensure exits read on store close. t.Run("StoreClose", func(t *testing.T) { - s := MustOpenNewTranslateStore() + s := MustOpenNewTranslateStore(t) defer MustCloseTranslateStore(s) // Start reader from initial position. @@ -533,8 +532,8 @@ func TestTranslateStore_EntryReader(t *testing.T) { } // MustNewTranslateStore returns a new TranslateStore with a temporary path. -func MustNewTranslateStore() *boltdb.TranslateStore { - f, err := ioutil.TempFile("", "") +func MustNewTranslateStore(tb testing.TB) *boltdb.TranslateStore { + f, err := testhook.TempFile(tb, "translate-store") if err != nil { panic(err) } else if err := f.Close(); err != nil { @@ -548,7 +547,7 @@ func MustNewTranslateStore() *boltdb.TranslateStore { func TestTranslateStore_ReadWrite(t *testing.T) { t.Run("WriteTo_ReadFrom", func(t *testing.T) { - s := MustOpenNewTranslateStore() + s := MustOpenNewTranslateStore(t) defer MustCloseTranslateStore(s) batch0 := []string{} @@ -612,8 +611,8 @@ func TestTranslateStore_ReadWrite(t *testing.T) { } // MustOpenNewTranslateStore returns a new, opened TranslateStore. -func MustOpenNewTranslateStore() *boltdb.TranslateStore { - s := MustNewTranslateStore() +func MustOpenNewTranslateStore(tb testing.TB) *boltdb.TranslateStore { + s := MustNewTranslateStore(tb) if err := s.Open(); err != nil { panic(err) } @@ -624,7 +623,5 @@ func MustOpenNewTranslateStore() *boltdb.TranslateStore { func MustCloseTranslateStore(s *boltdb.TranslateStore) { if err := s.Close(); err != nil { panic(err) - } else if err := os.Remove(s.Path); err != nil { - panic(err) } } diff --git a/cmd/root_test.go b/cmd/root_test.go index ec998d346..26da96799 100644 --- a/cmd/root_test.go +++ b/cmd/root_test.go @@ -26,6 +26,7 @@ import ( "time" "github.com/pilosa/pilosa/v2/cmd" + "github.com/pilosa/pilosa/v2/testhook" "github.com/spf13/cobra" ) @@ -135,7 +136,7 @@ func executeDry(t *testing.T, tests []commandTest) { // a temp config file with the cfgFileContent string as its content. func (ct *commandTest) setupCommand(t *testing.T) *cobra.Command { // make config file - cfgFile, err := ioutil.TempFile("", "") + cfgFile, err := testhook.TempFile(t, "cmdconf") failErr(t, err, "making temp file") _, err = cfgFile.WriteString(ct.cfgFileContent) failErr(t, err, "writing config to temp file") @@ -180,9 +181,9 @@ func TestRootCommand(t *testing.T) { } func TestRootCommand_Config(t *testing.T) { - file, err := ioutil.TempFile("", "test.conf") + file, err := testhook.TempFile(t, "test.conf") if err != nil { - panic(err) + t.Fatalf("creating config file: %v", err) } config := `data-dir = "/tmp/pil5_0" bind = "127.0.0.1:10101" diff --git a/cmd/server_test.go b/cmd/server_test.go index f698b9e20..452d1db18 100644 --- a/cmd/server_test.go +++ b/cmd/server_test.go @@ -16,13 +16,13 @@ package cmd_test import ( "fmt" - "io/ioutil" "strings" "testing" "time" "github.com/pilosa/pilosa/v2/cmd" _ "github.com/pilosa/pilosa/v2/test" + "github.com/pilosa/pilosa/v2/testhook" "github.com/pilosa/pilosa/v2/toml" "github.com/pkg/errors" ) @@ -42,9 +42,9 @@ func nextPort() string { //nolint:unused func TestServerConfig(t *testing.T) { t.Skip("pilosa hosts config (cmd.Server.Config.Cluster.Hosts and brethren) is test only and will go away with high probability. skip for now.") - actualDataDir, err := ioutil.TempDir("", "") + actualDataDir, err := testhook.TempDir(t, "") failErr(t, err, "making data dir") - logFile, err := ioutil.TempFile("", "") + logFile, err := testhook.TempFile(t, "") failErr(t, err, "making log file") tests := []commandTest{ // TEST 0 @@ -64,7 +64,7 @@ func TestServerConfig(t *testing.T) { bind-grpc = ` + nextPort() + ` max-writes-per-request = 3000 long-query-time = "1m10s" - + [cluster] replicas = 2 long-query-time = "1m10s" @@ -189,7 +189,7 @@ func TestServerConfig(t *testing.T) { } func TestServerConfig_DeprecateLongQueryTime(t *testing.T) { t.Skip("pilosa hosts config (cmd.Server.Config.Cluster.Hosts and brethren) is test only and will go away with high probability. skip for now.") - actualDataDir, err := ioutil.TempDir("", "") + actualDataDir, err := testhook.TempDir(t, "") failErr(t, err, "making data dir") tests := []commandTest{ diff --git a/ctl/check_test.go b/ctl/check_test.go index ab5ef9279..f5f466fd6 100644 --- a/ctl/check_test.go +++ b/ctl/check_test.go @@ -16,20 +16,22 @@ package ctl import ( "bytes" - "encoding/hex" "io" - "io/ioutil" - "math/rand" "os" - "path/filepath" "strings" "testing" "context" + + "github.com/pilosa/pilosa/v2/testhook" ) func TestCheckCommand_RunCacheFile(t *testing.T) { - cacheFile := TempFileName("test", ".cache") + fi, err := testhook.TempFile(t, "test*.cache") + if err != nil { + t.Fatalf("creating test file: %v", err) + } + cacheFile := fi.Name() rder := []byte{} stdin := bytes.NewReader(rder) @@ -37,7 +39,7 @@ func TestCheckCommand_RunCacheFile(t *testing.T) { cm := NewCheckCommand(stdin, w, w) cm.Paths = []string{cacheFile} - err := cm.Run(context.Background()) + err = cm.Run(context.Background()) w.Close() var buf bytes.Buffer if _, err := io.Copy(&buf, r); err != nil { @@ -50,7 +52,11 @@ func TestCheckCommand_RunCacheFile(t *testing.T) { } func TestCheckCommand_RunSnapshot(t *testing.T) { - snapshotFile := TempFileName("test", ".snapshotting") + fi, err := testhook.TempFile(t, "test*.snapshotting") + if err != nil { + t.Fatalf("creating test file: %v", err) + } + snapshotFile := fi.Name() rder := []byte{} stdin := bytes.NewReader(rder) @@ -58,7 +64,7 @@ func TestCheckCommand_RunSnapshot(t *testing.T) { cm := NewCheckCommand(stdin, w, w) cm.Paths = []string{snapshotFile} - err := cm.Run(context.Background()) + err = cm.Run(context.Background()) w.Close() var buf bytes.Buffer if _, err := io.Copy(&buf, r); err != nil { @@ -71,10 +77,11 @@ func TestCheckCommand_RunSnapshot(t *testing.T) { } func TestCheckCommand_Run(t *testing.T) { - file, err := ioutil.TempFile("", "") + file, err := testhook.TempFile(t, "run-command") if err != nil { t.Fatal(err) } + fname := file.Name() if _, err := file.Write([]byte("1234,1223")); err != nil { t.Fatalf("writing to temp file: %v", err) } @@ -84,7 +91,7 @@ func TestCheckCommand_Run(t *testing.T) { stdin := bytes.NewReader(rder) r, w, _ := os.Pipe() cm := NewCheckCommand(stdin, w, w) - cm.Paths = []string{file.Name()} + cm.Paths = []string{fname} err = cm.Run(context.Background()) w.Close() @@ -99,10 +106,3 @@ func TestCheckCommand_Run(t *testing.T) { } // Todo: need correct roaring file for happy path } - -// TempFileName generates a temporary filename with extension -func TempFileName(prefix, suffix string) string { - randBytes := make([]byte, 16) - rand.Read(randBytes) - return filepath.Join(os.TempDir(), prefix+hex.EncodeToString(randBytes)+suffix) -} diff --git a/ctl/import_test.go b/ctl/import_test.go index ff5db6295..8713e83ca 100644 --- a/ctl/import_test.go +++ b/ctl/import_test.go @@ -29,6 +29,7 @@ import ( "github.com/pilosa/pilosa/v2" "github.com/pilosa/pilosa/v2/test" + "github.com/pilosa/pilosa/v2/testhook" ) func TestImportCommand_Validation(t *testing.T) { @@ -58,7 +59,7 @@ func TestImportCommand_Basic(t *testing.T) { buf := bytes.Buffer{} stdin, stdout, stderr := GetIO(buf) cm := NewImportCommand(stdin, stdout, stderr) - file, err := ioutil.TempFile("", "import.csv") + file, err := testhook.TempFile(t, "import.csv") if err != nil { t.Fatalf("creating tempfile: %v", err) } @@ -90,7 +91,7 @@ func TestImportCommand_Basic(t *testing.T) { buf := bytes.Buffer{} stdin, stdout, stderr := GetIO(buf) cm := NewImportCommand(stdin, stdout, stderr) - file, err := ioutil.TempFile("", "import.csv") + file, err := testhook.TempFile(t, "import.csv") if err != nil { t.Fatalf("creating tempfile: %v", err) } @@ -123,7 +124,7 @@ func TestImportCommand_RunValue(t *testing.T) { buf := bytes.Buffer{} stdin, stdout, stderr := GetIO(buf) cm := NewImportCommand(stdin, stdout, stderr) - file, err := ioutil.TempFile("", "import-value.csv") + file, err := testhook.TempFile(t, "import-value.csv") if err != nil { t.Fatalf("creating tempfile: %v", err) } @@ -162,7 +163,7 @@ func TestImportCommand_RunValue(t *testing.T) { buf := bytes.Buffer{} stdin, stdout, stderr := GetIO(buf) cm := NewImportCommand(stdin, stdout, stderr) - file, err := ioutil.TempFile("", "import-value.csv") + file, err := testhook.TempFile(t, "import-value.csv") if err != nil { t.Fatalf("creating tempfile: %v", err) } @@ -207,7 +208,7 @@ func TestImportCommand_RunKeys(t *testing.T) { buf := bytes.Buffer{} stdin, stdout, stderr := GetIO(buf) cm := NewImportCommand(stdin, stdout, stderr) - file, err := ioutil.TempFile("", "import-key.csv") + file, err := testhook.TempFile(t, "import-key.csv") if err != nil { t.Fatal(err) } @@ -247,7 +248,7 @@ func TestImportCommand_KeyReplication(t *testing.T) { buf := bytes.Buffer{} stdin, stdout, stderr := GetIO(buf) cm := NewImportCommand(stdin, stdout, stderr) - file, err := ioutil.TempFile("", "import-key.csv") + file, err := testhook.TempFile(t, "import-key.csv") if err != nil { t.Fatal(err) } @@ -327,7 +328,7 @@ func TestImportCommand_RunValueKeys(t *testing.T) { buf := bytes.Buffer{} stdin, stdout, stderr := GetIO(buf) cm := NewImportCommand(stdin, stdout, stderr) - file, err := ioutil.TempFile("", "import-key.csv") + file, err := testhook.TempFile(t, "import-key.csv") if err != nil { t.Fatal(err) } @@ -373,7 +374,7 @@ func TestImportCommand_InvalidFile(t *testing.T) { cm.Host = cmd.API.Node().URI.HostPort() cm.Index = "i" cm.Field = "f" - file, err := ioutil.TempFile("", "import.csv") + file, err := testhook.TempFile(t, "import.csv") if err != nil { t.Fatalf("creating tempfile: %v", err) } @@ -387,7 +388,7 @@ func TestImportCommand_InvalidFile(t *testing.T) { t.Fatalf("expect error: invalid row id on row, actual: %s", err) } - file, err = ioutil.TempFile("", "import1.csv") + file, err = testhook.TempFile(t, "import1.csv") if err != nil { t.Fatalf("creating tempfile: %v", err) } @@ -401,7 +402,7 @@ func TestImportCommand_InvalidFile(t *testing.T) { t.Fatalf("expect error: invalid column id on row, actual: %s", err) } - file, err = ioutil.TempFile("", "import1.csv") + file, err = testhook.TempFile(t, "import1.csv") if err != nil { t.Fatal(err) } @@ -415,7 +416,7 @@ func TestImportCommand_InvalidFile(t *testing.T) { t.Fatalf("expect error: invalid timestamp on row, actual: %s", err) } - file, err = ioutil.TempFile("", "import1.csv") + file, err = testhook.TempFile(t, "import1.csv") if err != nil { t.Fatal(err) } @@ -458,7 +459,7 @@ func TestImportCommand_BugOverwriteValue(t *testing.T) { buf := bytes.Buffer{} stdin, stdout, stderr := GetIO(buf) cm := NewImportCommand(stdin, stdout, stderr) - file, err := ioutil.TempFile("", "import-value.csv") + file, err := testhook.TempFile(t, "import-value.csv") if err != nil { t.Fatal(err) } @@ -490,7 +491,7 @@ func TestImportCommand_BugOverwriteValue(t *testing.T) { } file.Close() - file, err = ioutil.TempFile("", "import-value2.csv") + file, err = testhook.TempFile(t, "import-value2.csv") if err != nil { t.Fatalf("Error creating tempfile: %s", err) } @@ -505,7 +506,7 @@ func TestImportCommand_BugOverwriteValue(t *testing.T) { } file.Close() - file, err = ioutil.TempFile("", "import-value3.csv") + file, err = testhook.TempFile(t, "import-value3.csv") if err != nil { t.Fatalf("Error creating tempfile: %s", err) } @@ -547,7 +548,7 @@ func TestImportCommand_RunBool(t *testing.T) { cm.Field = "f" t.Run("Valid", func(t *testing.T) { - file, err := ioutil.TempFile("", "import-bool.csv") + file, err := testhook.TempFile(t, "import-bool.csv") if err != nil { t.Fatal(err) } @@ -565,7 +566,7 @@ func TestImportCommand_RunBool(t *testing.T) { // Ensure that invalid bool values return an error. t.Run("Invalid", func(t *testing.T) { - file, err := ioutil.TempFile("", "import-invalid-bool.csv") + file, err := testhook.TempFile(t, "import-invalid-bool.csv") if err != nil { t.Fatal(err) } diff --git a/ctl/inspect_test.go b/ctl/inspect_test.go index 2698dcc03..97599dd68 100644 --- a/ctl/inspect_test.go +++ b/ctl/inspect_test.go @@ -18,10 +18,11 @@ import ( "bytes" "context" "io" - "io/ioutil" "os" "strings" "testing" + + "github.com/pilosa/pilosa/v2/testhook" ) func TestInspectCommand_Run(t *testing.T) { @@ -30,7 +31,7 @@ func TestInspectCommand_Run(t *testing.T) { r, w, _ := os.Pipe() cm := NewInspectCommand(stdin, w, w) - file, err := ioutil.TempFile("", "inspectTest") + file, err := testhook.TempFile(t, "inspectTest") if err != nil { t.Fatalf("Error creating tempfile: %s", err) } diff --git a/dbshard_internal_test.go b/dbshard_internal_test.go index 398ed7569..e56554319 100644 --- a/dbshard_internal_test.go +++ b/dbshard_internal_test.go @@ -16,7 +16,6 @@ package pilosa import ( "fmt" - "io/ioutil" "os" "path/filepath" "strings" @@ -25,6 +24,7 @@ import ( "github.com/pilosa/pilosa/v2/rbf" "github.com/pilosa/pilosa/v2/shardwidth" txkey "github.com/pilosa/pilosa/v2/short_txkey" + "github.com/pilosa/pilosa/v2/testhook" ) // Shard per db evaluation @@ -70,7 +70,7 @@ func TestShardPerDB_SetBit(t *testing.T) { // test that we find all *local* shards func Test_DBPerShard_GetShardsForIndex_LocalOnly(t *testing.T) { - tmpdir, err := ioutil.TempDir("", "Test_DBPerShard_GetShardsForIndex_LocalOnly") + tmpdir, err := testhook.TempDir(t, "Test_DBPerShard_GetShardsForIndex_LocalOnly") panicOn(err) defer os.RemoveAll(tmpdir) @@ -324,7 +324,7 @@ func makeTxTestDBWithViewsShards(holder *Holder, idx *Index, exp *FieldView2Shar // test that rbf can give us a map[view]*shardSet func Test_DBPerShard_GetFieldView2Shards_map_from_RBF(t *testing.T) { - tmpdir, err := ioutil.TempDir("", "Test_DBPerShard_GetFieldView2Shards_map_from_RBF") + tmpdir, err := testhook.TempDir(t, "Test_DBPerShard_GetFieldView2Shards_map_from_RBF") panicOn(err) defer os.RemoveAll(tmpdir) diff --git a/etcd/leasedkv_test.go b/etcd/leasedkv_test.go index 9c7965c2d..fb54fab93 100644 --- a/etcd/leasedkv_test.go +++ b/etcd/leasedkv_test.go @@ -17,12 +17,12 @@ package etcd import ( "context" "errors" - "io/ioutil" "os" "testing" "time" "github.com/pilosa/pilosa/v2/disco" + "github.com/pilosa/pilosa/v2/testhook" "go.etcd.io/etcd/embed" "go.etcd.io/etcd/etcdserver/api/v3client" ) @@ -33,7 +33,7 @@ const newVal = "newValue" func TestLeasedKv(t *testing.T) { cfg := embed.NewConfig() - dir, err := ioutil.TempDir("", "leasedkv-*") + dir, err := testhook.TempDir(t, "leasedkv-*") if err != nil { t.Fatal(err) } diff --git a/fragment_internal_test.go b/fragment_internal_test.go index 2d096640f..8471e0f28 100644 --- a/fragment_internal_test.go +++ b/fragment_internal_test.go @@ -3166,7 +3166,7 @@ func BenchmarkImportIntoLargeFragment(b *testing.B) { if err != nil { b.Fatalf("opening frag file: %v", err) } - fi, err := ioutil.TempFile(*TempDir, "") + fi, err := testhook.TempFileInDir(b, *TempDir, "") if err != nil { b.Fatalf("getting temp file: %v", err) } @@ -3216,7 +3216,7 @@ func BenchmarkImportRoaringIntoLargeFragment(b *testing.B) { if err != nil { b.Fatalf("opening frag file: %v", err) } - fi, err := ioutil.TempFile(*TempDir, "") + fi, err := testhook.TempFileInDir(b, *TempDir, "") if err != nil { b.Fatalf("getting temp file: %v", err) } @@ -3472,6 +3472,10 @@ func BenchmarkFileWrite(b *testing.B) { b.Run(fmt.Sprintf("Rows%d", numRows), func(b *testing.B) { b.StopTimer() for i := 0; i < b.N; i++ { + // DO NOT CONVERT THIS ONE TO USE TESTHOOK. + // We're deleting these files as we go because + // otherwise the benchmark could fill up + // $TMPDIR before it finishes running. f, err := ioutil.TempFile(*TempDir, "") if err != nil { b.Fatalf("getting temp file: %v", err) @@ -3479,14 +3483,17 @@ func BenchmarkFileWrite(b *testing.B) { b.StartTimer() _, err = f.Write(data) if err != nil { + os.Remove(f.Name()) b.Fatal(err) } err = f.Sync() if err != nil { + os.Remove(f.Name()) b.Fatal(err) } err = f.Close() if err != nil { + os.Remove(f.Name()) b.Fatal(err) } b.StopTimer() diff --git a/idalloc_test.go b/idalloc_test.go index 5c6d4103c..cc2ee49af 100644 --- a/idalloc_test.go +++ b/idalloc_test.go @@ -17,18 +17,17 @@ package pilosa import ( "crypto/rand" "io" - "io/ioutil" - "os" "reflect" "testing" "time" + "github.com/pilosa/pilosa/v2/testhook" bolt "go.etcd.io/bbolt" ) func TestIDAlloc(t *testing.T) { // Acquire a temporary file. - f, err := ioutil.TempFile("", "") + f, err := testhook.TempFile(t, "idalloc") if err != nil { t.Errorf("acquiring temporary file: %v", err) return @@ -42,10 +41,6 @@ func TestIDAlloc(t *testing.T) { // Open bolt. db, err := bolt.Open(f.Name(), 0666, &bolt.Options{Timeout: 1 * time.Second}) - if rerr := os.Remove(f.Name()); rerr != nil { - t.Errorf("removing temporary file: %v", rerr) - return - } if err != nil { t.Errorf("opening bolt: %v", err) return diff --git a/logger/filewriter_test.go b/logger/filewriter_test.go index 8351e580d..837a36be4 100644 --- a/logger/filewriter_test.go +++ b/logger/filewriter_test.go @@ -29,6 +29,8 @@ import ( "io/ioutil" "os" "testing" + + "github.com/pilosa/pilosa/v2/testhook" ) // TestReopenAppend -- make sure we always append to an existing file @@ -42,16 +44,12 @@ import ( // 5. read file, make sure it contains line0,line1,line2 // func TestReopenAppend(t *testing.T) { - // TODO fix - // (travis) I have no idea what this TODO is asking for. - // Perhaps use `ioutil.TempFile()`? - var fname = "/tmp/foo" - - // Step 1 -- Create a sample file using normal means - forig, err := os.Create(fname) + forig, err := testhook.TempFile(t, "logger-reopen") if err != nil { - t.Fatalf("Unable to create initial file %s: %s", fname, err) + t.Fatalf("unable to create initial file: %v", err) } + fname := forig.Name() + _, err = forig.Write([]byte("line0\n")) if err != nil { t.Fatalf("Unable to write initial line %s: %s", fname, err) @@ -96,26 +94,23 @@ func TestReopenAppend(t *testing.T) { } } -// Test that reopen works when Inode is swapped out -// 1. Create a sample file using normal means -// 2. Open a ioreopen.File -// write line 1 -// 3. call Reopen -// write line 2 -// 4. close file -// 5. read file, make sure it contains line0,line1,line2 +// Test that reopen works when Inode is swapped out. That is to say, +// if the previous file has been renamed, we want to get a new file +// that isn't the old one, not keep writing to the old file. // +// 1. Create a sample file using normal means +// 2. Write to it. +// 3. Rename it. +// 4. Call reopen. +// 5. Write line 2. +// 6. Read file, expecting to see only line 2. func TestChangeInode(t *testing.T) { - // TODO fix - // (travis) I have no idea what this TODO is asking for. - // Perhaps use `ioutil.TempFile()`? - var fname = "/tmp/foo" - // Step 1 -- Create a empty sample file - forig, err := os.Create(fname) + forig, err := testhook.TempFile(t, "changeInode") if err != nil { - t.Fatalf("Unable to create initial file %s: %s", fname, err) + t.Fatalf("Unable to create initial file: %s", err) } + fname := forig.Name() err = forig.Close() if err != nil { t.Fatalf("Unable to close initial file: %s", err) @@ -136,6 +131,8 @@ func TestChangeInode(t *testing.T) { if err != nil { t.Errorf("Renaming error: %s", err) } + // remove the scratch file + defer os.Remove(fname + ".orig") _, err = f.Write([]byte("after1\n")) if err != nil { t.Errorf("Write error: %s", err) diff --git a/rbf/db_test.go b/rbf/db_test.go index 54a2d1c39..431062a84 100644 --- a/rbf/db_test.go +++ b/rbf/db_test.go @@ -32,7 +32,7 @@ import ( ) func TestDB_Open(t *testing.T) { - db := NewDB() + db := NewDB(t) if err := db.Open(); err != nil { t.Fatal(err) } else if err := db.Close(); err != nil { diff --git a/rbf/ingest_test.go b/rbf/ingest_test.go index 6dcfdc9e6..966caead7 100644 --- a/rbf/ingest_test.go +++ b/rbf/ingest_test.go @@ -16,17 +16,19 @@ package rbf import ( "fmt" - "io/ioutil" "os" "path/filepath" "runtime" "runtime/pprof" "strings" "testing" + //"time" "github.com/pilosa/pilosa/v2/rbf/cfg" "github.com/pilosa/pilosa/v2/roaring" + "github.com/pilosa/pilosa/v2/testhook" + // "github.com/pilosa/pilosa/v2/txkey" txkey "github.com/pilosa/pilosa/v2/short_txkey" ) @@ -71,7 +73,7 @@ func TestIngest_lots_of_views(t *testing.T) { //vv("m1.TotalAlloc = %v", m1.TotalAlloc) }() - path, err := ioutil.TempDir("", "rbf_ingest_lots_of_views") + path, err := testhook.TempDir(t, "rbf_ingest_lots_of_views") panicOn(err) defer os.Remove(path) diff --git a/rbf/rbf_test.go b/rbf/rbf_test.go index 082ad4294..f6b1264df 100644 --- a/rbf/rbf_test.go +++ b/rbf/rbf_test.go @@ -18,7 +18,6 @@ import ( "flag" "fmt" "io" - "io/ioutil" "math/rand" "os" "runtime" @@ -27,6 +26,7 @@ import ( "github.com/pilosa/pilosa/v2/rbf" rbfcfg "github.com/pilosa/pilosa/v2/rbf/cfg" + "github.com/pilosa/pilosa/v2/testhook" ) var quickCheckN *int = flag.Int("quickchecks", 10, "The number of iterations for each quickcheck") @@ -61,8 +61,8 @@ func TestReadWriteRootRecord(t *testing.T) { } // NewDB returns a new instance of DB with a temporary path. -func NewDB(cfg ...*rbfcfg.Config) *rbf.DB { - path, err := ioutil.TempDir("", "") +func NewDB(tb testing.TB, cfg ...*rbfcfg.Config) *rbf.DB { + path, err := testhook.TempDir(tb, "rbfdb") if err != nil { panic(err) } @@ -78,7 +78,7 @@ func NewDB(cfg ...*rbfcfg.Config) *rbf.DB { // MustOpenDB returns a db opened on a temporary file. On error, fail test. func MustOpenDB(tb testing.TB, cfg ...*rbfcfg.Config) *rbf.DB { tb.Helper() - db := NewDB(cfg...) + db := NewDB(tb, cfg...) if err := db.Open(); err != nil { tb.Fatal(err) } diff --git a/rbf/util_test.go b/rbf/util_test.go index 10aff5a50..1af2d02ce 100644 --- a/rbf/util_test.go +++ b/rbf/util_test.go @@ -16,12 +16,12 @@ package rbf import ( "fmt" "io" - "io/ioutil" "os" "testing" rbfcfg "github.com/pilosa/pilosa/v2/rbf/cfg" "github.com/pilosa/pilosa/v2/roaring" + "github.com/pilosa/pilosa/v2/testhook" ) // util_test adds reusable utilities for testing. @@ -144,7 +144,7 @@ func verifyElemNBitN(tx *Tx, lc leafCell) { func testHelperMustOpenNewDB(tb testing.TB, cfg ...*rbfcfg.Config) *DB { tb.Helper() - path, err := ioutil.TempDir("", "") + path, err := testhook.TempDir(tb, "rbfdb") if err != nil { panic(err) } diff --git a/server/cluster_test.go b/server/cluster_test.go index 469c9302d..df346fc84 100644 --- a/server/cluster_test.go +++ b/server/cluster_test.go @@ -192,7 +192,7 @@ func TestClusterResize_AddNode(t *testing.T) { } lsns[i] = l.(*net.TCPListener) } - portsCfg := test.GenPortsConfig(test.NewPorts(lsns)) + portsCfg := test.GenPortsConfig(t, test.NewPorts(lsns)) m1.Config.Etcd = portsCfg[0].Etcd m1.Config.Name = portsCfg[0].Name diff --git a/test/disco.go b/test/disco.go index 9b9a8fe5a..ade6a2b2a 100644 --- a/test/disco.go +++ b/test/disco.go @@ -16,7 +16,6 @@ package test import ( "fmt" - "io/ioutil" "net" "strings" "testing" @@ -121,7 +120,7 @@ func GetPortsGenConfigs(tb testing.TB, nodes []*Command) error { } //GenPortsConfig creates specific configuration for etcd. -func GenPortsConfig(ports []Ports) []*server.Config { +func GenPortsConfig(tb testing.TB, ports []Ports) []*server.Config { cfgs := make([]*server.Config, len(ports)) clusterURLs := make([]string, len(ports)) for i := range cfgs { @@ -134,7 +133,7 @@ func GenPortsConfig(ports []Ports) []*server.Config { lPeerURL := fmt.Sprintf("http://localhost:%d", portP) discoDir := "" - if d, err := ioutil.TempDir("", "disco."); err == nil { + if d, err := testhook.TempDir(tb, "disco."); err == nil { discoDir = d } diff --git a/testhook/hook.go b/testhook/hook.go index a478a7cbb..afa036f84 100644 --- a/testhook/hook.go +++ b/testhook/hook.go @@ -87,12 +87,27 @@ func TempDir(tb testing.TB, pattern string) (path string, err error) { if err == nil { Cleanup(tb, func() { os.RemoveAll(path) - fmt.Println("--- testhook:", path, tb.Name()) + // fmt.Println("--- testhook: cleaning up dir", path, tb.Name()) }) } return path, err } +// TempFile creates a temp file that will be automatically deleted when +// this test completes, using go1.14's [TB].Cleanup() if available. +func TempFile(tb testing.TB, pattern string) (file *os.File, err error) { + file, err = ioutil.TempFile("", pattern) + if err == nil { + path := file.Name() + Cleanup(tb, func() { + file.Close() + os.Remove(path) + // fmt.Println("--- testhook: cleaning up file", path, tb.Name()) + }) + } + return file, err +} + // TempDirInDir creates a temp directory that will be automatically deleted when // this test completes, using go1.14's [TB].Cleanup(), but with a specified // path instead of the default Go TMPDIR. Only some tests use this, which is @@ -106,3 +121,19 @@ func TempDirInDir(tb testing.TB, dir string, pattern string) (path string, err e } return path, err } + +// TempFileInDir creates a temp file that will be automatically deleted when +// this test completes, using go1.14's [TB].Cleanup(), but with a specified +// path instead of the default Go TMPDIR. Only some tests use this, which is +// possibly an error... +func TempFileInDir(tb testing.TB, dir string, pattern string) (file *os.File, err error) { + file, err = ioutil.TempFile(dir, pattern) + if err == nil { + path := file.Name() + Cleanup(tb, func() { + file.Close() + os.Remove(path) + }) + } + return file, err +} From ef8dd91077f05cabaef0e40f6d53e318fb1b88ae Mon Sep 17 00:00:00 2001 From: Seebs Date: Wed, 10 Mar 2021 16:43:02 -0600 Subject: [PATCH 2/2] report errors more clearly when trying to recreate leases There's some loose ends here because really we probably want to be using the top-level server logger, and we should fix that, but in the mean time, let's not swallow the errors as much, because the last line printed doesn't actually show what the error was, but it could. To do this, we distinguish between the current error (which might be a wrapper around DeadlineExceeded) and a previous error which we might prefer to return, if one exists, since it's more likely the "real" cause. --- etcd/leasedkv.go | 31 ++++++++++++++++--------------- 1 file changed, 16 insertions(+), 15 deletions(-) diff --git a/etcd/leasedkv.go b/etcd/leasedkv.go index 3f0703b64..858c000d8 100644 --- a/etcd/leasedkv.go +++ b/etcd/leasedkv.go @@ -100,7 +100,7 @@ func (l *leasedKV) consumeLease(ch <-chan *clientv3.LeaseKeepAliveResponse) { return } - if ok := retry(1*time.Second, func() error { + if e := retry("consumeLease", 1*time.Second, func() error { kaChann, err := l.create(l.value) if err != nil { return err @@ -108,13 +108,13 @@ func (l *leasedKV) consumeLease(ch <-chan *clientv3.LeaseKeepAliveResponse) { go l.consumeLease(kaChann) return nil - }); !ok { - log.Println("lease cannot be recreated. Key:", l.key) + }); e != nil { + log.Printf("lease %q cannot be recreated: %v", l.key, e) l.mu.Unlock() return } - log.Println("lease recreated after a problem. Key:", l.key) + log.Printf("lease %q recreated after a problem", l.key) l.mu.Unlock() return } @@ -172,20 +172,21 @@ func (l *leasedKV) Get(ctx context.Context) (string, error) { return l.value, nil } -func retry(sleep time.Duration, f func() error) bool { +func retry(desc string, sleep time.Duration, f func() error) (err error) { for { - err := f() - if err == nil { - return true + lastErr := f() + if lastErr == nil { + return lastErr } - - // sometimes the element in charge of stopping the lease renewal doesn't do it, causing context errors. - if errors.Is(err, context.DeadlineExceeded) { - return false + if errors.Is(lastErr, context.DeadlineExceeded) { + if err != nil { + return err + } else { + return lastErr + } } - + log.Printf("%s: got error %v, retrying", desc, lastErr) + err = lastErr time.Sleep(sleep) - - log.Println("retrying after error:", err) } }