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

Extend bufferpool test #340

Merged
merged 5 commits into from
Mar 6, 2017
Merged

Extend bufferpool test #340

merged 5 commits into from
Mar 6, 2017

Conversation

willhug
Copy link
Contributor

@willhug willhug commented Feb 27, 2017

Summary: Adds on to the bufferpool test to write random bytes into
the buffers before putting them back into the sync.Pool

Summary: Adds on to the bufferpool test to write random bytes into
the buffers before putting them back into the sync.Pool
@mention-bot
Copy link

@willhug, thanks for your PR! By analyzing the history of the files in this pull request, we identified @akshayjshah and @prashantv to be potential reviewers.

@@ -36,10 +37,23 @@ func TestBuffers(t *testing.T) {
buf := Get()
assert.Zero(t, buf.Len(), "Expected truncated buffer")
assert.NotZero(t, buf.Cap(), "Expected non-zero capacity")

b := getRandBytes()
Copy link
Collaborator

Choose a reason for hiding this comment

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

do we really need random data? maybe just write a constant to simplify this

const dummyData = "dummy data"

buf.WriteString(dummyData)
assert.Equal(t, buf.Len(), len(dummyData), "...")


func getRandBytes() []byte {
b := make([]byte, rand.Intn(5000))
rand.Read(b)
Copy link
Collaborator

Choose a reason for hiding this comment

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

technically read can return an error that is being ignored here. might be cleaner to not use this at all

@willhug willhug changed the base branch from _start_-bettertest-part_1.0 to master February 27, 2017 22:25
Copy link
Contributor

@akshayjshah akshayjshah left a comment

Choose a reason for hiding this comment

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

Please fix the last remaining nit; otherwise lgtm.

"sync"
"testing"

"github.com/stretchr/testify/assert"
)

const dummyData = "dummy data"
Copy link
Contributor

Choose a reason for hiding this comment

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

Please underscore-prefix globals.

@@ -27,6 +27,8 @@ import (
"github.com/stretchr/testify/assert"
)

const dummyData = "dummy data"
Copy link
Collaborator

Choose a reason for hiding this comment

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

i'd move the const into the test since it's not used anywhere else

@akshayjshah akshayjshah merged commit 1b7cf6e into master Mar 6, 2017
@akshayjshah akshayjshah deleted the bettertest-part_1.0 branch March 14, 2017 18:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants