Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

swarm: Use pseudo-random instead of crypto-random for test files content generation #18083

Merged

Conversation

nizsheanez
Copy link
Contributor

@nizsheanez nizsheanez commented Nov 12, 2018

Related to CI speedup: #17854

Changes has been reviewed in and closes ethersphere/swarm#959

In this PR:

  • Replace "crypto/rand" by "math/rand" for files content generation
  • Use testutil.TempFileWithContent where possible
  • Move testutil/http.go file to swarm/api/http package because it depends on swarm/api and swarm/storage packages (it means that testutil packager not possible to use from that packages - cyclic imports)
  • Use math/rand.Shuffle instead of swarm/network_test.go.Shuffle and swarm/btm/btm_test.go.Shuffle. Because math/rand.Shuffle in stdlib since go1.10 (go1.9 support dropped in: travis: remove Go 1.9 #17807)

}
defer tmp.Close()
defer os.Remove(tmp.Name())
tmpFileName := testutil.TempFileWithContent(t, data)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you're writing different data here now. Is that an issue?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. There is existing data constant in cmd/swarm/access_test.go - data variable was shadowed in this function, but had same value. I cleanup couple more places to reuse that constant.

@@ -530,7 +529,8 @@ func testDeliveryFromNodes(t *testing.T, nodes, conns, chunkCount int, skipCheck
//now we can actually upload a (random) file to the round-robin store
size := chunkCount * chunkSize
log.Debug("Storing data to file store")
fileHash, wait, err := roundRobinFileStore.Store(ctx, io.LimitReader(crand.Reader, int64(size)), int64(size), false)
b := testutil.RandomBytes(1, size)
fileHash, wait, err := roundRobinFileStore.Store(ctx, bytes.NewReader(b), int64(size), false)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not use testutil.RandomReader() in this case? (there are more of these cases further down the diff)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

true

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed everywhere except. swarm/storage/filestore_test.go:testFileStoreRandom - this test looks a bit weird for me. newTestDbStore using ioutil.TempDir inside, but test cleanup /tmp folder. Also i'm not sure why test writing files like /tmp/slice.bzz.16M. Looks like this test need to cleanup, I prefer to do it separately.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you mean tmp/bzz? I agree, there's something not too obvious about that test. And yes, it leaves behind garbage. Please fix. I'm fine with separate PR for that.

…m, swarm/storage, swarm/storage/encryption, swarm/testutil: use pseudo-random instead of crypto-random for test files content generation

- Replace "crypto/rand" to "math/rand" for files content generation
- Remove swarm/network_test.go.Shuffle and swarm/btm/btm_test.go.Shuffle - because go1.9 support dropped (see ethereum/go-ethereum#17807 and comments to swarm/network_test.go.Shuffle)
@nizsheanez nizsheanez force-pushed the storage-test-random-data-gen-speedup branch from d05871c to 7763017 Compare November 13, 2018 08:09
@zelig zelig merged commit eb8fa3c into ethereum:master Nov 14, 2018
@karalabe karalabe added this to the 1.8.18 milestone Nov 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants