test/testutil,services: favor NewRandGenerator over InitRandGenerator
As mentioned in the newly added comment in rand.go, using the global singleton
is confusing and can lead to subtle cross-test interference. It's simpler and
more straightforward to create and use random objects explicitly.
This change makes it possible to do so and changes binarylib and
deviced/impl/internal to the new scheme.
Change-Id: I0193a79e30f9f510e13165e228f65a450211a3db
diff --git a/services/device/deviced/internal/impl/globsuid/signature_match_test.go b/services/device/deviced/internal/impl/globsuid/signature_match_test.go
index 8f7c3c5..8dbd380 100644
--- a/services/device/deviced/internal/impl/globsuid/signature_match_test.go
+++ b/services/device/deviced/internal/impl/globsuid/signature_match_test.go
@@ -28,9 +28,9 @@
)
func TestDownloadSignatureMatch(t *testing.T) {
- testutil.InitRandGenerator(t.Logf)
ctx, shutdown := test.V23Init()
defer shutdown()
+ rg := testutil.NewRandGenerator(t.Logf)
sh, deferFn := servicetest.CreateShellAndMountTable(t, ctx, nil)
defer deferFn()
@@ -39,7 +39,7 @@
pkgVON := naming.Join(binaryVON, "testpkg")
defer utiltest.StartRealBinaryRepository(t, ctx, binaryVON)()
- up := testutil.RandomBytes(testutil.RandomIntn(5 << 20))
+ up := rg.RandomBytes(rg.RandomIntn(5 << 20))
mediaInfo := repository.MediaInfo{Type: "application/octet-stream"}
sig, err := binarylib.Upload(ctx, naming.Join(binaryVON, "testbinary"), up, mediaInfo)
if err != nil {
@@ -52,7 +52,7 @@
t.Fatalf("ioutil.TempDir failed: %v", err)
}
defer os.RemoveAll(tmpdir)
- pkgContents := testutil.RandomBytes(testutil.RandomIntn(5 << 20))
+ pkgContents := rg.RandomBytes(rg.RandomIntn(5 << 20))
if err := ioutil.WriteFile(filepath.Join(tmpdir, "pkg.txt"), pkgContents, 0600); err != nil {
t.Fatalf("ioutil.WriteFile failed: %v", err)
}
diff --git a/services/internal/binarylib/client_test.go b/services/internal/binarylib/client_test.go
index 000e8ca..7c83ca2 100644
--- a/services/internal/binarylib/client_test.go
+++ b/services/internal/binarylib/client_test.go
@@ -73,13 +73,13 @@
// TestBufferAPI tests the binary repository client-side library
// interface using buffers.
func TestBufferAPI(t *testing.T) {
- testutil.InitRandGenerator(t.Logf)
ctx, shutdown := test.V23Init()
defer shutdown()
+ rg := testutil.NewRandGenerator(t.Logf)
von, cleanup := setupRepository(t, ctx)
defer cleanup()
- data := testutil.RandomBytes(testutil.RandomIntn(10 << 20))
+ data := rg.RandomBytes(rg.RandomIntn(10 << 20))
mediaInfo := repository.MediaInfo{Type: "application/octet-stream"}
sig, err := Upload(ctx, von, data, mediaInfo)
if err != nil {
@@ -125,14 +125,14 @@
// TestFileAPI tests the binary repository client-side library
// interface using files.
func TestFileAPI(t *testing.T) {
- testutil.InitRandGenerator(t.Logf)
ctx, shutdown := test.V23Init()
defer shutdown()
+ rg := testutil.NewRandGenerator(t.Logf)
von, cleanup := setupRepository(t, ctx)
defer cleanup()
// Create up to 10MB of random bytes.
- data := testutil.RandomBytes(testutil.RandomIntn(10 << 20))
+ data := rg.RandomBytes(rg.RandomIntn(10 << 20))
dir, prefix := "", ""
src, err := ioutil.TempFile(dir, prefix)
if err != nil {
diff --git a/services/internal/binarylib/http_test.go b/services/internal/binarylib/http_test.go
index 73b253f..198cb75 100644
--- a/services/internal/binarylib/http_test.go
+++ b/services/internal/binarylib/http_test.go
@@ -21,9 +21,9 @@
// TestHTTP checks that HTTP download works.
func TestHTTP(t *testing.T) {
- testutil.InitRandGenerator(t.Logf)
ctx, shutdown := test.V23Init()
defer shutdown()
+ rg := testutil.NewRandGenerator(t.Logf)
// TODO(caprita): This is based on TestMultiPart (impl_test.go). Share
// the code where possible.
@@ -34,8 +34,8 @@
data := make([][]byte, length)
for i := 0; i < length; i++ {
// Random size, but at least 1 (avoid empty parts).
- size := testutil.RandomIntn(1000*binarylib.BufferLength) + 1
- data[i] = testutil.RandomBytes(size)
+ size := rg.RandomIntn(1000*binarylib.BufferLength) + 1
+ data[i] = rg.RandomBytes(size)
}
mediaInfo := repository.MediaInfo{Type: "application/octet-stream"}
if err := binary.Create(ctx, int32(length), mediaInfo); err != nil {
diff --git a/services/internal/binarylib/impl_test.go b/services/internal/binarylib/impl_test.go
index a614217..1ffd9a6 100644
--- a/services/internal/binarylib/impl_test.go
+++ b/services/internal/binarylib/impl_test.go
@@ -79,11 +79,12 @@
func TestHierarchy(t *testing.T) {
ctx, shutdown := test.V23Init()
defer shutdown()
+ rg := testutil.NewRandGenerator(t.Logf)
for i := 0; i < md5.Size; i++ {
binary, ep, _, cleanup := startServer(t, ctx, i)
defer cleanup()
- data := testData()
+ data := testData(rg)
// Test the binary repository interface.
if err := binary.Create(ctx, 1, repository.MediaInfo{Type: "application/octet-stream"}); err != nil {
@@ -131,6 +132,7 @@
func TestMultiPart(t *testing.T) {
ctx, shutdown := test.V23Init()
defer shutdown()
+ rg := testutil.NewRandGenerator(t.Logf)
for length := 2; length < 5; length++ {
binary, _, _, cleanup := startServer(t, ctx, 2)
@@ -138,7 +140,7 @@
// Create <length> chunks of up to 4MB of random bytes.
data := make([][]byte, length)
for i := 0; i < length; i++ {
- data[i] = testData()
+ data[i] = testData(rg)
}
// Test the binary repository interface.
if err := binary.Create(ctx, int32(length), repository.MediaInfo{Type: "application/octet-stream"}); err != nil {
@@ -181,9 +183,9 @@
// resumption ranging the number of parts the uploaded binary consists
// of.
func TestResumption(t *testing.T) {
- testutil.InitRandGenerator(t.Logf)
ctx, shutdown := test.V23Init()
defer shutdown()
+ rg := testutil.NewRandGenerator(t.Logf)
for length := 2; length < 5; length++ {
binary, _, _, cleanup := startServer(t, ctx, 2)
@@ -191,7 +193,7 @@
// Create <length> chunks of up to 4MB of random bytes.
data := make([][]byte, length)
for i := 0; i < length; i++ {
- data[i] = testData()
+ data[i] = testData(rg)
}
if err := binary.Create(ctx, int32(length), repository.MediaInfo{Type: "application/octet-stream"}); err != nil {
t.Fatalf("Create() failed: %v", err)
@@ -211,7 +213,7 @@
break
}
for i := 0; i < length; i++ {
- fail := testutil.RandomIntn(2)
+ fail := rg.RandomIntn(2)
if parts[i] == binarylib.MissingPart && fail != 0 {
if streamErr, err := invokeUpload(t, ctx, binary, data[i], int32(i)); streamErr != nil || err != nil {
t.FailNow()
@@ -227,18 +229,18 @@
// TestErrors checks that the binary interface correctly reports errors.
func TestErrors(t *testing.T) {
- testutil.InitRandGenerator(t.Logf)
ctx, shutdown := test.V23Init()
defer shutdown()
+ rg := testutil.NewRandGenerator(t.Logf)
binary, _, _, cleanup := startServer(t, ctx, 2)
defer cleanup()
const length = 2
data := make([][]byte, length)
for i := 0; i < length; i++ {
- data[i] = testData()
+ data[i] = testData(rg)
for j := 0; j < len(data[i]); j++ {
- data[i][j] = byte(testutil.RandomInt())
+ data[i][j] = byte(rg.RandomInt())
}
}
if err := binary.Create(ctx, int32(length), repository.MediaInfo{Type: "application/octet-stream"}); err != nil {
@@ -295,10 +297,11 @@
func TestGlob(t *testing.T) {
ctx, shutdown := test.V23Init()
defer shutdown()
+ rg := testutil.NewRandGenerator(t.Logf)
_, ep, _, cleanup := startServer(t, ctx, 2)
defer cleanup()
- data := testData()
+ data := testData(rg)
objects := []string{"foo", "bar", "hello world", "a/b/c"}
for _, obj := range objects {
diff --git a/services/internal/binarylib/perms_test.go b/services/internal/binarylib/perms_test.go
index b4b684a..0729b69 100644
--- a/services/internal/binarylib/perms_test.go
+++ b/services/internal/binarylib/perms_test.go
@@ -76,6 +76,7 @@
func TestBinaryCreateAccessList(t *testing.T) {
ctx, shutdown := test.V23Init()
defer shutdown()
+ rg := testutil.NewRandGenerator(t.Logf)
selfCtx, err := v23.WithPrincipal(ctx, testutil.NewPrincipal("self"))
if err != nil {
@@ -105,7 +106,7 @@
if err := b("bini/private").Create(childCtx, 1, repository.MediaInfo{Type: "application/octet-stream"}); err != nil {
t.Fatalf("Create() failed %v", err)
}
- fakeDataPrivate := testData()
+ fakeDataPrivate := testData(rg)
if streamErr, err := invokeUpload(t, childCtx, b("bini/private"), fakeDataPrivate, 0); streamErr != nil || err != nil {
t.Fatalf("invokeUpload() failed %v, %v", err, streamErr)
}
@@ -130,6 +131,7 @@
func TestBinaryRootAccessList(t *testing.T) {
ctx, shutdown := test.V23Init()
defer shutdown()
+ rg := testutil.NewRandGenerator(t.Logf)
selfPrincipal := testutil.NewPrincipal("self")
selfCtx, err := v23.WithPrincipal(ctx, selfPrincipal)
@@ -161,7 +163,7 @@
if err := b("bini/private").Create(selfCtx, 1, repository.MediaInfo{Type: "application/octet-stream"}); err != nil {
t.Fatalf("Create() failed %v", err)
}
- fakeDataPrivate := testData()
+ fakeDataPrivate := testData(rg)
if streamErr, err := invokeUpload(t, selfCtx, b("bini/private"), fakeDataPrivate, 0); streamErr != nil || err != nil {
t.Fatalf("invokeUpload() failed %v, %v", err, streamErr)
}
@@ -169,7 +171,7 @@
if err := b("bini/shared").Create(selfCtx, 1, repository.MediaInfo{Type: "application/octet-stream"}); err != nil {
t.Fatalf("Create() failed %v", err)
}
- fakeDataShared := testData()
+ fakeDataShared := testData(rg)
if streamErr, err := invokeUpload(t, selfCtx, b("bini/shared"), fakeDataShared, 0); streamErr != nil || err != nil {
t.Fatalf("invokeUpload() failed %v, %v", err, streamErr)
}
@@ -296,7 +298,7 @@
if err := b("bini/otherbinary").Create(otherCtx, 1, repository.MediaInfo{Type: "application/octet-stream"}); err != nil {
t.Fatalf("Create() failed %v", err)
}
- fakeDataOther := testData()
+ fakeDataOther := testData(rg)
if streamErr, err := invokeUpload(t, otherCtx, b("bini/otherbinary"), fakeDataOther, 0); streamErr != nil || err != nil {
t.FailNow()
}
diff --git a/services/internal/binarylib/util_test.go b/services/internal/binarylib/util_test.go
index 30b414f..cd3d785 100644
--- a/services/internal/binarylib/util_test.go
+++ b/services/internal/binarylib/util_test.go
@@ -88,8 +88,8 @@
}
// testData creates up to 4MB of random bytes.
-func testData() []byte {
- size := testutil.RandomIntn(1000 * binarylib.BufferLength)
- data := testutil.RandomBytes(size)
+func testData(rg *testutil.Random) []byte {
+ size := rg.RandomIntn(1000 * binarylib.BufferLength)
+ data := rg.RandomBytes(size)
return data
}
diff --git a/test/testutil/rand.go b/test/testutil/rand.go
index 3cb1526..8e21640 100644
--- a/test/testutil/rand.go
+++ b/test/testutil/rand.go
@@ -28,7 +28,6 @@
// Random is a concurrent-access friendly source of randomness.
type Random struct {
mu sync.Mutex
- seed int64
rand *rand.Rand
}
@@ -72,9 +71,12 @@
return buffer
}
-// Create a new pseudo-random number generator, the seed may be supplied
-// by V23_RNG_SEED to allow for reproducing a previous sequence.
-func NewRandGenerator() *Random {
+type loggingFunc func(format string, args ...interface{})
+
+// NewRandGenerator creates a new pseudo-random number generator; the seed may
+// be supplied by V23_RNG_SEED to allow for reproducing a previous sequence, and
+// is printed using the supplied logging function.
+func NewRandGenerator(logger loggingFunc) *Random {
seed := time.Now().UnixNano()
seedString := os.Getenv(SeedEnv)
if seedString != "" {
@@ -85,17 +87,35 @@
panic(fmt.Sprintf("ParseInt(%v, %v, %v) failed: %v", seedString, base, bitSize, err))
}
}
- return &Random{seed: seed, rand: rand.New(rand.NewSource(seed))}
+ logger("Seeded pseudo-random number generator with %v", seed)
+ return &Random{rand: rand.New(rand.NewSource(seed))}
}
+// TODO(caprita): Consider deprecating InitRandGenerator in favor of using
+// NewRandGenerator directly. There are several drawbacks to using the global
+// singleton Random object:
+//
+// - tests that do not call InitRandGenerator themselves could depend on
+// InitRandGenerator having been called by other tests in the same package and
+// stop working when run standalone with test --run
+//
+// - conversely, a test case may call InitRandGenerator without actually
+// needing to; it's hard to figure out if some library called by a test
+// actually uses the Random object or not
+//
+// - when several test cases share the same Random object, there is
+// interference in the stream of random numbers generated for each test case
+// if run in parallel
+//
+// All these issues can be trivially addressed if the Random object is created
+// and plumbed through the call stack explicitly.
+
// InitRandGenerator creates an instance of Random in the public variable Rand
-// and returns a function intended to be defer'ed that prints out the
-// seed use when creating the number number generator using the supplied
-// logging function.
-func InitRandGenerator(loggingFunc func(format string, args ...interface{})) {
+// and prints out the seed use when creating the number number generator using
+// the supplied logging function.
+func InitRandGenerator(logger loggingFunc) {
once.Do(func() {
- Rand = NewRandGenerator()
- loggingFunc("Seeded pseudo-random number generator with %v", Rand.seed)
+ Rand = NewRandGenerator(logger)
})
}
