Refactor remaining tests to subtest style

cors_test.go

@@ -8,14 +8,12 @@ import (
 )
 
 func TestCORS(t *testing.T) {
-	store := NewStoreComposer()
+	SubTest(t, "Preflight", func(t *testing.T, store *MockFullDataStore) {
-	store.UseCore(zeroStore{})
 		handler, _ := NewHandler(Config{
-		StoreComposer: store,
+			DataStore: store,
 		})
 
 		(&httpTest{
-		Name:   "Preflight request",
 			Method: "OPTIONS",
 			ReqHeader: map[string]string{
 				"Origin": "tus.io",
@@ -28,6 +26,12 @@ func TestCORS(t *testing.T) {
 				"Access-Control-Allow-Origin":  "tus.io",
 			},
 		}).Run(handler, t)
+	})
+
+	SubTest(t, "Request", func(t *testing.T, store *MockFullDataStore) {
+		handler, _ := NewHandler(Config{
+			DataStore: store,
+		})
 
 		(&httpTest{
 			Name:   "Actual request",
@@ -41,4 +45,5 @@ func TestCORS(t *testing.T) {
 				"Access-Control-Allow-Origin":   "tus.io",
 			},
 		}).Run(handler, t)
+	})
 }

get_test.go

@@ -19,12 +19,12 @@ func (reader *closingStringReader) Close() error {
 	return nil
 }
 
-var reader = &closingStringReader{
-	Reader: strings.NewReader("hello"),
-}
-
 func TestGet(t *testing.T) {
 	SubTest(t, "Download", func(t *testing.T, store *MockFullDataStore) {
+		reader := &closingStringReader{
+			Reader: strings.NewReader("hello"),
+		}
+
 		gomock.InOrder(
 			store.EXPECT().GetInfo("yes").Return(FileInfo{
 				Offset: 5,

handler_mock_test.go

@@ -1,5 +1,5 @@
 // Automatically generated by MockGen. DO NOT EDIT!
-// Source: handler_test.go
+// Source: utils_test.go
 
 package tusd_test
 
@@ -94,3 +94,44 @@ func (_m *MockFullDataStore) GetReader(id string) (io.Reader, error) {
 func (_mr *_MockFullDataStoreRecorder) GetReader(arg0 interface{}) *gomock.Call {
 	return _mr.mock.ctrl.RecordCall(_mr.mock, "GetReader", arg0)
 }
+
+// Mock of Locker interface
+type MockLocker struct {
+	ctrl     *gomock.Controller
+	recorder *_MockLockerRecorder
+}
+
+// Recorder for MockLocker (not exported)
+type _MockLockerRecorder struct {
+	mock *MockLocker
+}
+
+func NewMockLocker(ctrl *gomock.Controller) *MockLocker {
+	mock := &MockLocker{ctrl: ctrl}
+	mock.recorder = &_MockLockerRecorder{mock}
+	return mock
+}
+
+func (_m *MockLocker) EXPECT() *_MockLockerRecorder {
+	return _m.recorder
+}
+
+func (_m *MockLocker) LockUpload(id string) error {
+	ret := _m.ctrl.Call(_m, "LockUpload", id)
+	ret0, _ := ret[0].(error)
+	return ret0
+}
+
+func (_mr *_MockLockerRecorder) LockUpload(arg0 interface{}) *gomock.Call {
+	return _mr.mock.ctrl.RecordCall(_mr.mock, "LockUpload", arg0)
+}
+
+func (_m *MockLocker) UnlockUpload(id string) error {
+	ret := _m.ctrl.Call(_m, "UnlockUpload", id)
+	ret0, _ := ret[0].(error)
+	return ret0
+}
+
+func (_mr *_MockLockerRecorder) UnlockUpload(arg0 interface{}) *gomock.Call {
+	return _mr.mock.ctrl.RecordCall(_mr.mock, "UnlockUpload", arg0)
+}

handler_test.go

@@ -1,45 +0,0 @@
-package tusd_test
-
-import (
-	"net/http"
-	"strings"
-	"testing"
-
-	"github.com/golang/mock/gomock"
-	. "github.com/tus/tusd"
-)
-
-func TestMethodOverride(t *testing.T) {
-	mockCtrl := gomock.NewController(t)
-	defer mockCtrl.Finish()
-
-	store := NewMockFullDataStore(mockCtrl)
-
-	store.EXPECT().GetInfo("yes").Return(FileInfo{
-		Offset: 5,
-		Size:   20,
-	}, nil)
-
-	store.EXPECT().WriteChunk("yes", int64(5), NewReaderMatcher("hello")).Return(int64(5), nil)
-
-	handler, _ := NewHandler(Config{
-		DataStore: store,
-	})
-
-	(&httpTest{
-		Name:   "Successful request",
-		Method: "POST",
-		URL:    "yes",
-		ReqHeader: map[string]string{
-			"Tus-Resumable":          "1.0.0",
-			"Upload-Offset":          "5",
-			"Content-Type":           "application/offset+octet-stream",
-			"X-HTTP-Method-Override": "PATCH",
-		},
-		ReqBody: strings.NewReader("hello"),
-		Code:    http.StatusNoContent,
-		ResHeader: map[string]string{
-			"Upload-Offset": "10",
-		},
-	}).Run(handler, t)
-}

options_test.go

@@ -8,31 +8,38 @@ import (
 )
 
 func TestOptions(t *testing.T) {
-	store := NewStoreComposer()
+	SubTest(t, "Discovery", func(t *testing.T, store *MockFullDataStore) {
-	store.UseCore(NewMockFullDataStore(nil))
+		composer := NewStoreComposer()
+		composer.UseCore(store)
+
 		handler, _ := NewHandler(Config{
-		StoreComposer: store,
+			StoreComposer: composer,
 			MaxSize:       400,
 		})
 
 		(&httpTest{
-		Name:   "Successful request",
 			Method: "OPTIONS",
-		Code:   http.StatusOK,
 			ResHeader: map[string]string{
 				"Tus-Extension": "creation,creation-with-upload",
 				"Tus-Version":   "1.0.0",
 				"Tus-Resumable": "1.0.0",
 				"Tus-Max-Size":  "400",
 			},
+			Code: http.StatusOK,
 		}).Run(handler, t)
+	})
+
+	SubTest(t, "InvalidVersion", func(t *testing.T, store *MockFullDataStore) {
+		handler, _ := NewHandler(Config{
+			DataStore: store,
+		})
 
 		(&httpTest{
-		Name:   "Invalid or unsupported version",
 			Method: "POST",
 			ReqHeader: map[string]string{
 				"Tus-Resumable": "foo",
 			},
 			Code: http.StatusPreconditionFailed,
 		}).Run(handler, t)
+	})
 }

patch_test.go

@@ -1,57 +1,31 @@
 package tusd_test
 
 import (
-	"io"
 	"io/ioutil"
 	"net/http"
 	"os"
 	"strings"
 	"testing"
 
+	"github.com/golang/mock/gomock"
 	"github.com/stretchr/testify/assert"
 
 	. "github.com/tus/tusd"
 )
 
-type patchStore struct {
+func TestPatch(t *testing.T) {
-	zeroStore
+	SubTest(t, "UploadChunk", func(t *testing.T, store *MockFullDataStore) {
-	t      *assert.Assertions
+		gomock.InOrder(
-	called bool
+			store.EXPECT().GetInfo("yes").Return(FileInfo{
-}
+				ID:     "yes",
-
-func (s patchStore) GetInfo(id string) (FileInfo, error) {
-	if id != "yes" {
-		return FileInfo{}, os.ErrNotExist
-	}
-
-	return FileInfo{
-		ID:     id,
 				Offset: 5,
 				Size:   10,
-	}, nil
+			}, nil),
-}
+			store.EXPECT().WriteChunk("yes", int64(5), NewReaderMatcher("hello")).Return(int64(5), nil),
-
+		)
-func (s patchStore) WriteChunk(id string, offset int64, src io.Reader) (int64, error) {
-	s.t.False(s.called, "WriteChunk must be called only once")
-	s.called = true
-
-	s.t.Equal(int64(5), offset)
-
-	data, err := ioutil.ReadAll(src)
-	s.t.Nil(err)
-	s.t.Equal("hello", string(data))
-
-	return 5, nil
-}
-
-func TestPatch(t *testing.T) {
-	a := assert.New(t)
 
 		handler, _ := NewHandler(Config{
-		MaxSize: 100,
+			DataStore:             store,
-		DataStore: patchStore{
-			t: a,
-		},
 			NotifyCompleteUploads: true,
 		})
 
@@ -59,7 +33,6 @@ func TestPatch(t *testing.T) {
 		handler.CompleteUploads = c
 
 		(&httpTest{
-		Name:   "Successful request",
 			Method: "PATCH",
 			URL:    "yes",
 			ReqHeader: map[string]string{
@@ -74,242 +47,55 @@ func TestPatch(t *testing.T) {
 			},
 		}).Run(handler, t)
 
+		a := assert.New(t)
 		info := <-c
 		a.Equal("yes", info.ID)
-	a.Equal(int64(10), info.Size)
+		a.EqualValues(int64(10), info.Size)
 		a.Equal(int64(10), info.Offset)
+	})
 
-	(&httpTest{
+	SubTest(t, "MethodOverriding", func(t *testing.T, store *MockFullDataStore) {
-		Name:   "Non-existing file",
+		gomock.InOrder(
-		Method: "PATCH",
+			store.EXPECT().GetInfo("yes").Return(FileInfo{
-		URL:    "no",
+				ID:     "yes",
-		ReqHeader: map[string]string{
-			"Tus-Resumable": "1.0.0",
-			"Content-Type":  "application/offset+octet-stream",
-			"Upload-Offset": "5",
-		},
-		Code: http.StatusNotFound,
-	}).Run(handler, t)
-
-	(&httpTest{
-		Name:   "Wrong offset",
-		Method: "PATCH",
-		URL:    "yes",
-		ReqHeader: map[string]string{
-			"Tus-Resumable": "1.0.0",
-			"Content-Type":  "application/offset+octet-stream",
-			"Upload-Offset": "4",
-		},
-		Code: http.StatusConflict,
-	}).Run(handler, t)
-
-	(&httpTest{
-		Name:   "Exceeding file size",
-		Method: "PATCH",
-		URL:    "yes",
-		ReqHeader: map[string]string{
-			"Tus-Resumable": "1.0.0",
-			"Content-Type":  "application/offset+octet-stream",
-			"Upload-Offset": "5",
-		},
-		ReqBody: strings.NewReader("hellothisismorethan15bytes"),
-		Code:    http.StatusRequestEntityTooLarge,
-	}).Run(handler, t)
-}
-
-type overflowPatchStore struct {
-	zeroStore
-	t      *assert.Assertions
-	called bool
-}
-
-func (s overflowPatchStore) GetInfo(id string) (FileInfo, error) {
-	if id != "yes" {
-		return FileInfo{}, os.ErrNotExist
-	}
-
-	return FileInfo{
 				Offset: 5,
-		Size:   20,
+				Size:   10,
-	}, nil
+			}, nil),
-}
+			store.EXPECT().WriteChunk("yes", int64(5), NewReaderMatcher("hello")).Return(int64(5), nil),
+		)
 
-func (s overflowPatchStore) WriteChunk(id string, offset int64, src io.Reader) (int64, error) {
-	s.t.False(s.called, "WriteChunk must be called only once")
-	s.called = true
-
-	s.t.Equal(int64(5), offset)
-
-	data, err := ioutil.ReadAll(src)
-	s.t.Nil(err)
-	s.t.Equal("hellothisismore", string(data))
-
-	return 15, nil
-}
-
-// noEOFReader implements io.Reader, io.Writer, io.Closer but does not return
-// an io.EOF when the internal buffer is empty. This way we can simulate slow
-// networks.
-type noEOFReader struct {
-	closed bool
-	buffer []byte
-}
-
-func (r *noEOFReader) Read(dst []byte) (int, error) {
-	if r.closed && len(r.buffer) == 0 {
-		return 0, io.EOF
-	}
-
-	n := copy(dst, r.buffer)
-	r.buffer = r.buffer[n:]
-	return n, nil
-}
-
-func (r *noEOFReader) Close() error {
-	r.closed = true
-	return nil
-}
-
-func (r *noEOFReader) Write(src []byte) (int, error) {
-	r.buffer = append(r.buffer, src...)
-	return len(src), nil
-}
-
-func TestPatchOverflow(t *testing.T) {
 		handler, _ := NewHandler(Config{
-		MaxSize: 100,
+			DataStore: store,
-		DataStore: overflowPatchStore{
-			t: assert.New(t),
-		},
 		})
 
-	body := &noEOFReader{}
-
-	body.Write([]byte("hellothisismorethan15bytes"))
-	body.Close()
-
 		(&httpTest{
-		Name:   "Too big body exceeding file size",
+			Method: "POST",
-		Method: "PATCH",
 			URL:    "yes",
 			ReqHeader: map[string]string{
 				"Tus-Resumable":          "1.0.0",
-			"Content-Type":   "application/offset+octet-stream",
 				"Upload-Offset":          "5",
-			"Content-Length": "3",
-		},
-		ReqBody: body,
-		Code:    http.StatusNoContent,
-	}).Run(handler, t)
-}
-
-const (
-	LOCK = iota
-	INFO
-	WRITE
-	UNLOCK
-	END
-)
-
-type lockingPatchStore struct {
-	zeroStore
-	callOrder chan int
-}
-
-func (s lockingPatchStore) GetInfo(id string) (FileInfo, error) {
-	s.callOrder <- INFO
-
-	return FileInfo{
-		Offset: 0,
-		Size:   20,
-	}, nil
-}
-
-func (s lockingPatchStore) WriteChunk(id string, offset int64, src io.Reader) (int64, error) {
-	s.callOrder <- WRITE
-
-	return 5, nil
-}
-
-func (s lockingPatchStore) LockUpload(id string) error {
-	s.callOrder <- LOCK
-	return nil
-}
-
-func (s lockingPatchStore) UnlockUpload(id string) error {
-	s.callOrder <- UNLOCK
-	return nil
-}
-
-func TestLockingPatch(t *testing.T) {
-	callOrder := make(chan int, 10)
-
-	handler, _ := NewHandler(Config{
-		DataStore: lockingPatchStore{
-			callOrder: callOrder,
-		},
-	})
-
-	(&httpTest{
-		Name:   "Uploading to locking store",
-		Method: "PATCH",
-		URL:    "yes",
-		ReqHeader: map[string]string{
-			"Tus-Resumable": "1.0.0",
 				"Content-Type":           "application/offset+octet-stream",
-			"Upload-Offset": "0",
+				"X-HTTP-Method-Override": "PATCH",
 			},
 			ReqBody: strings.NewReader("hello"),
 			Code:    http.StatusNoContent,
+			ResHeader: map[string]string{
+				"Upload-Offset": "10",
+			},
 		}).Run(handler, t)
+	})
 
-	callOrder <- END
+	SubTest(t, "UploadChunkToFinished", func(t *testing.T, store *MockFullDataStore) {
-	close(callOrder)
+		store.EXPECT().GetInfo("yes").Return(FileInfo{
-
-	if <-callOrder != LOCK {
-		t.Error("expected call to LockUpload")
-	}
-
-	if <-callOrder != INFO {
-		t.Error("expected call to GetInfo")
-	}
-
-	if <-callOrder != WRITE {
-		t.Error("expected call to WriteChunk")
-	}
-
-	if <-callOrder != UNLOCK {
-		t.Error("expected call to UnlockUpload")
-	}
-
-	if <-callOrder != END {
-		t.Error("expected no more calls to happen")
-	}
-}
-
-type finishedPatchStore struct {
-	zeroStore
-}
-
-func (s finishedPatchStore) GetInfo(id string) (FileInfo, error) {
-	return FileInfo{
 			Offset: 20,
 			Size:   20,
-	}, nil
+		}, nil)
-}
-
-func (s finishedPatchStore) WriteChunk(id string, offset int64, src io.Reader) (int64, error) {
-	panic("WriteChunk must not be called")
-}
-
-func TestFinishedPatch(t *testing.T) {
 
 		handler, _ := NewHandler(Config{
-		DataStore: finishedPatchStore{},
+			DataStore: store,
 		})
 
 		(&httpTest{
-		Name:   "Uploading to finished upload",
 			Method: "PATCH",
 			URL:    "yes",
 			ReqHeader: map[string]string{
@@ -323,4 +109,146 @@ func TestFinishedPatch(t *testing.T) {
 				"Upload-Offset": "20",
 			},
 		}).Run(handler, t)
+	})
+
+	SubTest(t, "UploadNotFoundFail", func(t *testing.T, store *MockFullDataStore) {
+		store.EXPECT().GetInfo("no").Return(FileInfo{}, os.ErrNotExist)
+
+		handler, _ := NewHandler(Config{
+			DataStore: store,
+		})
+
+		(&httpTest{
+			Method: "PATCH",
+			URL:    "no",
+			ReqHeader: map[string]string{
+				"Tus-Resumable": "1.0.0",
+				"Content-Type":  "application/offset+octet-stream",
+				"Upload-Offset": "5",
+			},
+			Code: http.StatusNotFound,
+		}).Run(handler, t)
+	})
+
+	SubTest(t, "MissmatchingOffsetFail", func(t *testing.T, store *MockFullDataStore) {
+		store.EXPECT().GetInfo("yes").Return(FileInfo{
+			Offset: 5,
+		}, nil)
+
+		handler, _ := NewHandler(Config{
+			DataStore: store,
+		})
+
+		(&httpTest{
+			Method: "PATCH",
+			URL:    "yes",
+			ReqHeader: map[string]string{
+				"Tus-Resumable": "1.0.0",
+				"Content-Type":  "application/offset+octet-stream",
+				"Upload-Offset": "4",
+			},
+			Code: http.StatusConflict,
+		}).Run(handler, t)
+	})
+
+	SubTest(t, "ExceedingMaxSizeFail", func(t *testing.T, store *MockFullDataStore) {
+		store.EXPECT().GetInfo("yes").Return(FileInfo{
+			Offset: 5,
+			Size:   10,
+		}, nil)
+
+		handler, _ := NewHandler(Config{
+			DataStore: store,
+		})
+
+		(&httpTest{
+			Method: "PATCH",
+			URL:    "yes",
+			ReqHeader: map[string]string{
+				"Tus-Resumable": "1.0.0",
+				"Content-Type":  "application/offset+octet-stream",
+				"Upload-Offset": "5",
+			},
+			ReqBody: strings.NewReader("hellothisismorethan15bytes"),
+			Code:    http.StatusRequestEntityTooLarge,
+		}).Run(handler, t)
+	})
+
+	SubTest(t, "OverflowWithoutLength", func(t *testing.T, store *MockFullDataStore) {
+		// In this test we attempt to upload more than 15 bytes to an upload
+		// which has only space for 15 bytes (offset of 5 and size of 20).
+		// The request does not contain the Content-Length header and the handler
+		// therefore does not know the chunk's size before. The wanted behavior
+		// is that even if the uploader supplies more than 15 bytes, we only
+		// pass 15 bytes to the data store and ignore the rest.
+
+		gomock.InOrder(
+			store.EXPECT().GetInfo("yes").Return(FileInfo{
+				Offset: 5,
+				Size:   20,
+			}, nil),
+			store.EXPECT().WriteChunk("yes", int64(5), NewReaderMatcher("hellothisismore")).Return(int64(15), nil),
+		)
+
+		handler, _ := NewHandler(Config{
+			DataStore: store,
+		})
+
+		// Wrap the string.Reader in a NopCloser to hide its type. else
+		// http.NewRequest() will detect the we supply a strings.Reader as body
+		// and use this information to set the Content-Length header which we
+		// explicitly do not want (see comment above for reason).
+		body := ioutil.NopCloser(strings.NewReader("hellothisismorethan15bytes"))
+
+		(&httpTest{
+			Method: "PATCH",
+			URL:    "yes",
+			ReqHeader: map[string]string{
+				"Tus-Resumable": "1.0.0",
+				"Content-Type":  "application/offset+octet-stream",
+				"Upload-Offset": "5",
+			},
+			ReqBody: body,
+			Code:    http.StatusNoContent,
+			ResHeader: map[string]string{
+				"Upload-Offset": "20",
+			},
+		}).Run(handler, t)
+	})
+
+	SubTest(t, "Locker", func(t *testing.T, store *MockFullDataStore) {
+		ctrl := gomock.NewController(t)
+		defer ctrl.Finish()
+		locker := NewMockLocker(ctrl)
+
+		gomock.InOrder(
+			locker.EXPECT().LockUpload("yes").Return(nil),
+			store.EXPECT().GetInfo("yes").Return(FileInfo{
+				Offset: 0,
+				Size:   20,
+			}, nil),
+			store.EXPECT().WriteChunk("yes", int64(0), NewReaderMatcher("hello")).Return(int64(5), nil),
+			locker.EXPECT().UnlockUpload("yes").Return(nil),
+		)
+
+		composer := NewStoreComposer()
+		composer.UseCore(store)
+		composer.UseLocker(locker)
+
+		handler, _ := NewHandler(Config{
+			StoreComposer: composer,
+		})
+
+		(&httpTest{
+			Method: "PATCH",
+			URL:    "yes",
+			ReqHeader: map[string]string{
+				"Tus-Resumable": "1.0.0",
+				"Content-Type":  "application/offset+octet-stream",
+				"Upload-Offset": "0",
+			},
+			ReqBody: strings.NewReader("hello"),
+			Code:    http.StatusNoContent,
+		}).Run(handler, t)
+	})
 }

utils_test.go

@@ -11,22 +11,8 @@ import (
 
 	"github.com/golang/mock/gomock"
 	"github.com/tus/tusd"
-	. "github.com/tus/tusd"
 )
 
-type zeroStore struct{}
-
-func (store zeroStore) NewUpload(info FileInfo) (string, error) {
-	return "", nil
-}
-func (store zeroStore) WriteChunk(id string, offset int64, src io.Reader) (int64, error) {
-	return 0, nil
-}
-
-func (store zeroStore) GetInfo(id string) (FileInfo, error) {
-	return FileInfo{}, nil
-}
-
 type FullDataStore interface {
 	tusd.DataStore
 	tusd.TerminaterDataStore
@@ -34,6 +20,10 @@ type FullDataStore interface {
 	tusd.GetReaderDataStore
 }
 
+type Locker interface {
+	tusd.LockerDataStore
+}
+
 type httpTest struct {
 	Name string
 
@@ -79,17 +69,17 @@ func (test *httpTest) Run(handler http.Handler, t *testing.T) *httptest.Response
 	return w
 }
 
-type ReaderMatcher struct {
+type readerMatcher struct {
 	expect string
 }
 
 func NewReaderMatcher(expect string) gomock.Matcher {
-	return ReaderMatcher{
+	return readerMatcher{
 		expect: expect,
 	}
 }
 
-func (m ReaderMatcher) Matches(x interface{}) bool {
+func (m readerMatcher) Matches(x interface{}) bool {
 	input, ok := x.(io.Reader)
 	if !ok {
 		return false
@@ -104,6 +94,6 @@ func (m ReaderMatcher) Matches(x interface{}) bool {
 	return reflect.DeepEqual(m.expect, readStr)
 }
 
-func (m ReaderMatcher) String() string {
+func (m readerMatcher) String() string {
 	return fmt.Sprintf("reads to %s", m.expect)
 }