Skip to content

Commit 760dc8b

Browse files
DanielleMaywoodDanielleMaywood
authored
fix(agent/agentcontainers): fix TestDevcontainerDiscovery/AutoStart flake (#19179)
Fixes coder/internal#864
1 parent 8b66a5a commit 760dc8b

File tree

1 file changed

+107
-147
lines changed

1 file changed

+107
-147
lines changed

‎agent/agentcontainers/api_test.go

Lines changed: 107 additions & 147 deletions
Original file line numberDiff line numberDiff line change
@@ -71,6 +71,7 @@ func (f *fakeContainerCLI) ExecAs(ctx context.Context, name, user string, args .
7171
// fakeDevcontainerCLI implements the agentcontainers.DevcontainerCLI
7272
// interface for testing.
7373
type fakeDevcontainerCLI struct {
74+
up func(workspaceFolder, configPath string) (string, error)
7475
upID string
7576
upErr error
7677
upErrC chan func() error // If set, send to return err, close to return upErr.
@@ -79,9 +80,14 @@ type fakeDevcontainerCLI struct {
7980
readConfig agentcontainers.DevcontainerConfig
8081
readConfigErr error
8182
readConfigErrC chan func(envs []string) error
83+
84+
configMap map[string]agentcontainers.DevcontainerConfig // By config path
8285
}
8386

84-
func (f *fakeDevcontainerCLI) Up(ctx context.Context, _, _ string, _ ...agentcontainers.DevcontainerCLIUpOptions) (string, error) {
87+
func (f *fakeDevcontainerCLI) Up(ctx context.Context, workspaceFolder, configPath string, _ ...agentcontainers.DevcontainerCLIUpOptions) (string, error) {
88+
if f.up != nil {
89+
return f.up(workspaceFolder, configPath)
90+
}
8591
if f.upErrC != nil {
8692
select {
8793
case <-ctx.Done():
@@ -109,7 +115,12 @@ func (f *fakeDevcontainerCLI) Exec(ctx context.Context, _, _ string, cmd string,
109115
return f.execErr
110116
}
111117

112-
func (f *fakeDevcontainerCLI) ReadConfig(ctx context.Context, _, _ string, envs []string, _ ...agentcontainers.DevcontainerCLIReadConfigOptions) (agentcontainers.DevcontainerConfig, error) {
118+
func (f *fakeDevcontainerCLI) ReadConfig(ctx context.Context, _, configPath string, envs []string, _ ...agentcontainers.DevcontainerCLIReadConfigOptions) (agentcontainers.DevcontainerConfig, error) {
119+
if f.configMap != nil {
120+
if v, found := f.configMap[configPath]; found {
121+
return v, f.readConfigErr
122+
}
123+
}
113124
if f.readConfigErrC != nil {
114125
select {
115126
case <-ctx.Done():
@@ -3576,193 +3587,112 @@ func TestDevcontainerDiscovery(t *testing.T) {
35763587
name string
35773588
agentDir string
35783589
fs map[string]string
3590+
configMap map[string]agentcontainers.DevcontainerConfig
35793591
expectDevcontainerCount int
3580-
setupMocks func(mDCCLI *acmock.MockDevcontainerCLI)
3592+
expectUpCalledCount int
35813593
}{
35823594
{
35833595
name: "SingleEnabled",
35843596
agentDir: "/home/coder",
35853597
expectDevcontainerCount: 1,
3598+
expectUpCalledCount: 1,
35863599
fs: map[string]string{
35873600
"/home/coder/.git/HEAD": "",
35883601
"/home/coder/.devcontainer/devcontainer.json": "",
35893602
},
3590-
setupMocks: func(mDCCLI *acmock.MockDevcontainerCLI) {
3591-
gomock.InOrder(
3592-
// Given: This dev container has auto start enabled.
3593-
mDCCLI.EXPECT().ReadConfig(gomock.Any(),
3594-
"/home/coder",
3595-
"/home/coder/.devcontainer/devcontainer.json",
3596-
[]string{},
3597-
).Return(agentcontainers.DevcontainerConfig{
3598-
Configuration: agentcontainers.DevcontainerConfiguration{
3599-
Customizations: agentcontainers.DevcontainerCustomizations{
3600-
Coder: agentcontainers.CoderCustomization{
3601-
AutoStart: true,
3602-
},
3603+
configMap: map[string]agentcontainers.DevcontainerConfig{
3604+
"/home/coder/.devcontainer/devcontainer.json": {
3605+
Configuration: agentcontainers.DevcontainerConfiguration{
3606+
Customizations: agentcontainers.DevcontainerCustomizations{
3607+
Coder: agentcontainers.CoderCustomization{
3608+
AutoStart: true,
36033609
},
36043610
},
3605-
}, nil),
3606-
3607-
// Then: We expect it to be started.
3608-
mDCCLI.EXPECT().Up(gomock.Any(),
3609-
"/home/coder",
3610-
"/home/coder/.devcontainer/devcontainer.json",
3611-
gomock.Any(),
3612-
).Return("", nil),
3613-
)
3611+
},
3612+
},
36143613
},
36153614
},
36163615
{
36173616
name: "SingleDisabled",
36183617
agentDir: "/home/coder",
36193618
expectDevcontainerCount: 1,
3619+
expectUpCalledCount: 0,
36203620
fs: map[string]string{
36213621
"/home/coder/.git/HEAD": "",
36223622
"/home/coder/.devcontainer/devcontainer.json": "",
36233623
},
3624-
setupMocks: func(mDCCLI *acmock.MockDevcontainerCLI) {
3625-
gomock.InOrder(
3626-
// Given: This dev container has auto start disabled.
3627-
mDCCLI.EXPECT().ReadConfig(gomock.Any(),
3628-
"/home/coder",
3629-
"/home/coder/.devcontainer/devcontainer.json",
3630-
[]string{},
3631-
).Return(agentcontainers.DevcontainerConfig{
3632-
Configuration: agentcontainers.DevcontainerConfiguration{
3633-
Customizations: agentcontainers.DevcontainerCustomizations{
3634-
Coder: agentcontainers.CoderCustomization{
3635-
AutoStart: false,
3636-
},
3624+
configMap: map[string]agentcontainers.DevcontainerConfig{
3625+
"/home/coder/.devcontainer/devcontainer.json": {
3626+
Configuration: agentcontainers.DevcontainerConfiguration{
3627+
Customizations: agentcontainers.DevcontainerCustomizations{
3628+
Coder: agentcontainers.CoderCustomization{
3629+
AutoStart: false,
36373630
},
36383631
},
3639-
}, nil),
3640-
3641-
// Then: We expect it to _not_ be started.
3642-
mDCCLI.EXPECT().Up(gomock.Any(),
3643-
"/home/coder",
3644-
"/home/coder/.devcontainer/devcontainer.json",
3645-
gomock.Any(),
3646-
).Return("", nil).Times(0),
3647-
)
3632+
},
3633+
},
36483634
},
36493635
},
36503636
{
36513637
name: "OneEnabledOneDisabled",
36523638
agentDir: "/home/coder",
36533639
expectDevcontainerCount: 2,
3640+
expectUpCalledCount: 1,
36543641
fs: map[string]string{
36553642
"/home/coder/.git/HEAD": "",
36563643
"/home/coder/.devcontainer/devcontainer.json": "",
36573644
"/home/coder/project/.devcontainer.json": "",
36583645
},
3659-
setupMocks: func(mDCCLI *acmock.MockDevcontainerCLI) {
3660-
gomock.InOrder(
3661-
// Given: This dev container has auto start enabled.
3662-
mDCCLI.EXPECT().ReadConfig(gomock.Any(),
3663-
"/home/coder",
3664-
"/home/coder/.devcontainer/devcontainer.json",
3665-
[]string{},
3666-
).Return(agentcontainers.DevcontainerConfig{
3667-
Configuration: agentcontainers.DevcontainerConfiguration{
3668-
Customizations: agentcontainers.DevcontainerCustomizations{
3669-
Coder: agentcontainers.CoderCustomization{
3670-
AutoStart: true,
3671-
},
3646+
configMap: map[string]agentcontainers.DevcontainerConfig{
3647+
"/home/coder/.devcontainer/devcontainer.json": {
3648+
Configuration: agentcontainers.DevcontainerConfiguration{
3649+
Customizations: agentcontainers.DevcontainerCustomizations{
3650+
Coder: agentcontainers.CoderCustomization{
3651+
AutoStart: true,
36723652
},
36733653
},
3674-
}, nil),
3675-
3676-
// Then: We expect it to be started.
3677-
mDCCLI.EXPECT().Up(gomock.Any(),
3678-
"/home/coder",
3679-
"/home/coder/.devcontainer/devcontainer.json",
3680-
gomock.Any(),
3681-
).Return("", nil),
3682-
)
3683-
3684-
gomock.InOrder(
3685-
// Given: This dev container has auto start disabled.
3686-
mDCCLI.EXPECT().ReadConfig(gomock.Any(),
3687-
"/home/coder/project",
3688-
"/home/coder/project/.devcontainer.json",
3689-
[]string{},
3690-
).Return(agentcontainers.DevcontainerConfig{
3691-
Configuration: agentcontainers.DevcontainerConfiguration{
3692-
Customizations: agentcontainers.DevcontainerCustomizations{
3693-
Coder: agentcontainers.CoderCustomization{
3694-
AutoStart: false,
3695-
},
3654+
},
3655+
},
3656+
"/home/coder/project/.devcontainer.json": {
3657+
Configuration: agentcontainers.DevcontainerConfiguration{
3658+
Customizations: agentcontainers.DevcontainerCustomizations{
3659+
Coder: agentcontainers.CoderCustomization{
3660+
AutoStart: false,
36963661
},
36973662
},
3698-
}, nil),
3699-
3700-
// Then: We expect it to _not_ be started.
3701-
mDCCLI.EXPECT().Up(gomock.Any(),
3702-
"/home/coder/project",
3703-
"/home/coder/project/.devcontainer.json",
3704-
gomock.Any(),
3705-
).Return("", nil).Times(0),
3706-
)
3663+
},
3664+
},
37073665
},
37083666
},
37093667
{
37103668
name: "MultipleEnabled",
37113669
agentDir: "/home/coder",
37123670
expectDevcontainerCount: 2,
3671+
expectUpCalledCount: 2,
37133672
fs: map[string]string{
37143673
"/home/coder/.git/HEAD": "",
37153674
"/home/coder/.devcontainer/devcontainer.json": "",
37163675
"/home/coder/project/.devcontainer.json": "",
37173676
},
3718-
setupMocks: func(mDCCLI *acmock.MockDevcontainerCLI) {
3719-
gomock.InOrder(
3720-
// Given: This dev container has auto start enabled.
3721-
mDCCLI.EXPECT().ReadConfig(gomock.Any(),
3722-
"/home/coder",
3723-
"/home/coder/.devcontainer/devcontainer.json",
3724-
[]string{},
3725-
).Return(agentcontainers.DevcontainerConfig{
3726-
Configuration: agentcontainers.DevcontainerConfiguration{
3727-
Customizations: agentcontainers.DevcontainerCustomizations{
3728-
Coder: agentcontainers.CoderCustomization{
3729-
AutoStart: true,
3730-
},
3677+
configMap: map[string]agentcontainers.DevcontainerConfig{
3678+
"/home/coder/.devcontainer/devcontainer.json": {
3679+
Configuration: agentcontainers.DevcontainerConfiguration{
3680+
Customizations: agentcontainers.DevcontainerCustomizations{
3681+
Coder: agentcontainers.CoderCustomization{
3682+
AutoStart: true,
37313683
},
37323684
},
3733-
}, nil),
3734-
3735-
// Then: We expect it to be started.
3736-
mDCCLI.EXPECT().Up(gomock.Any(),
3737-
"/home/coder",
3738-
"/home/coder/.devcontainer/devcontainer.json",
3739-
gomock.Any(),
3740-
).Return("", nil),
3741-
)
3742-
3743-
gomock.InOrder(
3744-
// Given: This dev container has auto start enabled.
3745-
mDCCLI.EXPECT().ReadConfig(gomock.Any(),
3746-
"/home/coder/project",
3747-
"/home/coder/project/.devcontainer.json",
3748-
[]string{},
3749-
).Return(agentcontainers.DevcontainerConfig{
3750-
Configuration: agentcontainers.DevcontainerConfiguration{
3751-
Customizations: agentcontainers.DevcontainerCustomizations{
3752-
Coder: agentcontainers.CoderCustomization{
3753-
AutoStart: true,
3754-
},
3685+
},
3686+
},
3687+
"/home/coder/project/.devcontainer.json": {
3688+
Configuration: agentcontainers.DevcontainerConfiguration{
3689+
Customizations: agentcontainers.DevcontainerCustomizations{
3690+
Coder: agentcontainers.CoderCustomization{
3691+
AutoStart: true,
37553692
},
37563693
},
3757-
}, nil),
3758-
3759-
// Then: We expect it to be started.
3760-
mDCCLI.EXPECT().Up(gomock.Any(),
3761-
"/home/coder/project",
3762-
"/home/coder/project/.devcontainer.json",
3763-
gomock.Any(),
3764-
).Return("", nil),
3765-
)
3694+
},
3695+
},
37663696
},
37673697
},
37683698
}
@@ -3775,43 +3705,73 @@ func TestDevcontainerDiscovery(t *testing.T) {
37753705
ctx = testutil.Context(t, testutil.WaitShort)
37763706
logger = testutil.Logger(t)
37773707
mClock = quartz.NewMock(t)
3778-
mDCCLI = acmock.NewMockDevcontainerCLI(gomock.NewController(t))
3708+
3709+
upCalledMu sync.Mutex
3710+
upCalledFor = map[string]bool{}
3711+
3712+
fCCLI = &fakeContainerCLI{}
3713+
fDCCLI = &fakeDevcontainerCLI{
3714+
configMap: tt.configMap,
3715+
up: func(_, configPath string) (string, error) {
3716+
upCalledMu.Lock()
3717+
upCalledFor[configPath] = true
3718+
upCalledMu.Unlock()
3719+
return "", nil
3720+
},
3721+
}
37793722

37803723
r = chi.NewRouter()
37813724
)
37823725

3783-
// Given: We setup our mocks. These mocks handle our expectations for these
3784-
// tests. If there are missing/unexpected mock calls, the test will fail.
3785-
tt.setupMocks(mDCCLI)
3786-
37873726
api := agentcontainers.NewAPI(logger,
37883727
agentcontainers.WithClock(mClock),
37893728
agentcontainers.WithWatcher(watcher.NewNoop()),
37903729
agentcontainers.WithFileSystem(initFS(t, tt.fs)),
37913730
agentcontainers.WithManifestInfo("owner", "workspace", "parent-agent", "/home/coder"),
3792-
agentcontainers.WithContainerCLI(&fakeContainerCLI{}),
3793-
agentcontainers.WithDevcontainerCLI(mDCCLI),
3731+
agentcontainers.WithContainerCLI(fCCLI),
3732+
agentcontainers.WithDevcontainerCLI(fDCCLI),
37943733
agentcontainers.WithProjectDiscovery(true),
37953734
agentcontainers.WithDiscoveryAutostart(true),
37963735
)
37973736
api.Start()
3798-
defer api.Close()
37993737
r.Mount("/", api.Routes())
38003738

3801-
// When: All expected dev containers have been found.
3739+
// Given: We allow the discover routing to progress
3740+
var got codersdk.WorkspaceAgentListContainersResponse
38023741
require.Eventuallyf(t, func() bool {
38033742
req := httptest.NewRequest(http.MethodGet, "/", nil).WithContext(ctx)
38043743
rec := httptest.NewRecorder()
38053744
r.ServeHTTP(rec, req)
38063745

3807-
got := codersdk.WorkspaceAgentListContainersResponse{}
3746+
got = codersdk.WorkspaceAgentListContainersResponse{}
38083747
err := json.NewDecoder(rec.Body).Decode(&got)
38093748
require.NoError(t, err)
38103749

3811-
return len(got.Devcontainers) >= tt.expectDevcontainerCount
3750+
upCalledMu.Lock()
3751+
upCalledCount := len(upCalledFor)
3752+
upCalledMu.Unlock()
3753+
3754+
return len(got.Devcontainers) >= tt.expectDevcontainerCount && upCalledCount >= tt.expectUpCalledCount
38123755
}, testutil.WaitShort, testutil.IntervalFast, "dev containers never found")
38133756

3814-
// Then: We expect the mock infra to not fail.
3757+
// Close the API. We expect this not to fail because we should have finished
3758+
// at this point.
3759+
err := api.Close()
3760+
require.NoError(t, err)
3761+
3762+
// Then: We expect to find the expected devcontainers
3763+
assert.Len(t, got.Devcontainers, tt.expectDevcontainerCount)
3764+
3765+
// And: We expect `up` to have been called the expected amount of times.
3766+
assert.Len(t, upCalledFor, tt.expectUpCalledCount)
3767+
3768+
// And: `up` was called on the correct containers
3769+
for configPath, config := range tt.configMap {
3770+
autoStart := config.Configuration.Customizations.Coder.AutoStart
3771+
wasUpCalled := upCalledFor[configPath]
3772+
3773+
require.Equal(t, autoStart, wasUpCalled)
3774+
}
38153775
})
38163776
}
38173777

0 commit comments

Comments
 (0)