isolate: pull exparchive filewalk code into its own func

client/cmd/isolate/exp_archive_test.go

Index: client/cmd/isolate/exp_archive_test.go
diff --git a/client/cmd/isolate/exp_archive_test.go b/client/cmd/isolate/exp_archive_test.go
new file mode 100644
index 0000000000000000000000000000000000000000..fb6948fe3dfc3fcca7733bc2533135f393fcfc8c
--- /dev/null
+++ b/client/cmd/isolate/exp_archive_test.go
@@ -0,0 +1,228 @@
+// Copyright 2017 The LUCI Authors. All rights reserved.
+// Use of this source code is governed under the Apache License, Version 2.0
+// that can be found in the LICENSE file.
+
+package main
+
+import (
+	"errors"
+	"os"
+	"reflect"
+	"testing"
+)
+
+// basicFileInfo implements some of os.FileInfo, and panics if unexpected parts
+// of that interface are called.
+type basicFileInfo struct {
+	size  int64
+	mode  os.FileMode
+	isDir bool
+
+	os.FileInfo
+}
+
+func (bfi basicFileInfo) Size() int64       { return bfi.size }
+func (bfi basicFileInfo) Mode() os.FileMode { return bfi.mode }
+func (bfi basicFileInfo) IsDir() bool       { return bfi.isDir }
+
+type file struct {
+	path string
+	bfi  basicFileInfo
+}
+
+func symlink(path string, size int64) file {
+	return file{
+		path: path,
+		bfi: basicFileInfo{
+			size:  size,
+			mode:  os.ModeSymlink,
+			isDir: false,
+		},
+	}
+}
+
+func regularFile(path string, size int64) file {
+	return file{
+		path: path,
+		bfi: basicFileInfo{
+			size:  size,
+			mode:  0,
+			isDir: false,
+		},
+	}
+}
+
+func directory(path string) file {
+	return file{
+		path: path,
+		bfi: basicFileInfo{
+			size:  0,
+			mode:  os.ModeDir,
+			isDir: true,
+		},
+	}
+}
+
+var errBang = errors.New("bang")
+
+func TestWalkFn(t *testing.T) {
+	type testCase struct {
+		name      string
+		files     []file
+		walkFnErr error
+		want      partitionedDeps
+		wantErr   error
+	}
+
+TestCases:
+	for _, tc := range []testCase{

[mithro, 2017/06/27 07:48:10]

I would have tested the following cases;

 * empty directory
 * non-existent directory
 * single file
 * multiple files in a directory
 * recursive
 * recursive with empty directory
 * zero sized file
 * something around symlinks
 * something around the error cases

[mcgreevy, 2017/06/28 01:25:51]

Most of the tests that you have suggested below would be tests of filepath.Walk,
rather than walkFn. What I am testing here is that walkFn correctly adheres to
the behaviour expected by filepath.Walk.  There is no way to mock out the
filesystem used by filepath.Walk, and I don't want to hit the actual filesystem
in these tests. I think testing the behaviour of walkFn is sufficient.

Regarding particular suggestions:
it's up to filepath.Walk to handle the contents of directories. walkFn has no
idea whether a directory is empty or not.
walkFn is only given directories which exist; testing non-existing directories
would be a test of filepath.Walk.
Done.
This is covered by multiple tests below.
This would be a test of filepath.Walk.
This would be a test of filepath.Walk.
Done.
symlinks are covered.
I've added a test for bad relative paths, which is the only case that wasn't
already covered.

[mithro, 2017/06/29 07:25:17]

If I understand correctly, walkFn is called with a directory object for every
directory that is traversed?

[mcgreevy, 2017/06/30 01:24:19]

walkFn is called with the *path*, and an os.FileInfo, and it can tell by calling
FileInfo.IsDir whether the path is a directory, but it can not inspect the
contents of the directory.  All it can do is signal (via its return value)
whether it wants filepath.Walk to enter the directory or not.

+		{
+			name: "partitions files",
+			files: []file{
+				symlink("/rootDir/patha", 1e3),
+				regularFile("/rootDir/pathb", 10e3),
+				regularFile("/rootDir/pathc", 100e3),
+			},
+			want: partitionedDeps{
+				links: itemGroup{
+					items: []*Item{
+						{
+							Path:    "/rootDir/patha",
+							RelPath: "patha",
+							Mode:    os.ModeSymlink,
+							Size:    1e3,
+						},
+					},
+					size: 1e3,
+				},
+				archiveFiles: itemGroup{
+					items: []*Item{
+						{
+							Path:    "/rootDir/pathb",
+							RelPath: "pathb",
+							Mode:    0,
+							Size:    10e3,
+						},
+					},
+					size: 10e3,
+				},
+				indivFiles: itemGroup{
+					items: []*Item{
+						{
+							Path:    "/rootDir/pathc",
+							RelPath: "pathc",
+							Mode:    0,
+							Size:    100e3,
+						},
+					},
+					size: 100e3,
+				},
+			},
+		},
+		{
+			name: "aggregates link sizes",
+			files: []file{
+				symlink("/rootDir/patha", 1),
+				symlink("/rootDir/pathb", 2),
+			},
+			want: partitionedDeps{
+				links: itemGroup{
+					items: []*Item{
+						{
+							Path:    "/rootDir/patha",
+							RelPath: "patha",
+							Mode:    os.ModeSymlink,
+							Size:    1,

[mithro, 2017/06/27 07:48:10]

Why are these size 1 / 2?

[mcgreevy, 2017/06/28 01:25:51]

When testing things that add up values, I tend to use powers of 2, since there's
no way to combine them by some means other than the correct way to end up with
the expected answer.  For example, if both of these were size 10, then the
expected total size would be 20, but that could be arrived at by adding the
first file twice, rather than the first and second file.  Using powers of 2
avoids this.

[mithro, 2017/06/29 07:25:17]

Minor comments;

That makes a lot of sense, but with sizes 1, 2 the pattern could actually be
mistake for 1, 2, 3, 4, 5

I would write them as 1**1, 1**2 or something if I was trying to be clear that
is what I was trying to do.

[mcgreevy, 2017/06/30 01:24:19]

OK, done.

+						},
+						{
+							Path:    "/rootDir/pathb",
+							RelPath: "pathb",
+							Mode:    os.ModeSymlink,
+							Size:    2,
+						},
+					},
+					size: 3,

[mithro, 2017/06/27 07:48:10]

Size 3?

Is this the cumulative size / total size?

[mcgreevy, 2017/06/28 01:25:50]

total size.

+				},
+			},
+		},
+		{
+			name: "aggregates large file sizes",
+			files: []file{
+				regularFile("/rootDir/patha", 100e3),
+				regularFile("/rootDir/pathb", 200e3),
+			},
+			want: partitionedDeps{
+				indivFiles: itemGroup{
+					items: []*Item{
+						{
+							Path:    "/rootDir/patha",
+							RelPath: "patha",
+							Mode:    0,
+							Size:    100e3,
+						},
+						{
+							Path:    "/rootDir/pathb",
+							RelPath: "pathb",
+							Mode:    0,
+							Size:    200e3,
+						},
+					},
+					size: 300e3,
+				},
+			},
+		},
+		{
+			name: "aggregates small file sizes",
+			files: []file{
+				regularFile("/rootDir/patha", 10e3),
+				regularFile("/rootDir/pathb", 20e3),
+			},
+			want: partitionedDeps{
+				archiveFiles: itemGroup{
+					items: []*Item{
+						{
+							Path:    "/rootDir/patha",
+							RelPath: "patha",
+							Mode:    0,
+							Size:    10e3,
+						},
+						{
+							Path:    "/rootDir/pathb",
+							RelPath: "pathb",
+							Mode:    0,
+							Size:    20e3,
+						},
+					},
+					size: 30e3,
+				},
+			},
+		},
+		{
+			name:      "returns errors unchanged",
+			files:     []file{regularFile("/rootDir/patha", 1)},
+			walkFnErr: errBang,
+			want:      partitionedDeps{},
+			wantErr:   errBang,
+		},
+		{
+			name:    "does nothing for directories",
+			files:   []file{directory("/rootDir/patha")},
+			want:    partitionedDeps{},
+			wantErr: nil,
+		},
+	} {
+		pw := partitionWalker{rootDir: "/rootDir"}
+		for _, f := range tc.files {
+			err := pw.walkFn(f.path, f.bfi, tc.walkFnErr)
+			if err != tc.wantErr {
+				t.Errorf("partitioning deps(%s): walkFn got err %v; want %v", tc.name, err, tc.wantErr)
+				continue TestCases

[mithro, 2017/06/27 07:48:10]

I don't quite get the usage of the continue here?

[mcgreevy, 2017/06/28 01:25:51]

If we get an unexpected error from walkFunc, there is no point in checking got
vs want below.  This could be t.Fatalf, except this is a table-driven test and
we still want to test the other test cases. So instead of using t.Fatalf, I use
continue to skip to the next for loop iteration and run the next test case.

[mithro, 2017/06/29 07:25:17]

I think the biggest problem is that the label for the continue is a long way
away from the place it is used. This could be solved by not defining the test
cases in the for statement.

I also dislike the use of the continue to a label.

I think I would rewrite this in the following form;
-----

testCases := []testCase{
}

for _, tc := range testCases {
  err = nil
  for _, f := range tc.files {
    err = pw.walkFn(f.path, f.fbi, tc.walkFnErr)
    if err != nil {
       break
    }
  }

  if err != tc.wantErr {
    t.Errorf("partitioning deps(%s): walkFn got err %v; want %v", tc.name, err,
tc.wantErr)
    continue
  }

  if got, want := pw.parts, tc.want; !reflect.DeepEqual(got, want) {
    t.Errorf("partitioning deps(%s): got %#v; want %#v", tc.name, got, want)
  }

}
-----

Ultimately you have done more go code then I have, so if you think this is more
closer to the Go style I'm okay with going with your recommendation.

[mcgreevy, 2017/06/30 01:24:19]

Looking at this more closely, I think that it is wrong to have a single
"wantErr" that is compared to the return value of potentially multiple calls to
walkFn. I'm pulling these cases into another test to fix that.

I've moved the label down, as you suggested.

As for removing the label, I think that it is more robust, when encountering an
expected error here, to bail out of the test case immediately, rather than set
up some state (namely, storing the error in an err variable) that then needs to
be inspected again after breaking from the loop. It's analogous to an early
return. Note, the label here is not being used with something unstructured like
a goto, it's merely a way of indicating which level of for loop you wish to
continue.

I mentioned above that the continue is analogous to an early return, and in fact
another way to address this would be to extract the entire body of the for loop
into a helper function.  Then the continue would actually become a return.  That
would be a good solution, except for the fact that the helper function would
need access to the testCase type, which means that either:

(a) the definition of the testCase type would need to be moved outside of
TestWalkFn (not ideal, since no other code needs to know about it; also not the
idiomatic way of writing table-driven tests), or

(b) the helper function would need to be defined inside TestWalkFn.  I think
that would just overcomplicate things and make it harder to read.

For these reasons, and the fact that labels are commonly used in this way in Go,
I'm going to retain the use of the label.

+			}
+		}
+		if got, want := pw.parts, tc.want; !reflect.DeepEqual(got, want) {
+			t.Errorf("partitioning deps(%s): got %#v; want %#v", tc.name, got, want)
+
+		}
+	}
+
+}