From db4806de53fcc7b744325af547f41717ee5e5605 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Sat, 5 Sep 2026 11:16:39 +0200 Subject: [PATCH 1/2] remove jonboulle/clockwork for testing/synctest The fake clock is only used by tests to control the debounce timer, but requires carrying a clock abstraction through production code. Use the standard time package for the debounce ticker and use testing/synctest to control time in tests instead. This removes the clock from composeService and BatchDebounceEvents, and eliminates the clockwork dependency from the watch implementation. Signed-off-by: Sebastiaan van Stijn --- go.mod | 1 - go.sum | 2 - pkg/compose/compose.go | 3 - pkg/compose/watch.go | 2 +- pkg/compose/watch_test.go | 178 ++++++++++++++++++------------------- pkg/watch/debounce.go | 7 +- pkg/watch/debounce_test.go | 58 ++++++------ 7 files changed, 114 insertions(+), 137 deletions(-) diff --git a/go.mod b/go.mod index c061e04d5c9..d644b783185 100644 --- a/go.mod +++ b/go.mod @@ -24,7 +24,6 @@ require ( github.com/google/go-cmp v0.7.0 github.com/google/uuid v1.6.0 github.com/hashicorp/go-version v1.9.0 - github.com/jonboulle/clockwork v0.5.0 github.com/mattn/go-shellwords v1.0.14 github.com/mitchellh/go-ps v1.0.0 github.com/moby/buildkit v0.33.0 diff --git a/go.sum b/go.sum index 92032c13677..8447f1e53aa 100644 --- a/go.sum +++ b/go.sum @@ -139,8 +139,6 @@ github.com/inconshreveable/mousetrap v1.1.0 h1:wN+x4NVGpMsO7ErUn/mUI3vEoE6Jt13X2 github.com/inconshreveable/mousetrap v1.1.0/go.mod h1:vpF70FUmC8bwa3OWnCshd2FqLfsEA9PFc4w1p2J65bw= github.com/inhies/go-bytesize v0.0.0-20220417184213-4913239db9cf h1:FtEj8sfIcaaBfAKrE1Cwb61YDtYq9JxChK1c7AKce7s= github.com/inhies/go-bytesize v0.0.0-20220417184213-4913239db9cf/go.mod h1:yrqSXGoD/4EKfF26AOGzscPOgTTJcyAwM2rpixWT+t4= -github.com/jonboulle/clockwork v0.5.0 h1:Hyh9A8u51kptdkR+cqRpT1EebBwTn1oK9YfGYbdFz6I= -github.com/jonboulle/clockwork v0.5.0/go.mod h1:3mZlmanh0g2NDKO5TWZVJAfofYk64M7XN3SzBPjZF60= github.com/kballard/go-shellquote v0.0.0-20180428030007-95032a82bc51 h1:Z9n2FFNUXsshfwJMBgNA0RU6/i7WVaAegv3PtuIHPMs= github.com/kballard/go-shellquote v0.0.0-20180428030007-95032a82bc51/go.mod h1:CzGEWj7cYgsdH8dAjBGEr58BoE7ScuLd+fwFZ44+/x8= github.com/klauspost/compress v1.19.2 h1:hMRETovs/pu/dVWN7zIT1PGG8t509MwT6bO7XSi26R8= diff --git a/pkg/compose/compose.go b/pkg/compose/compose.go index e927e150f35..dd7b9dd12bc 100644 --- a/pkg/compose/compose.go +++ b/pkg/compose/compose.go @@ -30,7 +30,6 @@ import ( "github.com/docker/cli/cli/config/configfile" "github.com/docker/cli/cli/flags" "github.com/docker/cli/cli/streams" - "github.com/jonboulle/clockwork" "github.com/moby/moby/api/types/container" "github.com/moby/moby/api/types/swarm" "github.com/moby/moby/client" @@ -65,7 +64,6 @@ type Option func(service *composeService) error func NewComposeService(dockerCli command.Cli, options ...Option) (api.Compose, error) { s := &composeService{ dockerCli: dockerCli, - clock: clockwork.NewRealClock(), maxConcurrency: -1, dryRun: false, } @@ -211,7 +209,6 @@ type composeService struct { contextInfo api.ContextInfo proxyConfig map[string]string - clock clockwork.Clock maxConcurrency int dryRun bool diff --git a/pkg/compose/watch.go b/pkg/compose/watch.go index f50c967a45a..baba7707d69 100644 --- a/pkg/compose/watch.go +++ b/pkg/compose/watch.go @@ -376,7 +376,7 @@ func (s *composeService) watchEvents(ctx context.Context, project *types.Project defer cancel() // debounce and group filesystem events so that we capture IDE saving many files as one "batch" event - batchEvents := watch.BatchDebounceEvents(ctx, s.clock, watcher.Events()) + batchEvents := watch.BatchDebounceEvents(ctx, watcher.Events()) for { select { diff --git a/pkg/compose/watch_test.go b/pkg/compose/watch_test.go index 02b5635c4de..963b5180b59 100644 --- a/pkg/compose/watch_test.go +++ b/pkg/compose/watch_test.go @@ -22,11 +22,11 @@ import ( "path/filepath" "slices" "testing" + "testing/synctest" "time" "github.com/compose-spec/compose-go/v2/types" "github.com/docker/cli/cli/streams" - "github.com/jonboulle/clockwork" "github.com/moby/moby/api/types/container" "github.com/moby/moby/api/types/image" "github.com/moby/moby/client" @@ -75,87 +75,81 @@ func (s stdLogger) Status(containerName, msg string) { } func TestWatch_Sync(t *testing.T) { - mockCtrl := gomock.NewController(t) - cli := mocks.NewMockCli(mockCtrl) - cli.EXPECT().Err().Return(streams.NewOut(os.Stderr)).AnyTimes() - apiClient := mocks.NewMockAPIClient(mockCtrl) - apiClient.EXPECT().ContainerList(gomock.Any(), gomock.Any()).Return(client.ContainerListResult{ - Items: []container.Summary{ - testContainer("test", "123", false), - }, - }, nil).AnyTimes() - // we expect the image to be pruned - apiClient.EXPECT().ImageList(gomock.Any(), client.ImageListOptions{ - Filters: make(client.Filters). - Add("dangling", "true"). - Add("label", api.ProjectLabel+"=myProjectName"), - }).Return(client.ImageListResult{ - Items: []image.Summary{ - {ID: "123"}, - {ID: "456"}, - }, - }, nil).Times(1) - apiClient.EXPECT().ImageRemove(gomock.Any(), "123", client.ImageRemoveOptions{}).Times(1) - apiClient.EXPECT().ImageRemove(gomock.Any(), "456", client.ImageRemoveOptions{}).Times(1) - // - cli.EXPECT().Client().Return(apiClient).AnyTimes() - - ctx, cancelFunc := context.WithCancel(t.Context()) - t.Cleanup(cancelFunc) - - proj := types.Project{ - Name: "myProjectName", - Services: types.Services{ - "test": { - Name: "test", + synctest.Test(t, func(t *testing.T) { + mockCtrl := gomock.NewController(t) + cli := mocks.NewMockCli(mockCtrl) + cli.EXPECT().Err().Return(streams.NewOut(os.Stderr)).AnyTimes() + apiClient := mocks.NewMockAPIClient(mockCtrl) + apiClient.EXPECT().ContainerList(gomock.Any(), gomock.Any()).Return(client.ContainerListResult{ + Items: []container.Summary{ + testContainer("test", "123", false), }, - }, - } - - watcher := testWatcher{ - events: make(chan watch.FileEvent), - errors: make(chan error), - } + }, nil).AnyTimes() + // we expect the image to be pruned + apiClient.EXPECT().ImageList(gomock.Any(), client.ImageListOptions{ + Filters: make(client.Filters). + Add("dangling", "true"). + Add("label", api.ProjectLabel+"=myProjectName"), + }).Return(client.ImageListResult{ + Items: []image.Summary{ + {ID: "123"}, + {ID: "456"}, + }, + }, nil).Times(1) + apiClient.EXPECT().ImageRemove(gomock.Any(), "123", client.ImageRemoveOptions{}).Times(1) + apiClient.EXPECT().ImageRemove(gomock.Any(), "456", client.ImageRemoveOptions{}).Times(1) + // + cli.EXPECT().Client().Return(apiClient).AnyTimes() + + proj := types.Project{ + Name: "myProjectName", + Services: types.Services{ + "test": { + Name: "test", + }, + }, + } - syncer := newFakeSyncer() - clock := clockwork.NewFakeClock() - go func() { - service := composeService{ - dockerCli: cli, - clock: clock, - maxConcurrency: -1, + watcher := testWatcher{ + events: make(chan watch.FileEvent), + errors: make(chan error), } - rules, err := getWatchRules(&types.DevelopConfig{ - Watch: []types.Trigger{ - { - Path: "/sync", - Action: "sync", - Target: "/work", - Ignore: []string{"ignore"}, - }, - { - Path: "/rebuild", - Action: "rebuild", + + syncer := newFakeSyncer() + go func() { + service := composeService{ + dockerCli: cli, + maxConcurrency: -1, + } + rules, err := getWatchRules(&types.DevelopConfig{ + Watch: []types.Trigger{ + { + Path: "/sync", + Action: "sync", + Target: "/work", + Ignore: []string{"ignore"}, + }, + { + Path: "/rebuild", + Action: "rebuild", + }, }, - }, - }, types.ServiceConfig{Name: "test"}) - assert.NilError(t, err) - - err = service.watchEvents(ctx, &proj, api.WatchOptions{ - Build: &api.BuildOptions{}, - LogTo: stdLogger{}, - Prune: true, - }, watcher, syncer, rules) - assert.NilError(t, err) - }() - - watcher.Events() <- watch.NewFileEvent("/sync/changed") - watcher.Events() <- watch.NewFileEvent("/sync/changed/sub") - err := clock.BlockUntilContext(ctx, 3) - assert.NilError(t, err) - clock.Advance(watch.QuietPeriod) - select { - case actual := <-syncer.synced: + }, types.ServiceConfig{Name: "test"}) + assert.NilError(t, err) + + err = service.watchEvents(t.Context(), &proj, api.WatchOptions{ + Build: &api.BuildOptions{}, + LogTo: stdLogger{}, + Prune: true, + }, watcher, syncer, rules) + assert.NilError(t, err) + }() + + watcher.Events() <- watch.NewFileEvent("/sync/changed") + watcher.Events() <- watch.NewFileEvent("/sync/changed/sub") + time.Sleep(watch.QuietPeriod) + synctest.Wait() + actual := <-syncer.synced expected := []*sync.PathMapping{ {HostPath: "/sync/changed", ContainerPath: "/work/changed"}, {HostPath: "/sync/changed/sub", ContainerPath: "/work/changed/sub"}, @@ -164,22 +158,20 @@ func TestWatch_Sync(t *testing.T) { return cmp.Compare(a.HostPath, b.HostPath) }) assert.DeepEqual(t, expected, actual) - case <-time.After(100 * time.Millisecond): - t.Error("timeout") - } - watcher.Events() <- watch.NewFileEvent("/rebuild") - watcher.Events() <- watch.NewFileEvent("/sync/changed") - err = clock.BlockUntilContext(ctx, 4) - assert.NilError(t, err) - clock.Advance(watch.QuietPeriod) - select { - case batch := <-syncer.synced: - t.Fatalf("received unexpected events: %v", batch) - case <-time.After(100 * time.Millisecond): - // expected - } - // TODO: there's not a great way to assert that the rebuild attempt happened + // Rebuild fails before sync actions from the same batch are processed. + watcher.Events() <- watch.NewFileEvent("/rebuild") + watcher.Events() <- watch.NewFileEvent("/sync/changed") + time.Sleep(watch.QuietPeriod) + synctest.Wait() + select { + case batch := <-syncer.synced: + t.Fatalf("received unexpected events: %v", batch) + default: + // expected + } + // TODO: there's not a great way to assert that the rebuild attempt happened + }) } type fakeSyncer struct { diff --git a/pkg/watch/debounce.go b/pkg/watch/debounce.go index d3cddb7e636..c42f3533fbb 100644 --- a/pkg/watch/debounce.go +++ b/pkg/watch/debounce.go @@ -18,7 +18,6 @@ import ( "context" "time" - "github.com/jonboulle/clockwork" "github.com/sirupsen/logrus" "github.com/docker/compose/v5/pkg/utils" @@ -30,7 +29,7 @@ const QuietPeriod = 500 * time.Millisecond // channel. // // The returned channel is closed when the debouncer is stopped via context cancellation or by closing the input channel. -func BatchDebounceEvents(ctx context.Context, clock clockwork.Clock, input <-chan FileEvent) <-chan []FileEvent { +func BatchDebounceEvents(ctx context.Context, input <-chan FileEvent) <-chan []FileEvent { out := make(chan []FileEvent) go func() { defer close(out) @@ -49,13 +48,13 @@ func BatchDebounceEvents(ctx context.Context, clock clockwork.Clock, input <-cha seen = utils.Set[FileEvent]{} } - t := clock.NewTicker(QuietPeriod) + t := time.NewTicker(QuietPeriod) defer t.Stop() for { select { case <-ctx.Done(): return - case <-t.Chan(): + case <-t.C: flushEvents() case e, ok := <-input: if !ok { diff --git a/pkg/watch/debounce_test.go b/pkg/watch/debounce_test.go index 160309c91db..6c0474f9ac8 100644 --- a/pkg/watch/debounce_test.go +++ b/pkg/watch/debounce_test.go @@ -15,52 +15,44 @@ package watch import ( - "context" "slices" "testing" + "testing/synctest" "time" - "github.com/jonboulle/clockwork" "gotest.tools/v3/assert" ) func Test_BatchDebounceEvents(t *testing.T) { - ch := make(chan FileEvent) - clock := clockwork.NewFakeClock() - ctx, stop := context.WithCancel(t.Context()) - t.Cleanup(stop) + synctest.Test(t, func(t *testing.T) { + ch := make(chan FileEvent) - eventBatchCh := BatchDebounceEvents(ctx, clock, ch) - for i := range 100 { - path := "/a" - if i%2 == 0 { - path = "/b" - } + eventBatchCh := BatchDebounceEvents(t.Context(), ch) + for i := range 100 { + path := "/a" + if i%2 == 0 { + path = "/b" + } - ch <- FileEvent(path) - } - // we sent 100 events + the debouncer - err := clock.BlockUntilContext(ctx, 101) - assert.NilError(t, err) - clock.Advance(QuietPeriod) - select { - case batch := <-eventBatchCh: + ch <- FileEvent(path) + } + time.Sleep(QuietPeriod) + synctest.Wait() + batch := <-eventBatchCh slices.Sort(batch) assert.Equal(t, len(batch), 2) assert.Equal(t, batch[0], FileEvent("/a")) assert.Equal(t, batch[1], FileEvent("/b")) - case <-time.After(50 * time.Millisecond): - t.Fatal("timed out waiting for events") - } - err = clock.BlockUntilContext(ctx, 1) - assert.NilError(t, err) - clock.Advance(QuietPeriod) - // there should only be a single batch - select { - case batch := <-eventBatchCh: - t.Fatalf("unexpected events: %v", batch) - case <-time.After(50 * time.Millisecond): - // channel is empty - } + time.Sleep(QuietPeriod) + synctest.Wait() + + // there should only be a single batch + select { + case batch := <-eventBatchCh: + t.Fatalf("unexpected events: %v", batch) + default: + // channel is empty + } + }) } From 8bda31207ba0c2fa65166382a21e0af46a6e6276 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Sat, 5 Sep 2026 11:18:04 +0200 Subject: [PATCH 2/2] golangci-lint: set e2e build-tag to prevent false "unused" positives Signed-off-by: Sebastiaan van Stijn --- .golangci.yml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/.golangci.yml b/.golangci.yml index bdfefedc929..d7f6b48b4e0 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -1,6 +1,8 @@ version: "2" run: concurrency: 2 + build-tags: + - e2e # Avoid "unused" linter issues for code only used in e2e. linters: default: none disable: