Skip to content

Commit

Permalink
Fix circular deps
Browse files Browse the repository at this point in the history
  • Loading branch information
Luke Bjerring committed Nov 12, 2018
2 parents 40165aa + 21b2f72 commit b31600a
Show file tree
Hide file tree
Showing 11 changed files with 71 additions and 41 deletions.
2 changes: 1 addition & 1 deletion api/query/autocomplete_medium_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ func TestAutocompleteHandler(t *testing.T) {
defer mockCtrl.Finish()

// TODO(markdittmer): Should this be hitting GCS instead?
store := shared.NewMockReadable(mockCtrl)
store := sharedtest.NewMockReadable(mockCtrl)
rs := []*sharedtest.MockReadCloser{
sharedtest.NewMockReadCloser(t, summaryBytes[0]),
sharedtest.NewMockReadCloser(t, summaryBytes[1]),
Expand Down
5 changes: 3 additions & 2 deletions api/query/query_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import (
"github.com/golang/mock/gomock"
"github.com/stretchr/testify/assert"
"github.com/web-platform-tests/wpt.fyi/shared"
"github.com/web-platform-tests/wpt.fyi/shared/sharedtest"
)

func TestGetMemcacheKey(t *testing.T) {
Expand Down Expand Up @@ -44,7 +45,7 @@ func TestLoadSummaries_success(t *testing.T) {
getMemcacheKey(testRuns[1]),
}

cachedStore := shared.NewMockCachedStore(mockCtrl)
cachedStore := sharedtest.NewMockCachedStore(mockCtrl)
sh := unstructuredSearchHandler{queryHandler{dataSource: cachedStore}}
summaryBytes := [][]byte{
[]byte(`{"/a/b/c":[1,2]}`),
Expand Down Expand Up @@ -94,7 +95,7 @@ func TestLoadSummaries_fail(t *testing.T) {
getMemcacheKey(testRuns[1]),
}

cachedStore := shared.NewMockCachedStore(mockCtrl)
cachedStore := sharedtest.NewMockCachedStore(mockCtrl)
sh := unstructuredSearchHandler{queryHandler{dataSource: cachedStore}}
summaryBytes := [][]byte{
[]byte(`{"/a/b/c":[1,2]}`),
Expand Down
4 changes: 2 additions & 2 deletions api/query/search_medium_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ func TestUnstructuredSearchHandler(t *testing.T) {
defer mockCtrl.Finish()

// TODO(markdittmer): Should this be hitting GCS instead?
store := shared.NewMockReadable(mockCtrl)
store := sharedtest.NewMockReadable(mockCtrl)
rs := []*sharedtest.MockReadCloser{
sharedtest.NewMockReadCloser(t, summaryBytes[0]),
sharedtest.NewMockReadCloser(t, summaryBytes[1]),
Expand Down Expand Up @@ -187,7 +187,7 @@ func TestStructuredSearchHandler_equivalentToUnstructured(t *testing.T) {
defer mockCtrl.Finish()

// TODO(markdittmer): Should this be hitting GCS instead?
store := shared.NewMockReadable(mockCtrl)
store := sharedtest.NewMockReadable(mockCtrl)
rs := []*sharedtest.MockReadCloser{
sharedtest.NewMockReadCloser(t, summaryBytes[0]),
sharedtest.NewMockReadCloser(t, summaryBytes[1]),
Expand Down
16 changes: 8 additions & 8 deletions api/receiver/receive_results_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ import (

"github.com/golang/mock/gomock"
"github.com/stretchr/testify/assert"
"github.com/web-platform-tests/wpt.fyi/shared"
"github.com/web-platform-tests/wpt.fyi/shared/sharedtest"
"google.golang.org/appengine/taskqueue"
)

Expand Down Expand Up @@ -92,7 +92,7 @@ func TestHandleResultsUpload_success(t *testing.T) {
mockAE := NewMockAppEngineAPI(mockCtrl)
f := &os.File{}
task := &taskqueue.Task{Name: "task"}
mockAE.EXPECT().Context().Return(shared.NewTestContext()).AnyTimes()
mockAE.EXPECT().Context().Return(sharedtest.NewTestContext()).AnyTimes()
gomock.InOrder(
mockAE.EXPECT().IsAdmin().Return(false),
mockAE.EXPECT().authenticateUploader("blade-runner", "123").Return(true),
Expand Down Expand Up @@ -130,7 +130,7 @@ func TestHandleResultsUpload_extra_params(t *testing.T) {
"os_version": "",
}
task := &taskqueue.Task{Name: "task"}
mockAE.EXPECT().Context().Return(shared.NewTestContext()).AnyTimes()
mockAE.EXPECT().Context().Return(sharedtest.NewTestContext()).AnyTimes()
gomock.InOrder(
mockAE.EXPECT().IsAdmin().Return(false),
mockAE.EXPECT().authenticateUploader("blade-runner", "123").Return(true),
Expand Down Expand Up @@ -171,7 +171,7 @@ func TestHandleURLPayload_single(t *testing.T) {
}

mockAE := NewMockAppEngineAPI(mockCtrl)
mockAE.EXPECT().Context().Return(shared.NewTestContext()).AnyTimes()
mockAE.EXPECT().Context().Return(sharedtest.NewTestContext()).AnyTimes()
gomock.InOrder(
mockAE.EXPECT().fetchWithTimeout("http://wpt.fyi/test.json.gz", DownloadTimeout).Return(f, nil),
mockAE.EXPECT().uploadToGCS(matchRegex(`^/wptd-results-buffer/blade-runner/.*\.json$`), f, true).Return(nil),
Expand All @@ -189,7 +189,7 @@ func TestHandleURLPayload_multiple(t *testing.T) {
urls := []string{"http://wpt.fyi/foo.json.gz", "http://wpt.fyi/bar.json.gz"}

mockAE := NewMockAppEngineAPI(mockCtrl)
mockAE.EXPECT().Context().Return(shared.NewTestContext()).AnyTimes()
mockAE.EXPECT().Context().Return(sharedtest.NewTestContext()).AnyTimes()
gomock.InOrder(
mockAE.EXPECT().fetchWithTimeout(urls[0], DownloadTimeout).Return(f, nil),
mockAE.EXPECT().uploadToGCS(matchRegex(`^/wptd-results-buffer/blade-runner/.*/0\.json$`), f, true).Return(nil),
Expand All @@ -211,7 +211,7 @@ func TestHandleURLPayload_retry_fetching(t *testing.T) {
errTimeout := fmt.Errorf("server timed out")

mockAE := NewMockAppEngineAPI(mockCtrl)
mockAE.EXPECT().Context().Return(shared.NewTestContext()).AnyTimes()
mockAE.EXPECT().Context().Return(sharedtest.NewTestContext()).AnyTimes()
gomock.InOrder(
mockAE.EXPECT().fetchWithTimeout("http://wpt.fyi/test.json.gz", DownloadTimeout).Return(nil, errTimeout),
mockAE.EXPECT().fetchWithTimeout("http://wpt.fyi/test.json.gz", DownloadTimeout).Return(nil, errTimeout),
Expand All @@ -230,7 +230,7 @@ func TestHandleURLPayload_fail_fetching(t *testing.T) {
errTimeout := fmt.Errorf("server timed out")

mockAE := NewMockAppEngineAPI(mockCtrl)
mockAE.EXPECT().Context().Return(shared.NewTestContext()).AnyTimes()
mockAE.EXPECT().Context().Return(sharedtest.NewTestContext()).AnyTimes()
gomock.InOrder(
mockAE.EXPECT().fetchWithTimeout("http://wpt.fyi/test.json.gz", DownloadTimeout).Return(nil, errTimeout),
mockAE.EXPECT().fetchWithTimeout("http://wpt.fyi/test.json.gz", DownloadTimeout).Return(nil, errTimeout),
Expand All @@ -250,7 +250,7 @@ func TestHandleURLPayload_fail_uploading(t *testing.T) {
errGCS := fmt.Errorf("failed to upload to GCS")

mockAE := NewMockAppEngineAPI(mockCtrl)
mockAE.EXPECT().Context().Return(shared.NewTestContext()).AnyTimes()
mockAE.EXPECT().Context().Return(sharedtest.NewTestContext()).AnyTimes()
gomock.InOrder(
mockAE.EXPECT().fetchWithTimeout("http://wpt.fyi/test.json.gz", DownloadTimeout).Return(f, nil),
mockAE.EXPECT().uploadToGCS(matchRegex(`^/wptd-results-buffer/blade-runner/.*\.json$`), f, true).Return(errGCS),
Expand Down
5 changes: 3 additions & 2 deletions shared/models_test.go
Original file line number Diff line number Diff line change
@@ -1,17 +1,18 @@
// +build medium

package shared
package shared_test

import (
"testing"

"github.com/stretchr/testify/assert"
"github.com/web-platform-tests/wpt.fyi/shared"
"github.com/web-platform-tests/wpt.fyi/shared/sharedtest"
"google.golang.org/appengine/datastore"
)

func TestTestRunIDs_LoadTestRuns(t *testing.T) {
testRuns := make(TestRuns, 2)
testRuns := make(shared.TestRuns, 2)
testRuns[0].BrowserName = "chrome"
testRuns[0].BrowserVersion = "63.0"
testRuns[0].OSName = "linux"
Expand Down
23 changes: 18 additions & 5 deletions shared/request_caching_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

package shared
package shared_test

import (
"context"
Expand All @@ -16,6 +16,7 @@ import (

"github.com/golang/mock/gomock"
"github.com/stretchr/testify/assert"
"github.com/web-platform-tests/wpt.fyi/shared"
"github.com/web-platform-tests/wpt.fyi/shared/sharedtest"
)

Expand All @@ -36,9 +37,15 @@ func (okHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
func TestNoCaching404(t *testing.T) {
mockCtrl := gomock.NewController(t)
defer mockCtrl.Finish()
cache := NewMockReadWritable(mockCtrl)
cache := sharedtest.NewMockReadWritable(mockCtrl)
cache.EXPECT().NewReadCloser("/some/url").Return(ioutil.NopCloser(failReader{}), nil)
h := NewCachingHandler(context.Background(), http.NotFoundHandler(), cache, AlwaysCachable, URLAsCacheKey, CacheStatusOK)
h := shared.NewCachingHandler(
context.Background(),
http.NotFoundHandler(),
cache,
shared.AlwaysCachable,
shared.URLAsCacheKey,
shared.CacheStatusOK)
w := httptest.NewRecorder()
r := httptest.NewRequest("GET", "/some/url", nil)
h.ServeHTTP(w, r)
Expand All @@ -48,11 +55,17 @@ func TestNoCaching404(t *testing.T) {
func TestCaching200(t *testing.T) {
mockCtrl := gomock.NewController(t)
defer mockCtrl.Finish()
cache := NewMockReadWritable(mockCtrl)
cache := sharedtest.NewMockReadWritable(mockCtrl)
cache.EXPECT().NewReadCloser("/some/url").Return(ioutil.NopCloser(failReader{}), nil)
wc := sharedtest.NewMockWriteCloser(t)
cache.EXPECT().NewWriteCloser("/some/url").Return(wc, nil)
h := NewCachingHandler(context.Background(), okHandler{}, cache, AlwaysCachable, URLAsCacheKey, CacheStatusOK)
h := shared.NewCachingHandler(
context.Background(),
okHandler{},
cache,
shared.AlwaysCachable,
shared.URLAsCacheKey,
shared.CacheStatusOK)
w := httptest.NewRecorder()
r := httptest.NewRequest("GET", "/some/url", nil)
h.ServeHTTP(w, r)
Expand Down
14 changes: 14 additions & 0 deletions shared/sharedtest/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
# sharedtest

`sharedtest` is a folder for shared test code.

Note that when authoring tests for `shared`, while also relying on utilities
available in `sharedtest`, you'll need to put the test in the `shared_test`
package, which is a Golang convention for "black box" testing of the `shared`
package. This is because we would otherwise have a circular dependency of

shared
sharedtest
shared (test)

where `shared (test)` are `_test.go` files in the `shared` package.
7 changes: 4 additions & 3 deletions shared/storage_mock.go → shared/sharedtest/storage_mock.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

8 changes: 7 additions & 1 deletion shared/sharedtest/util.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ import (
"context"

"github.com/golang/mock/gomock"

"github.com/web-platform-tests/wpt.fyi/shared"
"google.golang.org/appengine"
"google.golang.org/appengine/aetest"
Expand Down Expand Up @@ -66,3 +65,10 @@ func SameProductSpec(spec string) gomock.Matcher {
spec: spec,
}
}

// NewTestContext creates a new context.Context for small tests.
func NewTestContext() context.Context {
ctx := context.Background()
ctx = context.WithValue(ctx, shared.DefaultLoggerCtxKey(), shared.NewNilLogger())
return ctx
}
21 changes: 11 additions & 10 deletions shared/storage_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,14 +4,15 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

package shared
package shared_test

import (
"errors"
"testing"

"github.com/golang/mock/gomock"
"github.com/stretchr/testify/assert"
"github.com/web-platform-tests/wpt.fyi/shared"
"github.com/web-platform-tests/wpt.fyi/shared/sharedtest"
"google.golang.org/appengine/memcache"
)
Expand All @@ -22,9 +23,9 @@ func TestGet_cacheHit(t *testing.T) {

var cacheID, storeID interface{}

cache := NewMockReadWritable(mockCtrl)
store := NewMockReadable(mockCtrl)
cs := NewByteCachedStore(NewTestContext(), cache, store)
cache := sharedtest.NewMockReadWritable(mockCtrl)
store := sharedtest.NewMockReadable(mockCtrl)
cs := shared.NewByteCachedStore(sharedtest.NewTestContext(), cache, store)

data := []byte("{}")
cr := sharedtest.NewMockReadCloser(t, data)
Expand All @@ -42,9 +43,9 @@ func TestGet_cacheMiss(t *testing.T) {

var cacheID, storeID interface{}

cache := NewMockReadWritable(mockCtrl)
store := NewMockReadable(mockCtrl)
cs := NewByteCachedStore(NewTestContext(), cache, store)
cache := sharedtest.NewMockReadWritable(mockCtrl)
store := sharedtest.NewMockReadable(mockCtrl)
cs := shared.NewByteCachedStore(sharedtest.NewTestContext(), cache, store)

data := []byte("{}")
cw := sharedtest.NewMockWriteCloser(t)
Expand All @@ -66,9 +67,9 @@ func TestGet_missing(t *testing.T) {

var cacheID, storeID interface{}

cache := NewMockReadWritable(mockCtrl)
store := NewMockReadable(mockCtrl)
cs := NewByteCachedStore(NewTestContext(), cache, store)
cache := sharedtest.NewMockReadWritable(mockCtrl)
store := sharedtest.NewMockReadable(mockCtrl)
cs := shared.NewByteCachedStore(sharedtest.NewTestContext(), cache, store)

errMissing := errors.New("Failed to fetch from store")
cache.EXPECT().NewReadCloser(&cacheID).Return(nil, memcache.ErrCacheMiss)
Expand Down
7 changes: 0 additions & 7 deletions shared/util.go
Original file line number Diff line number Diff line change
Expand Up @@ -195,13 +195,6 @@ func NewRequestContext(r *http.Request) context.Context {
return ctx
}

// NewTestContext creates a new context.Context for small tests.
func NewTestContext() context.Context {
ctx := context.Background()
ctx = context.WithValue(ctx, DefaultLoggerCtxKey(), NewNilLogger())
return ctx
}

// NewSetFromStringSlice is a helper for the inability to cast []string to []interface{}
func NewSetFromStringSlice(items []string) mapset.Set {
if items == nil {
Expand Down

0 comments on commit b31600a

Please sign in to comment.