Repository: tilt-dev/tilt
Pull request #4341 (2021-03-18 19:05:07)

filewatch: add more reconciler tests
These cover the reconciler basics (i.e. do we actually start watching
for files?) as well as some of the more subtle/error-prone parts
such as implicit ephemeral ignores.

On the latter front, it'd be good to do some refactoring later on and
push some of that down a level so that the code at the controller level
is a bit "dumber" and just passes a spec to something else rather than
needing to do a bunch of conversion itself. It's definitely not critical
though, and the tests here should find any regressions on that front.
Thankfully "how do specs get created from manifests" has very good
coverage in `manifest_subscriber_test.go` (i.e. what used to be the
watch manager tests). Basically, as we rip this stuff apart, we need to
make sure we're testing the right things at the right levels and probably
beef up our integration test suite slightly as well.


Patch:
diff --git a/internal/controllers/core/filewatch/controller.go b/internal/controllers/core/filewatch/controller.go
index 262fe9954..e5c06d4c2 100644
--- a/internal/controllers/core/filewatch/controller.go
+++ b/internal/controllers/core/filewatch/controller.go
@@ -141,7 +141,10 @@ func (c *Controller) addOrReplace(ctx context.Context, st store.RStore, name typ
 		return fmt.Errorf("failed to initialize filesystem watch: %v", err)
 	}
 
