isolate: (refactor) extract blacklisting into common package

client/internal/common/filesystem_view_linux_test.go

+// Copyright 2017 The LUCI Authors.

[mithro, 2017/07/19 04:48:41]

Why filesystem_view_linux_test.go ?

Is there a windows / mac version?

[mcgreevy, 2017/07/19 05:11:04]

The presence of "_linux" before "_test" will cause this to run only on linux.
filepath.Match will have different behaviours depending on the platform. My
intention here is not to test the platform-specific behaviour of the
filepath.Match implementation, since that's already tested by they author of
that code. But I do need to run the tests on *some* platform. I chose linux.

[mithro, 2017/07/19 05:46:18]

Are blacklist patterns OS dependent?

I feel like there could be possible interactions between filepath.Match for
blacklisting and the relative path conversion -- specially around driver letters
and path separators. Especially where both things are working as expected but
the result is _not_ what was expected.

[mcgreevy, 2017/07/21 04:15:50]

In practice, blacklists look to be pretty simple, either the defaults:

https://cs.chromium.org/chromium/infra/go/src/github.com/luci/luci-go/client/...

or a simple list of file names:

https://cs.chromium.org/chromium/src/third_party/skia/infra/bots/ct/blacklist...


and in fact, the --blacklist flag for the archive command incorrectly describes
the patterns as regexps when they're actually globs.

So this code is already brittle in the sense that it is under- and
incorrectly-documented, and I don't intend to spend a lot of time improving it. 
I'll fix the documentation of the blacklist flag and leave it at that.

