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

fix incorrect end of file checking and add some unit tests #41

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions buffer/buffer.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ type BufferFile struct {
// DefaultCapacity is the size in bytes of a new BufferFile's backing buffer
const DefaultCapacity = 512

var errSeekToNegativeLocation = errors.New("unable to seek to a location <0")

// NewBufferFile creates new in memory parquet buffer.
func NewBufferFile() *BufferFile {
return NewBufferFileCapacity(DefaultCapacity)
Expand Down Expand Up @@ -54,7 +56,7 @@ func (bf *BufferFile) Seek(offset int64, whence int) (int64, error) {
}

if newLoc < 0 {
return int64(bf.loc), errors.New("unable to seek to a location <0")
return int64(bf.loc), errSeekToNegativeLocation
}

if newLoc > len(bf.buff) {
Expand All @@ -71,7 +73,8 @@ func (bf *BufferFile) Read(p []byte) (n int, err error) {
n = copy(p, bf.buff[bf.loc:len(bf.buff)])
bf.loc += n

if bf.loc == len(bf.buff) {
// if copied files into buffer with not enaugh capacity
if bf.loc < len(bf.buff) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@panikgaul 100% unit test coverage is nice.

But, it seems that your change is wrong and the code before the change is correct.

Did you intended if n < len(p) { ?

Even so, I don't understand how io.EOF relates to the capacity of the destination buffer.

Copy link
Author

Choose a reason for hiding this comment

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

Hello,

yes the if n < len(p) { was my intention.

When I used this with ==, I always got EOF, but that is not correct.
If destination buffer is smaller then source buffer, copy() gives you the count of copied bytes -> size of destination bugger, and you should get EOF in this case, because the source was not copied completely. That was my understanding and why I switched to <.
Now it works as expected.

I hope it was plausible :)

Copy link
Contributor

Choose a reason for hiding this comment

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

A partial copy due to insufficient capacity in the destination buffer (even if nothing is copied to a destination buffer with a capacity of 0 bytes) is not an error.

The io.EOF only indicates whether the end of the source has been reached.
It is absolutely not related at all with the destination buffer.

It is correct behavior for the Read of io.Reader to return io.EOF when the source is fully read.

return n, io.EOF
}

Expand Down Expand Up @@ -110,6 +113,7 @@ func (bf BufferFile) Close() error {
return nil
}

// Bytes returns the byte slice representing the buffer file.
func (bf BufferFile) Bytes() []byte {
return bf.buff
}
211 changes: 211 additions & 0 deletions buffer/buffer_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,211 @@
package buffer

import (
"io"
"math/rand"
"testing"
)

func TestNewBufferFile(t *testing.T) {
ce := 512
bf := NewBufferFile()
cg := cap(bf.Bytes())

if cg != ce {
t.Errorf("expected capacity: %v but got: %v", ce, cg)
}
}

func TestNewBufferFileFromBytes(t *testing.T) {
l := 1024
b := make([]byte, l)
bf := NewBufferFileFromBytes(b)

lg := len(bf.Bytes())

if lg != l {
t.Errorf("expected length: %v but got: %v", l, lg)
}
}

func TestNewBufferFileCapacity(t *testing.T) {
ce := 1024
bf := NewBufferFileCapacity(ce)
cg := cap(bf.Bytes())

if cg != ce {
t.Errorf("expected capacity: %v but got: %v", ce, cg)
}
}

func TestCreate(t *testing.T) {
bf := NewBufferFile()

if _, err := bf.Create("foo"); err != nil {
t.Errorf("unexpected error: %s", err)
}
}

func TestOpen(t *testing.T) {
bf := NewBufferFile()

if _, err := bf.Open("foo"); err != nil {
t.Errorf("unexpected error: %s", err)
}
}

func TestRead(t *testing.T) {
testCases := []struct {
name string // name of test case
capSrc int // capacity of source buffer
capDst int // capacity of destination count
cntExp int // expected copied bytes count
errExp error // expected count
}{
{
// regulary read without errors
name: "case1",
capSrc: 4,
capDst: 5,
cntExp: 4,
errExp: nil,
},
{
// read to buffer with not enaugh capacity
name: "case2",
capSrc: 4,
capDst: 3,
cntExp: 3,
errExp: io.EOF,
},
}

for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
bufSrc := make([]byte, tc.capSrc)
bufDst := make([]byte, tc.capDst)

rand.Read(bufSrc) // fill source buffer with random
bf := NewBufferFileFromBytes(bufSrc)

cnt, err := bf.Read(bufDst)

if tc.errExp != err {
t.Errorf("expected error to be: %v but got: %v", tc.errExp, err)
}

if tc.cntExp != cnt {
t.Errorf("expected copied bytes to be: %v but got: %v", tc.cntExp, cnt)
}
})
}
}

func TestWrite(t *testing.T) {
testCases := []struct {
name string // name of test case
capSrc int // capacity of source buffer
capDst int // capacity of destination count
errExp error // expected count
}{
{
// regulary write with enaugh capacity
name: "case1",
capSrc: 4,
capDst: 5,
errExp: nil,
},
{
// write to buffer with not enaugh capacity
name: "case2",
capSrc: 5,
capDst: 3,
errExp: nil,
},
}

for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
bufSrc := make([]byte, tc.capSrc)

rand.Read(bufSrc) // fill source buffer with random

bf := NewBufferFileCapacity(tc.capDst)

cnt, err := bf.Write(bufSrc)

if err != tc.errExp {
t.Errorf("expected error to be: %v but got: %v", tc.errExp, err)
}

if cnt != tc.capSrc {
t.Errorf("expected copied bytes to be: %v but got: %v", tc.capSrc, cnt)
}
})
}
}

func TestSeek(t *testing.T) {
testCases := []struct {
name string // name of test case
offset int64 // seek to this position
whence int // starting position
offsetExp int64 // expected location after seeking
errExp error // expected error
}{
{
name: "case1",
offset: 1,
whence: io.SeekStart,
offsetExp: 1,
errExp: nil,
},
{
name: "case2",
offset: 1,
whence: io.SeekCurrent,
offsetExp: 2,
errExp: nil,
},
{
name: "case3",
offset: 1,
whence: io.SeekEnd,
offsetExp: 42,
errExp: nil,
},
{
name: "case4",
offset: -44,
whence: io.SeekEnd,
offsetExp: 42,
errExp: errSeekToNegativeLocation,
},
}

// prepare buffer file with random bytes
buf := make([]byte, 42)
rand.Read(buf)
bf := NewBufferFileFromBytes(buf)

for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
offset, err := bf.Seek(tc.offset, tc.whence)

if offset != tc.offsetExp {
t.Errorf("expected offset to be %d but got %d", tc.offsetExp, offset)
}
if err != tc.errExp {
t.Errorf("expected error to be %v but got %v", tc.offsetExp, err)
}
})
}
}

func TestClose(t *testing.T) {
bf := NewBufferFile()

if err := bf.Close(); err != nil {
t.Errorf("unexpected error: %s\n", err)
}
}