From e05f165ac12cb54faad707548b8b8f2005f2eef6 Mon Sep 17 00:00:00 2001 From: Tim Pierce Date: Fri, 25 Apr 2014 16:58:32 -0400 Subject: [PATCH] Cleaned up unit tests. (refs #2620) Added a MockVolume implementation to use in unit tests for the Keep front-end handlers. Simplified IsValidLocator and keep_test.go:setup code. --- services/keep/src/keep/keep.go | 11 ++- services/keep/src/keep/keep_test.go | 134 ++++++++++++-------------- services/keep/src/keep/volume.go | 4 +- services/keep/src/keep/volume_test.go | 18 ++-- 4 files changed, 81 insertions(+), 86 deletions(-) diff --git a/services/keep/src/keep/keep.go b/services/keep/src/keep/keep.go index 9e0dfec23e..51dde9463f 100644 --- a/services/keep/src/keep/keep.go +++ b/services/keep/src/keep/keep.go @@ -92,7 +92,7 @@ func main() { "interface on which to listen for requests, in the format ipaddr:port. e.g. -listen=10.0.1.24:8000. Use -listen=:port to listen on all network interfaces.") flag.StringVar(&volumearg, "volumes", "", "Comma-separated list of directories to use for Keep volumes, e.g. -volumes=/var/keep1,/var/keep2. If empty or not supplied, Keep will scan mounted filesystems for volumes with a /keep top-level directory.") - flag.BoolVar(&serialize_io, "serialize-io", false, + flag.BoolVar(&serialize_io, "serialize", false, "If set, all read and write operations on local Keep volumes will be serialized.") flag.Parse() @@ -445,6 +445,11 @@ func ReadAtMost(r io.Reader, maxbytes int) ([]byte, error) { // When Keep is extended to support hash types other than MD5, // this should be updated to cover those as well. // -func IsValidLocator(loc string) (bool, error) { - return regexp.MatchString(`^[0-9a-f]{32}$`, loc) +func IsValidLocator(loc string) bool { + match, err := regexp.MatchString(`^[0-9a-f]{32}$`, loc) + if err == nil { + return match + } + log.Printf("IsValidLocator: %s\n", err) + return false } diff --git a/services/keep/src/keep/keep_test.go b/services/keep/src/keep/keep_test.go index 6c82920853..5b7c3c7b34 100644 --- a/services/keep/src/keep/keep_test.go +++ b/services/keep/src/keep/keep_test.go @@ -37,9 +37,6 @@ var BAD_BLOCK = []byte("The magic words are squeamish ossifrage.") // - use an interface to mock ioutil.TempFile with a File // object that always returns an error on write // -// TODO(twp): Make these tests less dependent on being able to access -// the UnixVolume root field. -// // ======================================== // GetBlock tests. // ======================================== @@ -51,8 +48,10 @@ func TestGetBlock(t *testing.T) { defer teardown() // Prepare two test Keep volumes. Our block is stored on the second volume. - KeepVolumes = setup(t, 2) - store(t, KeepVolumes[1], TEST_HASH, TEST_BLOCK) + KeepVolumes = setup(2) + if err := KeepVolumes[1].Put(TEST_HASH, TEST_BLOCK); err != nil { + t.Error(err) + } // Check that GetBlock returns success. result, err := GetBlock(TEST_HASH) @@ -71,7 +70,7 @@ func TestGetBlockMissing(t *testing.T) { defer teardown() // Create two empty test Keep volumes. - KeepVolumes = setup(t, 2) + KeepVolumes = setup(2) // Check that GetBlock returns failure. result, err := GetBlock(TEST_HASH) @@ -87,12 +86,9 @@ func TestGetBlockMissing(t *testing.T) { func TestGetBlockCorrupt(t *testing.T) { defer teardown() - // Create two test Keep volumes and store a block in each of them, - // but the hash of the block does not match the filename. - KeepVolumes = setup(t, 2) - for _, vol := range KeepVolumes { - store(t, vol, TEST_HASH, BAD_BLOCK) - } + // Create two test Keep volumes and store a corrupt block in one. + KeepVolumes = setup(2) + KeepVolumes[0].Put(TEST_HASH, BAD_BLOCK) // Check that GetBlock returns failure. result, err := GetBlock(TEST_HASH) @@ -112,20 +108,19 @@ func TestPutBlockOK(t *testing.T) { defer teardown() // Create two test Keep volumes. - KeepVolumes = setup(t, 2) + KeepVolumes = setup(2) // Check that PutBlock stores the data as expected. if err := PutBlock(TEST_BLOCK, TEST_HASH); err != nil { t.Fatalf("PutBlock: %v", err) } - result, err := GetBlock(TEST_HASH) + result, err := KeepVolumes[0].Get(TEST_HASH) if err != nil { - t.Fatalf("GetBlock returned error: %v", err) + t.Fatalf("KeepVolumes[0].Get returned error: %v", err) } if string(result) != string(TEST_BLOCK) { - t.Error("PutBlock/GetBlock mismatch") - t.Fatalf("PutBlock stored '%s', GetBlock retrieved '%s'", + t.Fatalf("PutBlock stored '%s', Get retrieved '%s'", string(TEST_BLOCK), string(result)) } } @@ -138,8 +133,8 @@ func TestPutBlockOneVol(t *testing.T) { defer teardown() // Create two test Keep volumes, but cripple one of them. - KeepVolumes = setup(t, 2) - os.Chmod(KeepVolumes[0].(*UnixVolume).root, 000) + KeepVolumes = setup(2) + KeepVolumes[0].(*MockVolume).Bad = true // Check that PutBlock stores the data as expected. if err := PutBlock(TEST_BLOCK, TEST_HASH); err != nil { @@ -165,7 +160,7 @@ func TestPutBlockMD5Fail(t *testing.T) { defer teardown() // Create two test Keep volumes. - KeepVolumes = setup(t, 2) + KeepVolumes = setup(2) // Check that PutBlock returns the expected error when the hash does // not match the block. @@ -188,10 +183,10 @@ func TestPutBlockCorrupt(t *testing.T) { defer teardown() // Create two test Keep volumes. - KeepVolumes = setup(t, 2) + KeepVolumes = setup(2) // Store a corrupted block under TEST_HASH. - store(t, KeepVolumes[0], TEST_HASH, BAD_BLOCK) + KeepVolumes[0].Put(TEST_HASH, BAD_BLOCK) if err := PutBlock(TEST_BLOCK, TEST_HASH); err != nil { t.Errorf("PutBlock: %v", err) } @@ -216,11 +211,14 @@ func TestPutBlockCollision(t *testing.T) { var b2 = []byte("\x0e0eaU\x9a\xa7\x87\xd0\x0b\xc6\xf7\x0b\xbd\xfe4\x04\xcf\x03e\x9etO\x854\xc0\x0f\xfbe\x9cL\x87@\xcc\x94/\xeb-\xa1\x15\xa3\xf4\x15\xdc\xbb\x86\x07Is\x86em}\x1f4\xa4 Y\xd7\x8fZ\x8d\xd1\xef") var locator = "cee9a457e790cf20d4bdaa6d69f01e41" - // Prepare two test Keep volumes. Store one block, - // then attempt to store the other. - KeepVolumes = setup(t, 2) - store(t, KeepVolumes[1], locator, b1) + // Prepare two test Keep volumes. + KeepVolumes = setup(2) + // Store one block, then attempt to store the other. Confirm that + // PutBlock reported a CollisionError. + if err := PutBlock(b1, locator); err != nil { + t.Error(err) + } if err := PutBlock(b2, locator); err == nil { t.Error("PutBlock did not report a collision") } else if err != CollisionError { @@ -237,15 +235,30 @@ func TestPutBlockCollision(t *testing.T) { // directories at the top level. // func TestFindKeepVolumes(t *testing.T) { - defer teardown() + var tempVols [2]string + var err error + + defer func() { + for _, path := range tempVols { + os.RemoveAll(path) + } + }() - // Initialize two keep volumes. - var tempVols []Volume = setup(t, 2) + // Create two directories suitable for using as keep volumes. + for i := range tempVols { + if tempVols[i], err = ioutil.TempDir("", "findvol"); err != nil { + t.Fatal(err) + } + tempVols[i] = tempVols[i] + "/keep" + if err = os.Mkdir(tempVols[i], 0755); err != nil { + t.Fatal(err) + } + } // Set up a bogus PROC_MOUNTS file. if f, err := ioutil.TempFile("", "keeptest"); err == nil { for _, vol := range tempVols { - fmt.Fprintf(f, "tmpfs %s tmpfs opts\n", path.Dir(vol.(*UnixVolume).root)) + fmt.Fprintf(f, "tmpfs %s tmpfs opts\n", path.Dir(vol)) } f.Close() PROC_MOUNTS = f.Name() @@ -257,10 +270,9 @@ func TestFindKeepVolumes(t *testing.T) { len(tempVols), len(resultVols)) } for i := range tempVols { - tempVolRoot := tempVols[i].(*UnixVolume).root - if tempVolRoot != resultVols[i] { + if tempVols[i] != resultVols[i] { t.Errorf("FindKeepVolumes returned %s, expected %s\n", - tempVolRoot, tempVols[i]) + resultVols[i], tempVols[i]) } } @@ -302,12 +314,12 @@ func TestIndex(t *testing.T) { // Set up Keep volumes and populate them. // Include multiple blocks on different volumes, and // some metadata files. - KeepVolumes = setup(t, 2) - store(t, KeepVolumes[0], TEST_HASH, TEST_BLOCK) - store(t, KeepVolumes[1], TEST_HASH_2, TEST_BLOCK_2) - store(t, KeepVolumes[0], TEST_HASH_3, TEST_BLOCK_3) - store(t, KeepVolumes[0], TEST_HASH+".meta", []byte("metadata")) - store(t, KeepVolumes[1], TEST_HASH_2+".meta", []byte("metadata")) + KeepVolumes = setup(2) + KeepVolumes[0].Put(TEST_HASH, TEST_BLOCK) + KeepVolumes[1].Put(TEST_HASH_2, TEST_BLOCK_2) + KeepVolumes[0].Put(TEST_HASH_3, TEST_BLOCK_3) + KeepVolumes[0].Put(TEST_HASH+".meta", []byte("metadata")) + KeepVolumes[1].Put(TEST_HASH_2+".meta", []byte("metadata")) index := KeepVolumes[0].Index("") + KeepVolumes[1].Index("") expected := `^` + TEST_HASH + `\+\d+ \d+\n` + @@ -333,16 +345,18 @@ func TestIndex(t *testing.T) { func TestNodeStatus(t *testing.T) { defer teardown() - // Set up test Keep volumes. - KeepVolumes = setup(t, 2) + // Set up test Keep volumes with some blocks. + KeepVolumes = setup(2) + KeepVolumes[0].Put(TEST_HASH, TEST_BLOCK) + KeepVolumes[1].Put(TEST_HASH_2, TEST_BLOCK_2) // Get node status and make a basic sanity check. st := GetNodeStatus() - for i, vol := range KeepVolumes { + for i := range KeepVolumes { volinfo := st.Volumes[i] mtp := volinfo.MountPoint - if mtp != vol.(*UnixVolume).root { - t.Errorf("GetNodeStatus mount_point %s != KeepVolume %s", mtp, vol) + if mtp != "/bogo" { + t.Errorf("GetNodeStatus mount_point %s, expected /bogo", mtp) } if volinfo.DeviceNum == 0 { t.Errorf("uninitialized device_num in %v", volinfo) @@ -364,16 +378,10 @@ func TestNodeStatus(t *testing.T) { // Create KeepVolumes for testing. // Returns a slice of pathnames to temporary Keep volumes. // -func setup(t *testing.T, num_volumes int) []Volume { +func setup(num_volumes int) []Volume { vols := make([]Volume, num_volumes) for i := range vols { - if dir, err := ioutil.TempDir(os.TempDir(), "keeptest"); err == nil { - root := dir + "/keep" - vols[i] = &UnixVolume{root, nil} - os.Mkdir(root, 0755) - } else { - t.Fatal(err) - } + vols[i] = CreateMockVolume() } return vols } @@ -382,27 +390,5 @@ func setup(t *testing.T, num_volumes int) []Volume { // Cleanup to perform after each test. // func teardown() { - for _, vol := range KeepVolumes { - os.RemoveAll(path.Dir(vol.(*UnixVolume).root)) - } KeepVolumes = nil } - -// store -// Low-level code to write Keep blocks directly to disk for testing. -// Note: works only on UnixVolumes. -// -func store(t *testing.T, vol Volume, filename string, block []byte) { - blockdir := fmt.Sprintf("%s/%s", vol.(*UnixVolume).root, filename[:3]) - if err := os.MkdirAll(blockdir, 0755); err != nil { - t.Fatal(err) - } - - blockpath := fmt.Sprintf("%s/%s", blockdir, filename) - if f, err := os.Create(blockpath); err == nil { - f.Write(block) - f.Close() - } else { - t.Fatal(err) - } -} diff --git a/services/keep/src/keep/volume.go b/services/keep/src/keep/volume.go index 425fcfd470..f2f61a9059 100644 --- a/services/keep/src/keep/volume.go +++ b/services/keep/src/keep/volume.go @@ -239,9 +239,7 @@ func (v *UnixVolume) Index(prefix string) (output string) { return filepath.SkipDir } // Skip any file that is not apparently a locator, e.g. .meta files - if is_valid, err := IsValidLocator(locator); err != nil { - return err - } else if !is_valid { + if !IsValidLocator(locator) { return nil } // Print filenames beginning with prefix diff --git a/services/keep/src/keep/volume_test.go b/services/keep/src/keep/volume_test.go index e1c628e940..f4c204df65 100644 --- a/services/keep/src/keep/volume_test.go +++ b/services/keep/src/keep/volume_test.go @@ -100,13 +100,19 @@ func TestPutBadVolume(t *testing.T) { } } -// Serialization tests. +// Serialization tests: launch a bunch of concurrent // -// TODO(twp): a proper test of I/O serialization requires that -// a second request start while the first one is still executing. -// Doing this correctly requires some tricky synchronization. -// For now we'll just launch a bunch of requests in goroutines -// and demonstrate that they return accurate results. +// TODO(twp): show that the underlying Read/Write operations executed +// serially and not concurrently. The easiest way to do this is +// probably to activate verbose or debug logging, capture log output +// and examine it to confirm that Reads and Writes did not overlap. +// +// TODO(twp): a proper test of I/O serialization requires that a +// second request start while the first one is still underway. +// Guaranteeing that the test behaves this way requires some tricky +// synchronization and mocking. For now we'll just launch a bunch of +// requests simultaenously in goroutines and demonstrate that they +// return accurate results. // func TestGetSerialized(t *testing.T) { v := TempUnixVolume(t, make(chan *IORequest)) -- 2.30.2