+//
+// 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 common
+
+import (
+        "testing"
+
+        . "github.com/smartystreets/goconvey/convey"
+)
+
+func TestValidatesBlacklist(t *testing.T) {
+        type testCase struct {
+                desc      string
+                blacklist []string
+                wantErr   bool
+        }
+
+        testCases := []testCase{
+                {
+                        desc:      "no patterns",
+                        blacklist: []string{},
+                        wantErr:   false,
+                },
+                {
+                        desc:      "good pattern",
+                        blacklist: []string{"*"},
+                        wantErr:   false,
+                },
+                {
+                        desc:      "good patterns",
+                        blacklist: []string{"a*", "b*"},
+                        wantErr:   false,
+                },
+                {
+                        desc:      "bad pattern",
+                        blacklist: []string{"["},
+                        wantErr:   true,
+                },
+                {
+                        desc:      "bad first pattern",
+                        blacklist: []string{"[", "*"},
+                        wantErr:   true,
+                },
+                {
+                        desc:      "bad last pattern",
+                        blacklist: []string{"*", "["},
+                        wantErr:   true,

[mithro, 2017/07/19 04:48:41]

bad middle pattern?

[mcgreevy, 2017/07/19 05:11:04]

I don't think that's a case worth testing.

[mithro, 2017/07/19 05:46:19]

If you don't really think it is worth testing, I'm okay with you not adding it.

I'm not sure about Go, but in many other languages 0, 1, 2 and "many" items are
all worth checking. The "2 element" case tends to be different from the "3
element" case if the first & last element cases are both special cases (which is
not uncommon) it doesn't test the "non-special case".

In Python it is also pretty easy to accidentally do a "return a[0], a[1]" rather
than a "return a[0], a[-1]" or a "return a[0], a[1:]" which could pass a lot of
these cases accidentally :-)

[mcgreevy, 2017/07/21 04:15:50]

The main failure mode I'm trying to cover here is where the implementation
determines whether there was a blacklist match based only on a single element
from the blacklist, rather than all of them. I think this covers it.

+                },
+        }
+
+        for _, tc := range testCases {
+                tc := tc
+                Convey(tc.desc, t, func() {
+                        _, err := NewFilesystemView("/root", tc.blacklist)
+                        if tc.wantErr {
+                                So(err, ShouldNotBeNil)
+                        } else {
+                                So(err, ShouldBeNil)
+                        }
+                })
+        }
+
+}
+
+func TestCalculatesRelativePaths(t *testing.T) {
+        type testCase struct {
+                desc        string
+                root        string
+                absPath     string
+                wantRelPath string
+                wantErr     bool
+        }
+
+        testCases := []testCase{
+                {
+                        desc:        "no trailing slash",
+                        root:        "/a",
+                        absPath:     "/a/b",
+                        wantRelPath: "b",
+                        wantErr:     false,
+                },
+                {
+                        desc:        "trailing slash",
+                        root:        "/a",
+                        absPath:     "/a/b/",
+                        wantRelPath: "b",
+                        wantErr:     false,
+                },
+                {
+                        desc:        "no common root",
+                        root:        "/a",
+                        absPath:     "/x/y",
+                        wantRelPath: "../x/y",
+                        wantErr:     false,
+                },
+                {
+                        desc:        "no computable relative path",
+                        root:        "/a",
+                        absPath:     "./x/y",
+                        wantRelPath: "",
+                        wantErr:     true,
+                },
+                {
+                        desc:        "root",
+                        root:        "/a",
+                        absPath:     "/a",
+                        wantRelPath: ".",
+                        wantErr:     false,
+                },
+        }
+
+        for _, tc := range testCases {
+                tc := tc
+                Convey(tc.desc, t, func() {
+                        fsView, err := NewFilesystemView(tc.root, nil)
+
+                        So(err, ShouldBeNil)
+
+                        relPath, err := fsView.RelativePath(tc.absPath)
+                        So(relPath, ShouldEqual, tc.wantRelPath)
+                        if tc.wantErr {
+                                So(err, ShouldNotBeNil)
+                        } else {
+                                So(err, ShouldBeNil)
+                        }
+                })
+        }
+}
+
+func TestAppliesBlacklist(t *testing.T) {

[mithro, 2017/07/19 05:46:19]

BTW Does blacklisting (via filepath.Match) support the full globbing syntax?

I might think about add a test which makes sure that escaping isn't some getting
mangled in some way. This probably isn't a common problem in Go but certainly in
bash scripts it comes up a lot...

There might also be something around on Windows where the file path separator is
`\` and escaping / matching.

[mcgreevy, 2017/07/21 04:15:50]

As mentioned in a comment above, this feature is already pretty badly documented
(the doc for the flag said regex instead of glob), and I don't propose to go
through and fix everything that's wrong with it. I'm just trying not to break
anything that's currently using it, and adding tests to ensure that my
refactoring is not causing any new breakage.

+        type testCase struct {
+                desc        string
+                root        string
+                blacklist   []string
+                absPath     string
+                wantRelPath string
+        }
+
+        testCases := []testCase{
+                {
+                        desc:        "no blacklist",
+                        root:        "/a",
+                        blacklist:   []string{},
+                        absPath:     "/a/x/y",
+                        wantRelPath: "x/y",
+                },
+                {
+                        desc:        "blacklist matches relative path",
+                        root:        "/a",
+                        blacklist:   []string{"?/z"},
+                        absPath:     "/a/x/z",
+                        wantRelPath: "",
+                },
+                {
+                        desc:        "blacklist doesn't match relative path",
+                        root:        "/a",
+                        blacklist:   []string{"?/z"},
+                        absPath:     "/a/x/y",
+                        wantRelPath: "x/y",
+                },
+                {
+                        desc:        "blacklist matches basename",
+                        root:        "/a",
+                        blacklist:   []string{"z"},
+                        absPath:     "/a/x/z",
+                        wantRelPath: "",
+                },
+                {
+                        desc:        "blacklist doesn't match basename",
+                        root:        "/a",
+                        blacklist:   []string{"z"},
+                        absPath:     "/a/z/y",
+                        wantRelPath: "z/y",
+                },
+                {
+                        desc:        "root never matches blacklist",
+                        root:        "/a",
+                        blacklist:   []string{"?"},
+                        absPath:     "/a",
+                        wantRelPath: ".",
+                },
+                {
+                        desc:        "only one blacklist need match path (1)",
+                        root:        "/a",
+                        blacklist:   []string{"z", "abc"},
+                        absPath:     "/a/x/z",
+                        wantRelPath: "",
+                },
+                {
+                        desc:        "only one blacklist need match path (2)",
+                        root:        "/a",
+                        blacklist:   []string{"abc", "z"},
+                        absPath:     "/a/x/z",
+                        wantRelPath: "",
+                },
+        }
+
+        for _, tc := range testCases {
+                tc := tc
+                Convey(tc.desc, t, func() {
+                        fsView, err := NewFilesystemView(tc.root, tc.blacklist)
+
+                        // These test cases contain only valid blacklists.
+                        // Invalid blacklists are tested in TestValidatesBlacklist.
+                        So(err, ShouldBeNil)

[mithro, 2017/07/19 05:46:19]

My mind keeps reading this as "Should Be Neil" :-P

[mcgreevy, 2017/07/21 04:15:50]

Acknowledged.

+
+                        relPath, err := fsView.RelativePath(tc.absPath)
+                        So(relPath, ShouldEqual, tc.wantRelPath)

[mithro, 2017/07/19 05:46:19]

It "feels" like you be checking err value first and then comparing relPath. Your
comment explains why it is perfectly safe to do things in this order, but that
comes *after* you have red the potentially "unsafe code".

[mcgreevy, 2017/07/21 04:15:50]

Done.

+
+                        // These test cases contain only valid relative paths.
+                        // non-computable relative paths are tested in TestCalculatesRelativePaths.
+                        So(err, ShouldBeNil)
+                })
+        }
+}