-	fw.Status.MonitorStartTime = metav1.NowMicro()
+	// replace the entirety of status to clear out any old events
+	fw.Status = filewatches.FileWatchStatus{
+		MonitorStartTime: metav1.NowMicro(),
+	}
 	if err := c.Client.Status().Update(ctx, fw); err != nil {
 		_ = notify.Close()
 		return fmt.Errorf("failed to update monitor start time: %v", err)
diff --git a/internal/controllers/fake/fixture.go b/internal/controllers/fake/fixture.go
index 8ebbd4210..8e82d26b9 100644
--- a/internal/controllers/fake/fixture.go
+++ b/internal/controllers/fake/fixture.go
@@ -36,6 +36,7 @@ type object interface {
 type ControllerFixture struct {
 	t          testing.TB
 	ctx        context.Context
+	cancel     context.CancelFunc
 	controller controller
 	Scheme     *runtime.Scheme
 	Client     ctrlclient.Client
@@ -56,12 +57,22 @@ func NewFixture(t testing.TB, c controller) *ControllerFixture {
 	return &ControllerFixture{
 		t:          t,
 		ctx:        ctx,
+		cancel:     cancel,
 		Scheme:     cli.Scheme(),
 		Client:     cli,
 		controller: c,
 	}
 }
 
+// Cancel cancels the internal context used for the controller and client requests.
+//
+// Normally, it's not necessary to call this - the fixture will automatically cancel the context as part of test
+// cleanup to avoid leaking resources. However, if you want to explicitly test how a controller reacts to context
+// cancellation, this method can be used.
+func (f ControllerFixture) Cancel() {
+	f.cancel()
+}
+
 func (f *ControllerFixture) RootContext() context.Context {
 	return f.ctx
 }
@@ -72,17 +83,20 @@ func (f *ControllerFixture) TimeoutContext() context.Context {
 	return ctx
 }
 
-func (f *ControllerFixture) MustReconcile(name string) ctrl.Result {
+func (f *ControllerFixture) KeyForObject(o object) types.NamespacedName {
+	return types.NamespacedName{Namespace: o.GetNamespace(), Name: o.GetName()}
+}
+
+func (f *ControllerFixture) MustReconcile(key types.NamespacedName) ctrl.Result {
 	f.t.Helper()
-	key := types.NamespacedName{Name: name}
 	res, err := f.controller.Reconcile(f.TimeoutContext(), ctrl.Request{NamespacedName: key})
 	require.NoError(f.t, err)
 	return res
 }
 
-func (f *ControllerFixture) Get(name string, out object) bool {
+func (f *ControllerFixture) Get(key types.NamespacedName, out object) bool {
 	f.t.Helper()
-	err := f.Client.Get(f.ctx, types.NamespacedName{Name: name}, out)
+	err := f.Client.Get(f.ctx, key, out)
 	if apierrors.IsNotFound(err) {
 		return false
 	}
@@ -90,34 +104,35 @@ func (f *ControllerFixture) Get(name string, out object) bool {
 	return true
 }
 
-func (f *ControllerFixture) MustGet(name string, out object) {
+func (f *ControllerFixture) MustGet(key types.NamespacedName, out object) {
 	f.t.Helper()
-	found := f.Get(name, out)
+	found := f.Get(key, out)
 	if !found {
 		// don't try to read from object Kind, it's probably not properly populated
-		f.t.Fatalf("%T object %q does not exist", out, name)
+		f.t.Fatalf("%T object %q does not exist", out, key.String())
 	}
 }
 
 func (f *ControllerFixture) Create(o object) ctrl.Result {
 	f.t.Helper()
 	require.NoError(f.t, f.Client.Create(f.ctx, o))
-	return f.MustReconcile(o.GetName())
+	return f.MustReconcile(f.KeyForObject(o))
 }
 
+// Update updates the object including Status subresource.
 func (f *ControllerFixture) Update(o object) ctrl.Result {
 	f.t.Helper()
-	// this is a safe cast since we know the original object type met the interface
-	old := o.New().(object)
-	key := types.NamespacedName{Name: o.GetObjectMeta().GetName()}
-	require.NoError(f.t, f.Client.Get(f.ctx, key, old))
-	o.GetObjectMeta().SetResourceVersion(old.GetResourceVersion())
 	require.NoError(f.t, f.Client.Update(f.ctx, o))
-	return f.MustReconcile(o.GetName())
+	return f.MustReconcile(f.KeyForObject(o))
 }
 
-func (f *ControllerFixture) Delete(o object) ctrl.Result {
+func (f *ControllerFixture) Delete(o object) (bool, ctrl.Result) {
 	f.t.Helper()
-	require.NoError(f.t, f.Client.Delete(f.ctx, o))
-	return f.MustReconcile(o.GetName())
+	err := f.Client.Delete(f.ctx, o)
+	require.NoError(f.t, ctrlclient.IgnoreNotFound(err))
+	if apierrors.IsNotFound(err) {
+		// skip reconciliation since no object was deleted
+		return false, ctrl.Result{}
+	}
+	return true, f.MustReconcile(f.KeyForObject(o))
 }
diff --git a/internal/engine/fswatch/manifest_subscriber.go b/internal/engine/fswatch/manifest_subscriber.go
index de9deef9e..d21f930b1 100644
--- a/internal/engine/fswatch/manifest_subscriber.go
+++ b/internal/engine/fswatch/manifest_subscriber.go
@@ -75,8 +75,6 @@ func (w ManifestSubscriber) OnChange(ctx context.Context, st store.RStore, summa
 
 			updated := existing.DeepCopy()
 			fw.Spec.DeepCopyInto(&updated.Spec)
-			// reset status since the spec changed
-			updated.Status = filewatches.FileWatchStatus{}
 			err := w.client.Update(ctx, updated)
 			if err == nil {
 				st.Dispatch(NewFileWatchUpdateAction(updated))


Test patch:
diff --git a/internal/controllers/core/filewatch/controller_test.go b/internal/controllers/core/filewatch/controller_test.go
index 2643ea9a1..e46f8d382 100644
--- a/internal/controllers/core/filewatch/controller_test.go
+++ b/internal/controllers/core/filewatch/controller_test.go
@@ -1,7 +1,6 @@
 package filewatch
 
 import (
-	"context"
 	"fmt"
 	"path/filepath"
 	"reflect"
@@ -10,6 +9,8 @@ import (
 	"testing"
 	"time"
 
+	"k8s.io/apimachinery/pkg/types"
+
 	"github.com/tilt-dev/tilt/internal/controllers/core/filewatch/fsevent"
 
 	"github.com/stretchr/testify/assert"
@@ -18,7 +19,6 @@ import (
 
 	"github.com/tilt-dev/tilt/internal/controllers/fake"
 	"github.com/tilt-dev/tilt/internal/store"
-	"github.com/tilt-dev/tilt/internal/testutils"
 	"github.com/tilt-dev/tilt/internal/testutils/tempdir"
 	"github.com/tilt-dev/tilt/internal/watch"
 	"github.com/tilt-dev/tilt/pkg/apis"
@@ -28,18 +28,14 @@ import (
 type fixture struct {
 	*fake.ControllerFixture
 	t                testing.TB
-	ctx              context.Context
 	tmpdir           *tempdir.TempDirFixture
+	controller       *Controller
 	store            *store.TestingStore
 	fakeMultiWatcher *fsevent.FakeMultiWatcher
 	fakeTimerMaker   fsevent.FakeTimerMaker
 }
 
 func newFixture(t *testing.T) *fixture {
-	ctx, _, _ := testutils.CtxAndAnalyticsForTest()
-	ctx, cancel := context.WithCancel(ctx)
-	t.Cleanup(cancel)
-
 	tmpdir := tempdir.NewTempDirFixture(t)
 	t.Cleanup(tmpdir.TearDown)
 	tmpdir.Chdir()
@@ -55,77 +51,87 @@ func newFixture(t *testing.T) *fixture {
 	return &fixture{
 		ControllerFixture: ctrlFixture,
 		t:                 t,
-		ctx:               ctx,
 		tmpdir:            tmpdir,
+		controller:        controller,
 		store:             testingStore,
 		fakeMultiWatcher:  fakeMultiWatcher,
 		fakeTimerMaker:    timerMaker,
 	}
 }
 
-func (f *fixture) ChangeFile(t testing.TB, path string) {
-	path, _ = filepath.Abs(path)
+func (f *fixture) ChangeAndWaitForSeenFile(key types.NamespacedName, pathElems ...string) {
+	f.t.Helper()
+	f.ChangeFile(pathElems...)
+	f.WaitForSeenFile(key, pathElems...)
+}
 
+func (f *fixture) ChangeFile(elem ...string) {
+	f.t.Helper()
+	path, err := filepath.Abs(f.tmpdir.JoinPath(elem...))
+	require.NoErrorf(f.t, err, "Could not get abs path for %q", path)
 	select {
 	case f.fakeMultiWatcher.Events <- watch.NewFileEvent(path):
 	default:
-		t.Fatal("emitting a FileEvent would block. Perhaps there are too many events or the buffer size is too small.")
+		f.t.Fatal("emitting a FileEvent would block. Perhaps there are too many events or the buffer size is too small.")
 	}
 }
 
-func (f *fixture) WaitForSeenFile(name string, path string) []string {
+func (f *fixture) WaitForSeenFile(key types.NamespacedName, pathElems ...string) {
 	f.t.Helper()
+	relPath := filepath.Join(pathElems...)
 	var seenPaths []string
 	require.Eventuallyf(f.t, func() bool {
 		seenPaths = nil
 		var fw filewatches.FileWatch
-		if !f.Get(name, &fw) {
+		if !f.Get(key, &fw) {
 			return false
 		}
 		found := false
 		for _, e := range fw.Status.FileEvents {
 			for _, p := range e.SeenFiles {
+				// relativize all the paths before comparison/storage
+				// (this makes the test output way more comprehensible on failure by hiding all the tmpdir cruft)
 				p, _ = filepath.Rel(f.tmpdir.Path(), p)
-				if p == path {
+				if p == relPath {
 					found = true
 				}
 				seenPaths = append(seenPaths, p)
 			}
 		}
 		return found
-	}, 2*time.Second, 20*time.Millisecond, "Did not find path %q, seen: %v", path, &seenPaths)
-	return seenPaths
+	}, 2*time.Second, 20*time.Millisecond, "Did not find path %q, seen: %v", relPath, &seenPaths)
 }
 
-func (f *fixture) CreateSimpleFileWatch() *filewatches.FileWatch {
-	name := apis.SanitizeName(f.t.Name())
+func (f *fixture) CreateSimpleFileWatch() (types.NamespacedName, *filewatches.FileWatch) {
+	f.t.Helper()
 	fw := &filewatches.FileWatch{
-		ObjectMeta: metav1.ObjectMeta{Name: name},
+		ObjectMeta: metav1.ObjectMeta{
+			Namespace: apis.SanitizeName(f.t.Name()),
+			Name:      "test-file-watch",
+		},
 		Spec: filewatches.FileWatchSpec{
-			WatchedPaths: []string{f.tmpdir.Path()},
+			WatchedPaths: []string{f.tmpdir.JoinPath("a"), f.tmpdir.JoinPath("b", "c")},
 		},
 	}
 	f.Create(fw)
-	return fw
+	return f.KeyForObject(fw), fw
 }
 
 func TestController_LimitFileEventsHistory(t *testing.T) {
 	f := newFixture(t)
 
-	fw := f.CreateSimpleFileWatch()
+	key, fw := f.CreateSimpleFileWatch()
 
 	const eventOverflowCount = 5
 	for i := 0; i < MaxFileEventHistory+eventOverflowCount; i++ {
-		p := strconv.Itoa(i)
-		f.ChangeFile(t, p)
 		// need to wait for each file 1-by-1 to prevent batching
-		f.WaitForSeenFile(fw.Name, p)
+		f.ChangeAndWaitForSeenFile(key, "a", strconv.Itoa(i))
 	}
 
-	f.MustGet(fw.Name, fw)
+	f.MustGet(key, fw)
 	require.Equal(t, MaxFileEventHistory, len(fw.Status.FileEvents), "Wrong number of file events")
 	for i := 0; i < len(fw.Status.FileEvents); i++ {
-		p := f.tmpdir.JoinPath(strconv.Itoa(i + eventOverflowCount))
+		p := f.tmpdir.JoinPath("a", strconv.Itoa(i+eventOverflowCount))
 		assert.Contains(t, fw.Status.FileEvents[i].SeenFiles, p)
 	}
 }
@@ -143,3 +149,120 @@ func TestController_ShortRead(t *testing.T) {
 		assert.Contains(t, storeErr.Error(), "https://github.com/tilt-dev/tilt/issues/3556")
 	}
 }
+
+func TestController_IgnoreEphemeralFiles(t *testing.T) {
+	f := newFixture(t)
+	key, orig := f.CreateSimpleFileWatch()
+	// spec should have no ignores - these are purely implicit ignores
+	require.Empty(t, orig.Spec.Ignores)
+
+	// sandwich in some ignored files with seen files on the outside as synchronization
+	f.ChangeAndWaitForSeenFile(key, "a", "start")
+	// see internal/ignore/ephemeral.go for where these come from - they're NOT part of a FileWatch spec
+	// but are always included at the filesystem watcher level by Tilt
+	f.ChangeFile("a", ".idea", "workspace.xml")
+	f.ChangeFile("b", "c", ".vim.swp")
+	f.ChangeAndWaitForSeenFile(key, "b", "c", "stop")
+
+	var fw filewatches.FileWatch
+	f.MustGet(key, &fw)
+	require.Equal(t, 2, len(fw.Status.FileEvents), "Wrong file event count")
+	assert.Equal(t, []string{f.tmpdir.JoinPath("a", "start")}, fw.Status.FileEvents[0].SeenFiles)
+	assert.Equal(t, []string{f.tmpdir.JoinPath("b", "c", "stop")}, fw.Status.FileEvents[1].SeenFiles)
+}
+
+// TestController_Watcher_Cancel peeks into internal/unexported portions of the controller to inspect the actual
+// filesystem monitor so it can ensure reconciler is not leaking resources; other tests should prefer observing
+// desired state!
+func TestController_Watcher_Cancel(t *testing.T) {
+	f := newFixture(t)
+	key, _ := f.CreateSimpleFileWatch()
+
+	assert.Equalf(t, 1, len(f.controller.targetWatches), "There should be exactly one file watcher")
+	watcher := f.controller.targetWatches[key]
+	require.NotNilf(t, watcher, "Watcher does not exist for %q", key.String())
+
+	// cancel the root context, which should propagate to the watcher
+	f.Cancel()
+
+	require.Eventuallyf(t, func() bool {
+		watcher.mu.Lock()
+		defer watcher.mu.Unlock()
+		return watcher.done
+	}, time.Second, 10*time.Millisecond, "Watcher was never cleaned up")
+}
+
+func TestController_Reconcile_Create(t *testing.T) {
+	f := newFixture(t)
+	key, fw := f.CreateSimpleFileWatch()
+
+	f.MustGet(key, fw)
+	assert.NotZero(t, fw.Status.MonitorStartTime, "Filesystem monitor was not started")
+}
+
+// TestController_Reconcile_Delete peeks into internal/unexported portions of the controller to inspect the actual
+// filesystem monitor so it can ensure reconciler is not leaking resources; other tests should prefer observing
+// desired state!
+func TestController_Reconcile_Delete(t *testing.T) {
+	f := newFixture(t)
+	key, fw := f.CreateSimpleFileWatch()
+
+	assert.Equalf(t, 1, len(f.controller.targetWatches), "There should be exactly one file watcher")
+	watcher := f.controller.targetWatches[key]
+	require.NotNilf(t, watcher, "Watcher does not exist for %q", key.String())
+
+	deleted, _ := f.Delete(fw)
+	require.True(t, deleted, "FileWatch was not deleted")
+
+	watcher.mu.Lock()
+	defer watcher.mu.Unlock()
+	require.True(t, watcher.done, "Watcher was not stopped")
+	require.Empty(t, f.controller.targetWatches, "There should not be any remaining file watchers")
+}
+
+func TestController_Reconcile_Watches(t *testing.T) {
+	f := newFixture(t)
+	key, fw := f.CreateSimpleFileWatch()
+
+	f.ChangeAndWaitForSeenFile(key, "a", "1")
+
+	f.MustGet(key, fw)
+	originalStart := fw.Status.MonitorStartTime.Time
+	assert.NotZero(t, originalStart, "Filesystem monitor was not started")
+
+	fw.Spec.Ignores = []filewatches.IgnoreDef{
+		{
+			BasePath: f.tmpdir.Path(),
+			Patterns: []string{"**/ignore_me"},
+		},
+		{
+			// no patterns means ignore the path recursively
+			BasePath: f.tmpdir.JoinPath("d", "ignore_dir"),
+		},
+	}
+	fw.Spec.WatchedPaths = []string{f.tmpdir.JoinPath("d")}
+	f.Update(fw)
+
+	// sandwich in some ignored files with seen files on the outside as synchronization
+	f.ChangeAndWaitForSeenFile(key, "d", "1")
+	f.ChangeFile("a", "2")
+	f.ChangeFile("d", "ignore_me")
+	f.ChangeFile("d", "ignore_dir", "file")
+	f.ChangeAndWaitForSeenFile(key, "d", "2")
+
+	var updated filewatches.FileWatch
+	f.MustGet(key, &updated)
+	updatedStart := updated.Status.MonitorStartTime.Time
+	assert.Truef(t, updatedStart.After(originalStart),
+		"Monitor start time should be more recent after update, (original: %s, updated: %s)",
+		originalStart, updatedStart)
+	if assert.Equal(t, 2, len(updated.Status.FileEvents)) {
+		// ensure ONLY the expected files were seen
+		assert.NotZero(t, updated.Status.FileEvents[0].Time.Time)
+		mostRecentEventTime := updated.Status.FileEvents[1].Time.Time
+		assert.NotZero(t, mostRecentEventTime)
+		assert.Equal(t, []string{f.tmpdir.JoinPath("d", "1")}, updated.Status.FileEvents[0].SeenFiles)
+		assert.Equal(t, []string{f.tmpdir.JoinPath("d", "2")}, updated.Status.FileEvents[1].SeenFiles)
+		assert.Equal(t, mostRecentEventTime, updated.Status.LastEventTime.Time)
+	}
+}