isolate: test uploading of individual regular files

client/cmd/isolate/upload_tracker_test.go

Index: client/cmd/isolate/upload_tracker_test.go
diff --git a/client/cmd/isolate/upload_tracker_test.go b/client/cmd/isolate/upload_tracker_test.go
index ca45233ac985821177091c90e2bf4d3985b041c7..62585228d85cddff462328892795a78a3ad5eb99 100644
--- a/client/cmd/isolate/upload_tracker_test.go
+++ b/client/cmd/isolate/upload_tracker_test.go
@@ -18,8 +18,10 @@ import (
 	"bytes"
 	"fmt"
 	"io"
+	"io/ioutil"
 	"os"
 	"reflect"
+	"strings"
 	"testing"
 
 	"github.com/luci/luci-go/common/isolated"
@@ -30,7 +32,8 @@ import (
 // Fake OS imitiates a filesystem by storing file contents in a map.
 // It also provides a dummy Readlink implementation.
 type fakeOS struct {
-	files map[string]*bytes.Buffer
+	writeFiles map[string]*bytes.Buffer
+	readFiles  map[string]io.Reader
 }
 
 // shouldResembleByteMap asserts that actual (a map[string]*bytes.Buffer) contains
@@ -73,11 +76,20 @@ func (nopWriteCloser) Close() error { return nil }
 
 // implements OpenFile by returning a writer that writes to a bytes.Buffer.
 func (fos *fakeOS) OpenFile(name string, flag int, perm os.FileMode) (io.WriteCloser, error) {
-	if fos.files == nil {
-		fos.files = make(map[string]*bytes.Buffer)
+	if fos.writeFiles == nil {
+		fos.writeFiles = make(map[string]*bytes.Buffer)
 	}
-	fos.files[name] = &bytes.Buffer{}
-	return nopWriteCloser{fos.files[name]}, nil
+	fos.writeFiles[name] = &bytes.Buffer{}
+	return nopWriteCloser{fos.writeFiles[name]}, nil
+}
+
+// implements Open by returning a pre-configured Reader.
+func (fos *fakeOS) Open(name string) (io.ReadCloser, error) {
+	r, ok := fos.readFiles[name]
+	if !ok {
+		panic(fmt.Sprintf("fakeOS: file not found (%s); not implemented.", name))
+	}
+	return ioutil.NopCloser(r), nil
 }
 
 // fakeChecker implements Checker by responding to method invocations by
@@ -106,6 +118,8 @@ func (checker *fakeChecker) Close() error { return nil }
 type fakeUploader struct {
 	// uploadBytesCalls is a record of the arguments to each call of UploadBytes.
 	uploadBytesCalls []uploaderUploadBytesArgs
+	// uploadFileCalls is a record of the arguments to each call of UploadFile.
+	uploadFileCalls []uploaderUploadFileArgs
 
 	Uploader // TODO(mcgreevy): implement other methods.
 }
@@ -116,10 +130,18 @@ type uploaderUploadBytesArgs struct {
 	ps       *isolatedclient.PushState
 }
 
+type uploaderUploadFileArgs struct {
+	item *Item
+	ps   *isolatedclient.PushState
+}
+
 func (uploader *fakeUploader) UploadBytes(relPath string, isolJSON []byte, ps *isolatedclient.PushState, f func()) {
 	uploader.uploadBytesCalls = append(uploader.uploadBytesCalls, uploaderUploadBytesArgs{relPath, isolJSON, ps})
 }
 
+func (uploader *fakeUploader) UploadFile(item *Item, ps *isolatedclient.PushState, done func()) {
+	uploader.uploadFileCalls = append(uploader.uploadFileCalls, uploaderUploadFileArgs{item, ps})

[mithro, 2017/08/01 04:45:42]

Do you ever call the done function? Don't you need too?

[mcgreevy, 2017/08/02 03:59:30]

I didn't bother because the done function is only ever a one-line function which
prints a log line. But I suppose I should anyway.

Done.

+}
 func (uploader *fakeUploader) Close() error { return nil }
 
 func TestSkipsUpload(t *testing.T) {
@@ -149,7 +171,7 @@ func TestSkipsUpload(t *testing.T) {
 
 		// In this test, the only item that is checked and uploaded is the generated isolated file.
 		wantIsolJSON := []byte(`{"algo":"","version":""}`)
-		So(fos.files, shouldResembleByteMap, map[string][]byte{"/a/isolatedPath": wantIsolJSON})
+		So(fos.writeFiles, shouldResembleByteMap, map[string][]byte{"/a/isolatedPath": wantIsolJSON})
 
 		So(isolSummary.Digest, ShouldResemble, isolated.HashBytes(wantIsolJSON))
 		So(isolSummary.Name, ShouldEqual, "isolatedPath")
@@ -185,7 +207,7 @@ func TestDontSkipUpload(t *testing.T) {
 
 		// In this test, the only item that is checked and uploaded is the generated isolated file.
 		wantIsolJSON := []byte(`{"algo":"","version":""}`)
-		So(fos.files, shouldResembleByteMap, map[string][]byte{"/a/isolatedPath": wantIsolJSON})
+		So(fos.writeFiles, shouldResembleByteMap, map[string][]byte{"/a/isolatedPath": wantIsolJSON})

[mithro, 2017/08/01 04:45:42]

Thought to consider (no action needed in this CL..);

The only file we ever write is the isolated json output right? Is there any
reason to have a generic writeFiles like functionality rather than doing
something more around that?

[mcgreevy, 2017/08/02 03:59:30]

I notice that after I uploaded this CL.  But then I realized I would still need
to store both the path and the json contents, which means I need either two
fields, or a new struct type, and I figured that this works just as well.

 
 		So(isolSummary.Digest, ShouldResemble, isolated.HashBytes(wantIsolJSON))
 		So(isolSummary.Name, ShouldEqual, "isolatedPath")
@@ -236,7 +258,7 @@ func TestHandlesSymlinks(t *testing.T) {
 
 		// In this test, the only item that is checked and uploaded is the generated isolated file.
 		wantIsolJSON := []byte(`{"algo":"","files":{"c":{"l":"link:/a/b/c"}},"version":""}`)
-		So(fos.files, shouldResembleByteMap, map[string][]byte{"/a/isolatedPath": wantIsolJSON})
+		So(fos.writeFiles, shouldResembleByteMap, map[string][]byte{"/a/isolatedPath": wantIsolJSON})
 
 		So(isolSummary.Digest, ShouldResemble, isolated.HashBytes(wantIsolJSON))
 		So(isolSummary.Name, ShouldEqual, "isolatedPath")
@@ -246,3 +268,92 @@ func TestHandlesSymlinks(t *testing.T) {
 		})
 	})
 }
+
+func TestHandlesIndividualFiles(t *testing.T) {
+	t.Parallel()
+	Convey(`Individual files should be stored in the isolated json and uploaded`, t, func() {
+		isol := &isolated.Isolated{}
+
+		// non-nil PushState means don't skip the upload.

[mithro, 2017/08/01 04:45:42]

I'm a bit confused at how the push state is being used here?

[mcgreevy, 2017/08/02 03:59:30]

The PushState is not really meant to be inspected by the client, so it doesn't
really matter what's in it.  What the client cares about is whether it is nil or
not.

So here I am returning a non-nil PushState from the checker to signify that the
file should be uploaded. 
 Later, I re-use that same PushState in the expected upload file args, because
the UploadTracker passes (to the uploader) the same PushState that it gets back
from the checker.

I am not returning a separate PushState each time the checker is called because
it would add extra complexity to the test that I don't think is worth it.

+		pushState := &isolatedclient.PushState{}
+		checker := &fakeChecker{ps: pushState}
+		uploader := &fakeUploader{}
+
+		ut := NewUploadTracker(checker, uploader, isol)
+		fos := &fakeOS{
+			readFiles: map[string]io.Reader{
+				"/a/b/foo": strings.NewReader("foo contents"),
+				"/a/b/bar": strings.NewReader("bar contents"),
+			},
+		}
+		ut.lOS = fos // Override filesystem calls with fake.
+
+		parts := partitionedDeps{
+			indivFiles: itemGroup{
+				items: []*Item{
+					{Path: "/a/b/foo", RelPath: "foo", Size: 1, Mode: 006},
+					{Path: "/a/b/bar", RelPath: "bar", Size: 2, Mode: 006},
+				},
+				totalSize: 3,
+			},
+		}
+		err := ut.UploadDeps(parts)
+		So(err, ShouldBeNil)
+
+		fooHash := isolated.HashBytes([]byte("foo contents"))
+		barHash := isolated.HashBytes([]byte("bar contents"))
+		wantFiles := map[string]isolated.File{
+			"foo": {
+				Digest: fooHash,
+				Mode:   Int(6),
+				Size:   Int64(1)},
+			"bar": {
+				Digest: barHash,
+				Mode:   Int(6),
+				Size:   Int64(2)},
+		}
+
+		So(ut.isol.Files, ShouldResemble, wantFiles)
+
+		So(uploader.uploadBytesCalls, ShouldEqual, nil)
+
+		isolSummary, err := ut.Finalize("/a/isolatedPath")
+		So(err, ShouldBeNil)
+
+		wantIsolJSONTmpl := `{"algo":"","files":{"bar":{"h":"%s","m":6,"s":2},"foo":{"h":"%s","m":6,"s":1}},"version":""}`
+		wantIsolJSON := []byte(fmt.Sprintf(wantIsolJSONTmpl, barHash, fooHash))

[mithro, 2017/08/01 04:45:42]

Is there any reason particular reason to define wantIsolJSONTmpl as a separate
value rather than just putting it inside the Sprintf call?

[mcgreevy, 2017/08/02 03:59:29]

Line length.

It's not Go style to split function parameter lists over multiple lines. So I am
doing this instead to avoid the Sprintf line getting too long. 

+		So(fos.writeFiles, shouldResembleByteMap, map[string][]byte{"/a/isolatedPath": wantIsolJSON})
+
+		So(isolSummary.Digest, ShouldResemble, isolated.HashBytes(wantIsolJSON))
+		So(isolSummary.Name, ShouldEqual, "isolatedPath")
+
+		So(uploader.uploadBytesCalls, ShouldResemble, []uploaderUploadBytesArgs{
+			{"isolatedPath", wantIsolJSON, checker.ps},
+		})
+		So(uploader.uploadFileCalls, ShouldResemble, []uploaderUploadFileArgs{
+			{
+				item: &Item{
+					Path:    "/a/b/foo",
+					RelPath: "foo",
+					Size:    1,
+					Mode:    os.FileMode(6),
+					Digest:  fooHash,
+				},
+				ps: pushState,
+			},
+			{
+				item: &Item{
+					Path:    "/a/b/bar",
+					RelPath: "bar",
+					Size:    2,
+					Mode:    os.FileMode(6),

[mithro, 2017/08/01 04:45:42]

You should be trying with different File modes maybe?

[mcgreevy, 2017/08/02 03:59:30]

Done.

+					Digest:  barHash,
+				},
+				ps: pushState,
+			},
+		})
+	})
+}
+
+func Int(i int) *int       { return &i }
+func Int64(i int64) *int64 { return &i }

[mithro, 2017/08/01 04:45:43]

Could you add a comment about why these casts are needed?

[mcgreevy, 2017/08/02 03:59:30]

Done.

FYI they're not casts, they're just helpers to take the address of a literal
value, which is not otherwise allowed in Go  i.e. you can't do

var i *int = &10

but you can (with the above helper) do:

var i *int = Int(10)