Skip to content

Commit 0b1aa12

Browse files
deansheatherdeansheather
committed
PR comments
1 parent c7f8fcc commit 0b1aa12

File tree

8 files changed

+99
-282
lines changed

8 files changed

+99
-282
lines changed

‎coderd/agentapi/apps_test.go

Lines changed: 11 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -2,12 +2,11 @@ package agentapi_test
22

33
import (
44
"context"
5-
"sync/atomic"
65
"testing"
76

8-
"github.com/golang/mock/gomock"
97
"github.com/google/uuid"
108
"github.com/stretchr/testify/require"
9+
"go.uber.org/mock/gomock"
1110

1211
"cdr.dev/slog/sloggers/slogtest"
1312

@@ -56,20 +55,20 @@ func TestBatchUpdateAppHealths(t *testing.T) {
5655
Health: database.WorkspaceAppHealthUnhealthy,
5756
}).Return(nil)
5857

59-
var publishCalled int64
58+
publishCalled := false
6059
api := &agentapi.AppsAPI{
6160
AgentFn: func(context.Context) (database.WorkspaceAgent, error) {
6261
return agent, nil
6362
},
6463
Database: dbM,
6564
Log: slogtest.Make(t, nil),
6665
PublishWorkspaceUpdateFn: func(ctx context.Context, wa *database.WorkspaceAgent) error {
67-
atomic.AddInt64(&publishCalled, 1)
66+
publishCalled = true
6867
return nil
6968
},
7069
}
7170

