Skip to content

Commit deda816

Browse files
watcher: don't fail the grace period test on a slow machine
`TestRefreshGracePeriod` was the most frequent CI failure, reporting a GPT.ini version one bump ahead of what it expected. The test writes a file, waits for half of the one second grace period, and asserts that no bump happened yet. Reaching that assertion involves `testutils.WaitForWrites`, which flushes every mounted file system; on a runner busy with I/O this alone can outlast the grace period. The watcher was right to bump, the test just looked too late. Measure how much of the grace period is left before each such assertion and, when the window was missed, retry the whole scenario with a longer one rather than reporting a failure that says nothing about the watcher. The assertions themselves are unchanged and still strict. The final bump is now polled instead of being waited for blindly, so a late bump no longer fails either. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent af422e7 commit deda816

3 files changed

Lines changed: 136 additions & 21 deletions

File tree

‎internal/watcher/export_test.go‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,15 @@ func WithRefreshDuration(refreshDuration time.Duration) func(o *options) error {
1010
}
1111
}
1212

13+
// WithEventProcessed reports when the watcher has processed a filesystem event.
14+
// The timestamp is captured immediately before the refresh timer is reset.
15+
func WithEventProcessed(f func(string, time.Time)) func(o *options) error {
16+
return func(o *options) error {
17+
o.eventProcessed = f
18+
return nil
19+
}
20+
}
21+
1322
// RefreshDuration returns the refresh duration used by the watcher.
1423
func (w Watcher) RefreshDuration() time.Duration {
1524
return w.refreshDuration

‎internal/watcher/watcher.go‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@ type Watcher struct {
3535
cmdErr chan error
3636

3737
refreshDuration time.Duration
38+
eventProcessed func(string, time.Time)
3839
}
3940

4041
type command struct {
@@ -46,6 +47,7 @@ type command struct {
4647
// options are the configurable functional options for the watcher.
4748
type options struct {
4849
refreshDuration time.Duration
50+
eventProcessed func(string, time.Time)
4951
}
5052
type option func(*options) error
5153

@@ -77,6 +79,7 @@ func New(ctx context.Context, initialDirs []string, opts ...option) (*Watcher, e
7779
cmdErr: cmdErr,
7880

7981
refreshDuration: args.refreshDuration,
82+
eventProcessed: args.eventProcessed,
8083
}
8184

8285
go func() {
@@ -284,7 +287,14 @@ func (w *Watcher) watch(ctx context.Context, dirs []string, initError chan<- err
284287
}
285288

286289
// We got a change, so reset the timer to the grace period.
290+
var timerResetStarted time.Time
291+
if w.eventProcessed != nil {
292+
timerResetStarted = time.Now()
293+
}
287294
refreshTimer.Reset(w.refreshDuration)
295+
if w.eventProcessed != nil {
296+
w.eventProcessed(event.Name, timerResetStarted)
297+
}
288298

289299
case err, ok := <-fsWatcher.Errors:
290300
if ok {

‎internal/watcher/watcher_test.go‎

Lines changed: 117 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import (
55
"os"
66
"path/filepath"
77
"strings"
8+
"sync"
89
"testing"
910
"time"
1011

@@ -193,49 +194,136 @@ func TestWatchDirectory(t *testing.T) {
193194
func TestRefreshGracePeriod(t *testing.T) {
194195
t.Parallel()
195196

197+
// The scenario asserts that no version bump happened *while still inside* the
198+
// grace period. Such an assertion is only meaningful if we reach it before
199+
// the grace period elapses, which a loaded machine does not guarantee:
200+
// flushing the file system alone can take a sizeable amount of time. Instead
201+
// of failing on a slow runner, retry with a longer grace period, keeping the
202+
// assertions themselves strict.
203+
const maxAttempts = 4
204+
205+
gracePeriod := time.Second
206+
for range maxAttempts {
207+
if refreshGracePeriodScenario(t, gracePeriod) {
208+
return
209+
}
210+
gracePeriod *= 2
211+
}
212+
t.Fatalf("Machine is too slow to observe the refresh grace period reliably, even with a grace period of %s", gracePeriod/2)
213+
}
214+
215+
type eventTracker struct {
216+
mu sync.Mutex
217+
counts map[string]uint64
218+
times map[string]time.Time
219+
}
220+
221+
func (tracker *eventTracker) notify(path string, processedAt time.Time) {
222+
tracker.mu.Lock()
223+
defer tracker.mu.Unlock()
224+
tracker.counts[path]++
225+
tracker.times[path] = processedAt
226+
}
227+
228+
func (tracker *eventTracker) count(path string) uint64 {
229+
tracker.mu.Lock()
230+
defer tracker.mu.Unlock()
231+
return tracker.counts[path]
232+
}
233+
234+
func (tracker *eventTracker) waitFor(t *testing.T, path string, previous uint64) time.Time {
235+
t.Helper()
236+
237+
require.Eventually(t, func() bool {
238+
return tracker.count(path) > previous
239+
}, 30*time.Second, 10*time.Millisecond, "watcher should process event for %s", path)
240+
241+
tracker.mu.Lock()
242+
defer tracker.mu.Unlock()
243+
return tracker.times[path]
244+
}
245+
246+
// refreshGracePeriodScenario checks that the watcher only bumps the GPT.ini
247+
// version once the given grace period elapsed without any change, and that any
248+
// new change pushes that deadline back.
249+
//
250+
// It reports whether the run was conclusive: when the machine takes longer than
251+
// the grace period to reach an assertion, a missing bump can’t be told apart
252+
// from a late one, and the caller should retry with a longer grace period.
253+
func refreshGracePeriodScenario(t *testing.T, gracePeriod time.Duration) (conclusive bool) {
254+
t.Helper()
255+
196256
dir := "withsubdir"
197257
temp := t.TempDir()
198258
dest := filepath.Join(temp, dir)
199259
testutils.Copy(t, filepath.Join("testdata", dir), dest)
200260

261+
events := &eventTracker{
262+
counts: make(map[string]uint64),
263+
times: make(map[string]time.Time),
264+
}
265+
201266
// Instantiate the object
202-
w, err := watcher.New(context.Background(), []string{dest}, watcher.WithRefreshDuration(time.Second))
267+
w, err := watcher.New(context.Background(), []string{dest},
268+
watcher.WithRefreshDuration(gracePeriod),
269+
watcher.WithEventProcessed(events.notify),
270+
)
203271
require.NoError(t, err, "Setup: Can't create watcher")
204272

205273
// Start it
206274
err = w.Start(mockService{})
207275
require.NoError(t, err, "Setup: Can't start watcher")
208276
defer w.Stop(mockService{})
209277

210-
// Modify first file
211-
err = os.WriteFile(filepath.Join(temp, dir, "alreadyexists"), []byte("new content"), 0600)
278+
// Modify first file: this arms the grace period timer.
279+
firstPath := filepath.Join(dest, "alreadyexists")
280+
firstEvent := events.count(firstPath)
281+
err = os.WriteFile(firstPath, []byte("new content"), 0600)
212282
require.NoError(t, err, "Setup: Can't update file")
213-
214-
testutils.WaitForWrites(t)
283+
firstProcessed := events.waitFor(t, firstPath, firstEvent)
215284

216285
// Wait for half of the grace period
217286
time.Sleep(w.RefreshDuration() / 2)
218287

219288
// GPT.ini version was not changed
220-
assertGPTVersionEquals(t, dest, 2)
289+
firstVersion, err := gptVersion(dest)
290+
require.NoError(t, err, "Can't get GPT.ini version as an integer")
291+
if time.Since(firstProcessed) >= gracePeriod {
292+
return false
293+
}
294+
assert.Equal(t, 2, firstVersion, "GPT.ini version is not equal to the expected one")
221295

222-
// Modify second file
223-
err = os.WriteFile(filepath.Join(temp, dir, "alreadyexistsDir", "alreadyexists"), []byte("new content"), 0600)
296+
// Modify second file: this must push the deadline back by a whole grace period.
297+
secondPath := filepath.Join(dest, "alreadyexistsDir", "alreadyexists")
298+
secondEvent := events.count(secondPath)
299+
err = os.WriteFile(secondPath, []byte("new content"), 0600)
224300
require.NoError(t, err, "Setup: Can't update file")
301+
secondProcessed := events.waitFor(t, secondPath, secondEvent)
302+
if secondProcessed.Sub(firstProcessed) >= gracePeriod {
303+
return false
304+
}
225305

226-
testutils.WaitForWrites(t)
227-
228-
// Wait for 3/4 of the grace period (to be sure that we waited for more than one whole grace period in total).
229-
time.Sleep(time.Duration(float64(w.RefreshDuration()) * 0.75))
306+
// Wait for half of the grace period again: we are now past a whole grace
307+
// period since the first write, but still within the one opened by the
308+
// second write.
309+
time.Sleep(w.RefreshDuration() / 2)
230310

231311
// GPT.ini version was still not changed
232-
assertGPTVersionEquals(t, dest, 2)
233-
234-
// Wait for another 1/2 of the grace period (to be sure that we waited for more than one whole grace period in total).
235-
time.Sleep(w.RefreshDuration() / 2)
312+
secondVersion, err := gptVersion(dest)
313+
require.NoError(t, err, "Can't get GPT.ini version as an integer")
314+
if time.Since(secondProcessed) >= gracePeriod || time.Since(firstProcessed) <= gracePeriod {
315+
return false
316+
}
317+
assert.Equal(t, 2, secondVersion, "GPT.ini version is not equal to the expected one")
236318

237-
// GPT.ini version was updated
319+
// GPT.ini version is updated once the grace period elapsed, and only once.
320+
require.Eventually(t, func() bool {
321+
v, err := gptVersion(dest)
322+
return err == nil && v > 2
323+
}, 30*time.Second, 10*time.Millisecond, "GPT.ini version should have been bumped after the grace period")
238324
assertGPTVersionEquals(t, dest, 3)
325+
326+
return true
239327
}
240328

241329
func TestUpdateDirs(t *testing.T) {
@@ -422,15 +510,23 @@ func assertGPTVersionEquals(t *testing.T, path string, version int) {
422510
}
423511
require.True(t, gptFileExists, "GPT.ini not created")
424512

425-
cfg, err := ini.Load(gptfile)
426-
require.NoError(t, err, "Can't load GPT.ini")
427-
428-
v, err := cfg.Section("General").Key("Version").Int()
513+
v, err := gptVersion(path)
429514
require.NoError(t, err, "Can't get GPT.ini version as an integer")
430515

431516
assert.Equal(t, version, v, "GPT.ini version is not equal to the expected one")
432517
}
433518

519+
// gptVersion returns the version set in the GPT.ini file of the given directory.
520+
// It doesn’t assert, so that it can be polled from a testify condition.
521+
func gptVersion(path string) (int, error) {
522+
cfg, err := ini.Load(filepath.Join(path, "GPT.INI"))
523+
if err != nil {
524+
return 0, err
525+
}
526+
527+
return cfg.Section("General").Key("Version").Int()
528+
}
529+
434530
func requireGPTVersionError(t *testing.T, path string) {
435531
t.Helper()
436532

0 commit comments

Comments
 (0)