isolate: test uploading of individual regular files

client/cmd/isolate/upload_tracker_test.go

 // Copyright 2017 The LUCI Authors.
 //
 // Licensed under the Apache License, Version 2.0 (the "License");
 // you may not use this file except in compliance with the License.
 // You may obtain a copy of the License at
 //
 //      http://www.apache.org/licenses/LICENSE-2.0
 //
 // Unless required by applicable law or agreed to in writing, software
 // distributed under the License is distributed on an "AS IS" BASIS,
 // WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
 package main
 
 import (
         "bytes"
         "fmt"
         "io"
+        "io/ioutil"
         "os"
         "reflect"
+        "strings"
         "testing"
 
         "github.com/luci/luci-go/common/isolated"
         "github.com/luci/luci-go/common/isolatedclient"
         . "github.com/smartystreets/goconvey/convey"
 )
 
 // 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
 // the same data as expected (a map[string][]byte).
 // The types of actual and expected differ to make writing tests with fakeOS more convenient.
 func shouldResembleByteMap(actual interface{}, expected ...interface{}) string {
         act, ok := actual.(map[string]*bytes.Buffer)
         if !ok {
                 return "actual is not a map[string]*bytes.Buffer"
         }
@@ skipping 22 matching lines @@
 }
 
 type nopWriteCloser struct {
         io.Writer
 }
 
 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
 // invoking the supplied callback with the supplied item, and a hard-coded *PushState.
 type fakeChecker struct {
         ps *isolatedclient.PushState
 }
 
 type checkerAddItemArgs struct {
         item     *Item
         isolated bool
 }
 
 type checkerAddItemResponse struct {
         item *Item
         ps   *isolatedclient.PushState
 }
 
 func (checker *fakeChecker) AddItem(item *Item, isolated bool, callback CheckerCallback) {
         callback(item, checker.ps)
 }
 
 func (checker *fakeChecker) Close() error { return nil }
 
 // fakeChecker implements Uploader while recording method arguments.
 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.
 }
 
 type uploaderUploadBytesArgs struct {
         relPath  string
         isolJSON []byte
         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) {
         t.Parallel()
         Convey(`nil push state signals that upload should be skipped`, t, func() {
                 isol := &isolated.Isolated{}
 
                 // nil PushState means skip the upload.
                 checker := &fakeChecker{ps: nil}
 
                 uploader := &fakeUploader{}
 
                 ut := NewUploadTracker(checker, uploader, isol)
                 fos := &fakeOS{}
                 ut.lOS = fos // Override filesystem calls with fake.
 
                 parts := partitionedDeps{} // no actual deps.
                 err := ut.UploadDeps(parts)
                 So(err, ShouldBeNil)
 
                 // No deps, so Files should be empty.
                 wantFiles := map[string]isolated.File{}
                 So(ut.isol.Files, ShouldResemble, wantFiles)
 
                 isolSummary, err := ut.Finalize("/a/isolatedPath")
                 So(err, ShouldBeNil)
 
                 // 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")
 
                 So(uploader.uploadBytesCalls, ShouldEqual, nil)
         })
 }
 
 func TestDontSkipUpload(t *testing.T) {
         t.Parallel()
@@ skipping 15 matching lines @@
 
                 // No deps, so Files should be empty.
                 wantFiles := map[string]isolated.File{}
                 So(ut.isol.Files, ShouldResemble, wantFiles)
 
                 isolSummary, err := ut.Finalize("/a/isolatedPath")
                 So(err, ShouldBeNil)
 
                 // 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")
 
                 // Upload was not skipped.
                 So(uploader.uploadBytesCalls, ShouldResemble, []uploaderUploadBytesArgs{
                         {"isolatedPath", wantIsolJSON, checker.ps},
                 })
         })
 }
@@ skipping 30 matching lines @@
                 So(ut.isol.Files, ShouldResemble, wantFiles)
 
                 // Symlinks are not uploaded.
                 So(uploader.uploadBytesCalls, ShouldEqual, nil)
 
                 isolSummary, err := ut.Finalize("/a/isolatedPath")
                 So(err, ShouldBeNil)
 
                 // 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")
 
                 So(uploader.uploadBytesCalls, ShouldResemble, []uploaderUploadBytesArgs{
                         {"isolatedPath", wantIsolJSON, checker.ps},
                 })
         })
 }
+
+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)