Skip to content

Commit 3093efd

Browse files
ethanndicksonethanndickson
committed
test: fix TestCache_DeploymentStats flake
1 parent f867a9d commit 3093efd

File tree

3 files changed

+66
-67
lines changed

3 files changed

+66
-67
lines changed

‎coderd/coderd.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -459,7 +459,7 @@ func New(options *Options) *API {
459459
metricsCache := metricscache.New(
460460
options.Database,
461461
options.Logger.Named("metrics_cache"),
462-
options.Clock,
462+
quartz.NewReal(),
463463
metricscache.Intervals{
464464
TemplateBuildTimes: options.MetricsCacheRefreshInterval,
465465
DeploymentStats: options.AgentStatsRefreshInterval,

‎coderd/metricscache/metricscache.go

Lines changed: 20 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,6 @@ package metricscache
33
import (
44
"context"
55
"database/sql"
6-
"sync"
76
"sync/atomic"
87
"time"
98

@@ -34,8 +33,8 @@ type Cache struct {
3433
templateAverageBuildTime atomic.Pointer[map[uuid.UUID]database.GetTemplateAverageBuildTimeRow]
3534
deploymentStatsResponse atomic.Pointer[codersdk.DeploymentStats]
3635

37-
done chan struct{}
38-
cancel func()
36+
cancel func()
37+
tickers []quartz.Waiter
3938

4039
// usage is a experiment flag to enable new workspace usage tracking behavior and will be
4140
// removed when the experiment is complete.
@@ -61,25 +60,13 @@ func New(db database.Store, log slog.Logger, clock quartz.Clock, intervals Inter
6160
database: db,
6261
intervals: intervals,
6362
log: log,
64-
done: make(chan struct{}),
6563
cancel: cancel,
6664
usage: usage,
6765
}
68-
go func() {
69-
var wg sync.WaitGroup
70-
defer close(c.done)
71-
wg.Add(1)
72-
go func() {
73-
defer wg.Done()
74-
c.run(ctx, "template build times", intervals.TemplateBuildTimes, c.refreshTemplateBuildTimes)
75-
}()
76-
wg.Add(1)
77-
go func() {
78-
defer wg.Done()
79-
c.run(ctx, "deployment stats", intervals.DeploymentStats, c.refreshDeploymentStats)
80-
}()
81-
wg.Wait()
82-
}()
66+
67+
go c.run(ctx, "template build times", intervals.TemplateBuildTimes, c.refreshTemplateBuildTimes)
68+
go c.run(ctx, "deployment stats", intervals.DeploymentStats, c.refreshDeploymentStats)
69+
8370
return c
8471
}
8572

@@ -181,40 +168,39 @@ func (c *Cache) refreshDeploymentStats(ctx context.Context) error {
181168

182169
func (c *Cache) run(ctx context.Context, name string, interval time.Duration, refresh func(context.Context) error) {
183170
logger := c.log.With(slog.F("name", name), slog.F("interval", interval))
184-
ticker := time.NewTicker(interval)
185-
defer ticker.Stop()
186171

187-
for {
172+
tickerFunc := func() error {
173+
start := c.clock.Now()
188174
for r := retry.New(time.Millisecond*100, time.Minute); r.Wait(ctx); {
189-
start := time.Now()
190175
err := refresh(ctx)
191176
if err != nil {
192177
if ctx.Err() != nil {
193-
return
178+
return nil
194179
}
195180
if xerrors.Is(err, sql.ErrNoRows) {
196181
break
197182
}
198183
logger.Error(ctx, "refresh metrics failed", slog.Error(err))
199184
continue
200185
}
201-
logger.Debug(ctx, "metrics refreshed", slog.F("took", time.Since(start)))
186+
logger.Debug(ctx, "metrics refreshed", slog.F("took", c.clock.Since(start)))
202187
break
203188
}
204-
205-
select {
206-
case <-ticker.C:
207-
case <-c.done:
208-
return
209-
case <-ctx.Done():
210-
return
211-
}
189+
return nil
212190
}
191+
192+
// Call once immediately before starting ticker
193+
_ = tickerFunc()
194+
195+
tkr := c.clock.TickerFunc(ctx, interval, tickerFunc, "metricscache", name)
196+
c.tickers = append(c.tickers, tkr)
213197
}
214198

215199
func (c *Cache) Close() error {
216200
c.cancel()
217-
<-c.done
201+
for _, tkr := range c.tickers {
202+
_ = tkr.Wait()
203+
}
218204
return nil
219205
}
220206

‎coderd/metricscache/metricscache_test.go

Lines changed: 45 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -205,14 +205,20 @@ func TestCache_BuildTime(t *testing.T) {
205205
t.Run(tt.name, func(t *testing.T) {
206206
t.Parallel()
207207

208+
ctx := testutil.Context(t, testutil.WaitShort)
209+
208210
var (
209-
log = testutil.Logger(t)
210-
clock = quartz.NewMock(t)
211-
cache, db = newMetricsCache(t, log, clock, metricscache.Intervals{
212-
TemplateBuildTimes: testutil.IntervalFast,
213-
}, false)
211+
log = testutil.Logger(t)
212+
clock = quartz.NewMock(t)
214213
)
215214

215+
trapTickerFunc := clock.Trap().TickerFunc("metricscache")
216+
217+
defer trapTickerFunc.Close()
218+
cache, db := newMetricsCache(t, log, clock, metricscache.Intervals{
219+
TemplateBuildTimes: testutil.IntervalFast,
220+
}, false)
221+
216222
clock.Set(someDay)
217223

218224
org := dbgen.Organization(t, db, database.Organization{})
@@ -257,17 +263,20 @@ func TestCache_BuildTime(t *testing.T) {
257263
})
258264
}
259265

266+
// Wait for both ticker functions to be created (template build times and deployment stats)
267+
trapTickerFunc.MustWait(ctx).MustRelease(ctx)
268+
trapTickerFunc.MustWait(ctx).MustRelease(ctx)
269+
270+
_, wait := clock.AdvanceNext()
271+
wait.MustWait(ctx)
272+
260273
if tt.want.loads {
261274
wantTransition := codersdk.WorkspaceTransition(tt.args.transition)
262-
require.Eventuallyf(t, func() bool {
263-
stats := cache.TemplateBuildTimeStats(template.ID)
264-
ts := stats[wantTransition]
265-
return ts.P50 != nil && *ts.P50 == tt.want.buildTimeMs
266-
}, testutil.WaitLong, testutil.IntervalMedium,
267-
"P50 never reached expected value for %v", wantTransition,
268-
)
269-
270275
gotStats := cache.TemplateBuildTimeStats(template.ID)
276+
ts := gotStats[wantTransition]
277+
require.NotNil(t, ts.P50, "P50 should be set for %v", wantTransition)
278+
require.Equal(t, tt.want.buildTimeMs, *ts.P50, "P50 should match expected value for %v", wantTransition)
279+
271280
for transition, ts := range gotStats {
272281
if transition == wantTransition {
273282
// Checked above
@@ -276,14 +285,8 @@ func TestCache_BuildTime(t *testing.T) {
276285
require.Empty(t, ts, "%v", transition)
277286
}
278287
} else {
279-
var stats codersdk.TemplateBuildTimeStats
280-
require.Never(t, func() bool {
281-
stats = cache.TemplateBuildTimeStats(template.ID)
282-
requireBuildTimeStatsEmpty(t, stats)
283-
return t.Failed()
284-
}, testutil.WaitShort/2, testutil.IntervalMedium,
285-
"BuildTimeStats populated", stats,
286-
)
288+
stats := cache.TemplateBuildTimeStats(template.ID)
289+
requireBuildTimeStatsEmpty(t, stats)
287290
}
288291
})
289292
}
@@ -292,14 +295,21 @@ func TestCache_BuildTime(t *testing.T) {
292295
func TestCache_DeploymentStats(t *testing.T) {
293296
t.Parallel()
294297

298+
ctx := testutil.Context(t, testutil.WaitShort)
299+
295300
var (
296-
log = testutil.Logger(t)
297-
clock = quartz.NewMock(t)
298-
cache, db = newMetricsCache(t, log, clock, metricscache.Intervals{
299-
DeploymentStats: testutil.IntervalFast,
300-
}, false)
301+
log = testutil.Logger(t)
302+
clock = quartz.NewMock(t)
301303
)
302304

305+
tickerTrap := clock.Trap().TickerFunc("metricscache")
306+
defer tickerTrap.Close()
307+
308+
cache, db := newMetricsCache(t, log, clock, metricscache.Intervals{
309+
TemplateBuildTimes: testutil.IntervalFast,
310+
DeploymentStats: testutil.IntervalFast,
311+
}, false)
312+
303313
err := db.InsertWorkspaceAgentStats(context.Background(), database.InsertWorkspaceAgentStatsParams{
304314
ID: []uuid.UUID{uuid.New()},
305315
CreatedAt: []time.Time{clock.Now()},
@@ -323,11 +333,14 @@ func TestCache_DeploymentStats(t *testing.T) {
323333
})
324334
require.NoError(t, err)
325335

326-
var stat codersdk.DeploymentStats
327-
require.Eventually(t, func() bool {
328-
var ok bool
329-
stat, ok = cache.DeploymentStats()
330-
return ok
331-
}, testutil.WaitLong, testutil.IntervalMedium)
336+
// Wait for both ticker functions to be created (template build times and deployment stats)
337+
tickerTrap.MustWait(ctx).MustRelease(ctx)
338+
tickerTrap.MustWait(ctx).MustRelease(ctx)
339+
340+
_, wait := clock.AdvanceNext()
341+
wait.MustWait(ctx)
342+
343+
stat, ok := cache.DeploymentStats()
344+
require.True(t, ok, "cache should be populated after refresh")
332345
require.Equal(t, int64(1), stat.SessionCount.VSCode)
333346
}

0 commit comments

Comments
 (0)