72-
// Set both to healthy, only one should be updated in the DB.
71+
// Set one to healthy, set another to unhealthy.
7372
resp, err := api.BatchUpdateAppHealths(context.Background(), &agentproto.BatchUpdateAppHealthRequest{
7473
Updates: []*agentproto.BatchUpdateAppHealthRequest_HealthUpdate{
7574
{
@@ -85,7 +84,7 @@ func TestBatchUpdateAppHealths(t *testing.T) {
8584
require.NoError(t, err)
8685
require.Equal(t, &agentproto.BatchUpdateAppHealthResponse{}, resp)
8786

88-
require.EqualValues(t, 1, atomic.LoadInt64(&publishCalled))
87+
require.True(t, publishCalled)
8988
})
9089

9190
t.Run("Unchanged", func(t *testing.T) {
@@ -94,15 +93,15 @@ func TestBatchUpdateAppHealths(t *testing.T) {
9493
dbM := dbmock.NewMockStore(gomock.NewController(t))
9594
dbM.EXPECT().GetWorkspaceAppsByAgentID(gomock.Any(), agent.ID).Return([]database.WorkspaceApp{app1, app2}, nil)
9695

97-
var publishCalled int64
96+
publishCalled := false
9897
api := &agentapi.AppsAPI{
9998
AgentFn: func(context.Context) (database.WorkspaceAgent, error) {
10099
return agent, nil
101100
},
102101
Database: dbM,
103102
Log: slogtest.Make(t, nil),
104103
PublishWorkspaceUpdateFn: func(ctx context.Context, wa *database.WorkspaceAgent) error {
105-
atomic.AddInt64(&publishCalled, 1)
104+
publishCalled = true
106105
return nil
107106
},
108107
}
@@ -124,7 +123,7 @@ func TestBatchUpdateAppHealths(t *testing.T) {
124123
require.NoError(t, err)
125124
require.Equal(t, &agentproto.BatchUpdateAppHealthResponse{}, resp)
126125

127-
require.EqualValues(t, 0, atomic.LoadInt64(&publishCalled))
126+
require.False(t, publishCalled)
128127
})
129128

130129
t.Run("Empty", func(t *testing.T) {
@@ -133,15 +132,15 @@ func TestBatchUpdateAppHealths(t *testing.T) {
133132
// No DB queries are made if there are no updates to process.
134133
dbM := dbmock.NewMockStore(gomock.NewController(t))
135134

136-
var publishCalled int64
135+
publishCalled := false
137136
api := &agentapi.AppsAPI{
138137
AgentFn: func(context.Context) (database.WorkspaceAgent, error) {
139138
return agent, nil
140139
},
141140
Database: dbM,
142141
Log: slogtest.Make(t, nil),
143142
PublishWorkspaceUpdateFn: func(ctx context.Context, wa *database.WorkspaceAgent) error {
144-
atomic.AddInt64(&publishCalled, 1)
143+
publishCalled = true
145144
return nil
146145
},
147146
}
@@ -153,7 +152,7 @@ func TestBatchUpdateAppHealths(t *testing.T) {
153152
require.NoError(t, err)
154153
require.Equal(t, &agentproto.BatchUpdateAppHealthResponse{}, resp)
155154

156-
require.EqualValues(t, 0, atomic.LoadInt64(&publishCalled))
155+
require.False(t, publishCalled)
157156
})
158157

159158
t.Run("AppNoHealthcheck", func(t *testing.T) {

‎coderd/agentapi/lifecycle_test.go

Lines changed: 15 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -8,9 +8,9 @@ import (
88
"testing"
99
"time"
1010

11-
"github.com/golang/mock/gomock"
1211
"github.com/google/uuid"
1312
"github.com/stretchr/testify/require"
13+
"go.uber.org/mock/gomock"
1414
"google.golang.org/protobuf/types/known/timestamppb"
1515

1616
"cdr.dev/slog/sloggers/slogtest"
@@ -64,7 +64,7 @@ func TestUpdateLifecycle(t *testing.T) {
6464
ReadyAt: sql.NullTime{Valid: false},
6565
}).Return(nil)
6666

67-
var publishCalled int64
67+
publishCalled := false
6868
api := &agentapi.LifecycleAPI{
6969
AgentFn: func(ctx context.Context) (database.WorkspaceAgent, error) {
7070
return agentCreated, nil
@@ -75,7 +75,7 @@ func TestUpdateLifecycle(t *testing.T) {
7575
Database: dbM,
7676
Log: slogtest.Make(t, nil),
7777
PublishWorkspaceUpdateFn: func(ctx context.Context, agent *database.WorkspaceAgent) error {
78-
atomic.AddInt64(&publishCalled, 1)
78+
publishCalled = true
7979
return nil
8080
},
8181
}
@@ -85,7 +85,7 @@ func TestUpdateLifecycle(t *testing.T) {
8585
})
8686
require.NoError(t, err)
8787
require.Equal(t, lifecycle, resp)
88-
require.Equal(t, int64(1), atomic.LoadInt64(&publishCalled))
88+
require.True(t, publishCalled)
8989
})
9090

9191
t.Run("OKReadying", func(t *testing.T) {
@@ -151,7 +151,7 @@ func TestUpdateLifecycle(t *testing.T) {
151151
},
152152
}).Return(nil)
153153

154-
var publishCalled int64
154+
publishCalled := false
155155
api := &agentapi.LifecycleAPI{
156156
AgentFn: func(ctx context.Context) (database.WorkspaceAgent, error) {
157157
return agentCreated, nil
@@ -162,7 +162,7 @@ func TestUpdateLifecycle(t *testing.T) {
162162
Database: dbM,
163163
Log: slogtest.Make(t, nil),
164164
PublishWorkspaceUpdateFn: func(ctx context.Context, agent *database.WorkspaceAgent) error {
165-
atomic.AddInt64(&publishCalled, 1)
165+
publishCalled = true
166166
return nil
167167
},
168168
}
@@ -172,7 +172,7 @@ func TestUpdateLifecycle(t *testing.T) {
172172
})
173173
require.NoError(t, err)
174174
require.Equal(t, lifecycle, resp)
175-
require.Equal(t, int64(1), atomic.LoadInt64(&publishCalled))
175+
require.True(t, publishCalled)
176176
})
177177

178178
t.Run("NoTimeSpecified", func(t *testing.T) {
@@ -263,19 +263,20 @@ func TestUpdateLifecycle(t *testing.T) {
263263
}
264264
for i, state := range states {
265265
t.Log("state", state)
266-
now := now.Add(time.Hour * time.Duration(i))
266+
// Use a time after the last state change to ensure ordering.
267+
stateNow := now.Add(time.Hour * time.Duration(i))
267268
lifecycle := &agentproto.Lifecycle{
268269
State: state,
269-
ChangedAt: timestamppb.New(now),
270+
ChangedAt: timestamppb.New(stateNow),
270271
}
271272

272273
expectedStartedAt := agent.StartedAt
273274
expectedReadyAt := agent.ReadyAt
274275
if state == agentproto.Lifecycle_STARTING {
275-
expectedStartedAt = sql.NullTime{Valid: true, Time: now}
276+
expectedStartedAt = sql.NullTime{Valid: true, Time: stateNow}
276277
}
277278
if state == agentproto.Lifecycle_READY || state == agentproto.Lifecycle_START_ERROR {
278-
expectedReadyAt = sql.NullTime{Valid: true, Time: now}
279+
expectedReadyAt = sql.NullTime{Valid: true, Time: stateNow}
279280
}
280281

281282
dbM.EXPECT().UpdateWorkspaceAgentLifecycleStateByID(gomock.Any(), database.UpdateWorkspaceAgentLifecycleStateByIDParams{
@@ -308,7 +309,7 @@ func TestUpdateLifecycle(t *testing.T) {
308309

309310
dbM := dbmock.NewMockStore(gomock.NewController(t))
310311

311-
var publishCalled int64
312+
publishCalled := false
312313
api := &agentapi.LifecycleAPI{
313314
AgentFn: func(ctx context.Context) (database.WorkspaceAgent, error) {
314315
return agentCreated, nil
@@ -319,7 +320,7 @@ func TestUpdateLifecycle(t *testing.T) {
319320
Database: dbM,
320321
Log: slogtest.Make(t, nil),
321322
PublishWorkspaceUpdateFn: func(ctx context.Context, agent *database.WorkspaceAgent) error {
322-
atomic.AddInt64(&publishCalled, 1)
323+
publishCalled = true
323324
return nil
324325
},
325326
}
@@ -330,7 +331,7 @@ func TestUpdateLifecycle(t *testing.T) {
330331
require.Error(t, err)
331332
require.ErrorContains(t, err, "unknown lifecycle state")
332333
require.Nil(t, resp)
333-
require.Equal(t, int64(0), atomic.LoadInt64(&publishCalled))
334+
require.False(t, publishCalled)
334335
})
335336
}
336337

0 commit comments

Comments
 (0)