Refactor interfaces for persisting files

The packages will now own their own interfaces instead of borrowing one
that was implemented in storage.

browser/module.go

@@ -3,6 +3,7 @@ package browser
 
 import (
 	"context"
+	"io"
 	"log"
 	"net/http"
 	_ "net/http/pprof" //nolint:gosec
@@ -19,14 +20,20 @@ import (
 )
 
 type (
+	// FilePersister is the type that all file persisters must implement. It's job is
+	// to persist a file somewhere, hiding the details of where and how from the caller.
+	filePersister interface {
+		Persist(ctx context.Context, path string, data io.Reader) (err error)
+	}
+
 	// RootModule is the global module instance that will create module
 	// instances for each VU.
 	RootModule struct {
 		PidRegistry    *pidRegistry
 		remoteRegistry *remoteRegistry
 		initOnce       *sync.Once
 		tracesMetadata map[string]string
-		filePersister  storage.FilePersister
+		filePersister  filePersister
 	}
 
 	// JSModule exposes the properties available to the JS script.

browser/modulevu.go

@@ -5,7 +5,6 @@ import (
 
 	"github.com/grafana/xk6-browser/common"
 	"github.com/grafana/xk6-browser/k6ext"
-	"github.com/grafana/xk6-browser/storage"
 
 	k6modules "go.k6.io/k6/js/modules"
 )
@@ -22,7 +21,7 @@ type moduleVU struct {
 
 	*taskQueueRegistry
 
-	storage.FilePersister
+	FilePersister filePersister
 }
 
 // browser returns the VU browser instance for the current iteration.

common/element_handle.go

@@ -17,7 +17,6 @@ import (
 
 	"github.com/grafana/xk6-browser/common/js"
 	"github.com/grafana/xk6-browser/k6ext"
-	"github.com/grafana/xk6-browser/storage"
 )
 
 const resultDone = "done"
@@ -1180,7 +1179,7 @@ func (h *ElementHandle) setChecked(apiCtx context.Context, checked bool, p *Posi
 // Screenshot will instruct Chrome to save a screenshot of the current element and save it to specified file.
 func (h *ElementHandle) Screenshot(
 	opts *ElementHandleScreenshotOptions,
-	fp storage.FilePersister,
+	sp screenshotPersister,
 ) ([]byte, error) {
 	spanCtx, span := TraceAPICall(
 		h.ctx,
@@ -1191,7 +1190,7 @@ func (h *ElementHandle) Screenshot(
 
 	span.SetAttributes(attribute.String("screenshot.path", opts.Path))
 
-	s := newScreenshotter(spanCtx, fp)
+	s := newScreenshotter(spanCtx, sp)
 	buf, err := s.screenshotElement(h, opts)
 	if err != nil {
 		return nil, fmt.Errorf("taking screenshot of elementHandle: %w", err)

common/page.go

@@ -25,7 +25,6 @@ import (
 
 	"github.com/grafana/xk6-browser/k6ext"
 	"github.com/grafana/xk6-browser/log"
-	"github.com/grafana/xk6-browser/storage"
 
 	k6modules "go.k6.io/k6/js/modules"
 )
@@ -1121,13 +1120,13 @@ func (p *Page) Route(url goja.Value, handler goja.Callable) {
 }
 
 // Screenshot will instruct Chrome to save a screenshot of the current page and save it to specified file.
-func (p *Page) Screenshot(opts *PageScreenshotOptions, fp storage.FilePersister) ([]byte, error) {
+func (p *Page) Screenshot(opts *PageScreenshotOptions, sp screenshotPersister) ([]byte, error) {
 	spanCtx, span := TraceAPICall(p.ctx, p.targetID.String(), "page.screenshot")
 	defer span.End()
 
 	span.SetAttributes(attribute.String("screenshot.path", opts.Path))
 
-	s := newScreenshotter(spanCtx, fp)
+	s := newScreenshotter(spanCtx, sp)
 	buf, err := s.screenshotPage(p, opts)
 	if err != nil {
 		return nil, fmt.Errorf("taking screenshot of page: %w", err)

common/screenshotter.go

@@ -6,16 +6,21 @@ import (
 	"encoding/json"
 	"errors"
 	"fmt"
+	"io"
 	"math"
 	"strings"
 
 	"github.com/chromedp/cdproto/cdp"
 	"github.com/chromedp/cdproto/emulation"
 	cdppage "github.com/chromedp/cdproto/page"
-
-	"github.com/grafana/xk6-browser/storage"
 )
 
+// screenshotPersister is the type that all file persisters must implement. It's job is
+// to persist a file somewhere, hiding the details of where and how from the caller.
+type screenshotPersister interface {
+	Persist(ctx context.Context, path string, data io.Reader) (err error)
+}
+
 // ImageFormat represents an image file format.
 type ImageFormat string
 
@@ -61,11 +66,11 @@ func (f *ImageFormat) UnmarshalJSON(b []byte) error {
 
 type screenshotter struct {
 	ctx       context.Context
-	persister storage.FilePersister
+	persister screenshotPersister
 }
 
-func newScreenshotter(ctx context.Context, fp storage.FilePersister) *screenshotter {
-	return &screenshotter{ctx, fp}
+func newScreenshotter(ctx context.Context, sp screenshotPersister) *screenshotter {
+	return &screenshotter{ctx, sp}
 }
 
 func (s *screenshotter) fullPageSize(p *Page) (*Size, error) {

storage/file_persister.go

@@ -17,27 +17,27 @@ import (
 	"github.com/grafana/xk6-browser/env"
 )
 
-// FilePersister is the type that all file persisters must implement. It's job is
+// filePersister is the type that all file persisters must implement. It's job is
 // to persist a file somewhere, hiding the details of where and how from the caller.
-type FilePersister interface {
+type filePersister interface {
 	Persist(ctx context.Context, path string, data io.Reader) (err error)
 }
 
-// NewFilePersister will return either a LocalFilePersister or a RemoteFilePersister
-// depending on whether the K6_BROWSER_SCREENSHOTS_OUTPUT env var is setup with the
-// correct configs.
-func NewFilePersister(envLookup env.LookupFunc) (FilePersister, error) {
+// NewFilePersister will return either a persister that persists file to the local
+// disk or uploads the files to a remote location. This decision depends on whether
+// the K6_BROWSER_SCREENSHOTS_OUTPUT env var is setup with the correct configs.
+func NewFilePersister(envLookup env.LookupFunc) (filePersister, error) {
 	envVar, ok := envLookup(env.ScreenshotsOutput)
 	if !ok || envVar == "" {
-		return &LocalFilePersister{}, nil
+		return &localFilePersister{}, nil
 	}
 
 	u, b, h, err := parseEnvVar(envVar)
 	if err != nil {
 		return nil, fmt.Errorf("parsing %s: %w", env.ScreenshotsOutput, err)
 	}
 
-	return NewRemoteFilePersister(u, h, b), nil
+	return newRemoteFilePersister(u, h, b), nil
 }
 
 // parseEnvVar will parse a value such as:
@@ -92,12 +92,12 @@ func parseEnvVar(envVarValue string) (string, string, map[string]string, error)
 	return url, basePath, headers, nil
 }
 
-// LocalFilePersister will persist files to the local disk.
-type LocalFilePersister struct{}
+// localFilePersister will persist files to the local disk.
+type localFilePersister struct{}
 
 // Persist will write the contents of data to the local disk on the specified path.
 // TODO: we should not write to disk here but put it on some queue for async disk writes.
-func (l *LocalFilePersister) Persist(_ context.Context, path string, data io.Reader) (err error) {
+func (l *localFilePersister) Persist(_ context.Context, path string, data io.Reader) (err error) {
 	cp := filepath.Clean(path)
 
 	dir := filepath.Dir(cp)
@@ -128,25 +128,25 @@ func (l *LocalFilePersister) Persist(_ context.Context, path string, data io.Rea
 	return nil
 }
 
-// RemoteFilePersister is to be used when files created by the browser module need
+// remoteFilePersister is to be used when files created by the browser module need
 // to be uploaded to a remote location. This uses a preSignedURLGetterURL to
 // retrieve one pre-signed URL. The pre-signed url is used to upload the file
 // to the remote location.
-type RemoteFilePersister struct {
+type remoteFilePersister struct {
 	preSignedURLGetterURL string
 	headers               map[string]string
 	basePath              string
 
 	httpClient *http.Client
 }
 
-// NewRemoteFilePersister creates a new instance of RemoteFilePersister.
-func NewRemoteFilePersister(
+// newRemoteFilePersister creates a new instance of RemoteFilePersister.
+func newRemoteFilePersister(
 	preSignedURLGetterURL string,
 	headers map[string]string,
 	basePath string,
-) *RemoteFilePersister {
-	return &RemoteFilePersister{
+) *remoteFilePersister {
+	return &remoteFilePersister{
 		preSignedURLGetterURL: preSignedURLGetterURL,
 		headers:               headers,
 		basePath:              basePath,
@@ -157,7 +157,7 @@ func NewRemoteFilePersister(
 }
 
 // Persist will upload the contents of data to a remote location.
-func (r *RemoteFilePersister) Persist(ctx context.Context, path string, data io.Reader) (err error) {
+func (r *remoteFilePersister) Persist(ctx context.Context, path string, data io.Reader) (err error) {
 	pURL, err := r.getPreSignedURL(ctx, path)
 	if err != nil {
 		return fmt.Errorf("getting presigned url: %w", err)
@@ -194,7 +194,7 @@ func checkStatusCode(resp *http.Response) error {
 }
 
 // getPreSignedURL will retrieve the presigned url for the current file.
-func (r *RemoteFilePersister) getPreSignedURL(ctx context.Context, path string) (string, error) {
+func (r *remoteFilePersister) getPreSignedURL(ctx context.Context, path string) (string, error) {
 	b, err := buildPresignedRequestBody(r.basePath, path)
 	if err != nil {
 		return "", fmt.Errorf("building request body: %w", err)

storage/file_persister_test.go

@@ -60,7 +60,7 @@ func TestLocalFilePersister(t *testing.T) {
 				require.NoError(t, err)
 			}
 
-			var l LocalFilePersister
+			var l localFilePersister
 			err := l.Persist(context.Background(), p, strings.NewReader(tt.data))
 			assert.NoError(t, err)
 
@@ -232,7 +232,7 @@ func TestRemoteFilePersister(t *testing.T) {
 					w.WriteHeader(tt.uploadResponse)
 				}))
 
-			r := NewRemoteFilePersister(s.URL+presignedEndpoint, tt.wantPresignedHeaders, basePath)
+			r := newRemoteFilePersister(s.URL+presignedEndpoint, tt.wantPresignedHeaders, basePath)
 			err := r.Persist(context.Background(), tt.path, strings.NewReader(tt.dataToUpload))
 			if tt.wantError != "" {
 				assert.EqualError(t, err, tt.wantError)

tests/element_handle_test.go

@@ -8,6 +8,7 @@ import (
 	"testing"
 
 	"github.com/grafana/xk6-browser/common"
+	"github.com/grafana/xk6-browser/env"
 	"github.com/grafana/xk6-browser/storage"
 
 	"github.com/stretchr/testify/assert"
@@ -375,9 +376,11 @@ func TestElementHandleScreenshot(t *testing.T) {
 	elem, err := p.Query("div")
 	require.NoError(t, err)
 
+	fp, err := storage.NewFilePersister(env.ConstLookup(env.ScreenshotsOutput, ""))
+	require.NoError(t, err)
+
 	buf, err := elem.Screenshot(
-		common.NewElementHandleScreenshotOptions(elem.Timeout()),
-		&storage.LocalFilePersister{},
+		common.NewElementHandleScreenshotOptions(elem.Timeout()), fp,
 	)
 	require.NoError(t, err)
 

tests/page_test.go

@@ -18,6 +18,7 @@ import (
 
 	"github.com/grafana/xk6-browser/browser"
 	"github.com/grafana/xk6-browser/common"
+	"github.com/grafana/xk6-browser/env"
 	"github.com/grafana/xk6-browser/k6ext/k6test"
 	"github.com/grafana/xk6-browser/storage"
 )
@@ -476,7 +477,9 @@ func TestPageScreenshotFullpage(t *testing.T) {
 
 	opts := common.NewPageScreenshotOptions()
 	opts.FullPage = true
-	buf, err := p.Screenshot(opts, &storage.LocalFilePersister{})
+	fp, err := storage.NewFilePersister(env.ConstLookup(env.ScreenshotsOutput, ""))
+	require.NoError(t, err)
+	buf, err := p.Screenshot(opts, fp)
 	require.NoError(t, err)
 
 	reader := bytes.NewReader(buf)