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.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) } } 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 +}