- Notifications
You must be signed in to change notification settings - Fork1k
refactor: convert workspacesdk.AgentConn to an interface#19392
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| ifoptions.PrometheusRegistry==nil { | ||
| options.PrometheusRegistry=prometheus.NewRegistry() | ||
| } | ||
| ifoptions.Authorizer==nil { | ||
| options.Authorizer=rbac.NewCachingAuthorizer(options.PrometheusRegistry) | ||
| ifbuildinfo.IsDev() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This re-order fixes a nil pointer access when noPrometheusRegistry andAuthorizer are passed through options.
| t.Run("PayloadTooLarge",func(t*testing.T) { | ||
| t.Parallel() | ||
| var ( | ||
| ctx=testutil.Context(t,testutil.WaitSuperLong) | ||
| logger=slogtest.Make(t,&slogtest.Options{IgnoreErrors:true}).Leveled(slog.LevelDebug) | ||
| mClock=quartz.NewMock(t) | ||
| updaterTickerTrap=mClock.Trap().TickerFunc("updaterLoop") | ||
| mCtrl=gomock.NewController(t) | ||
| mCCLI=acmock.NewMockContainerCLI(mCtrl) | ||
| client,db=coderdtest.NewWithDatabase(t,&coderdtest.Options{Logger:&logger}) | ||
| user=coderdtest.CreateFirstUser(t,client) | ||
| r=dbfake.WorkspaceBuild(t,db, database.WorkspaceTable{ | ||
| OrganizationID:user.OrganizationID, | ||
| OwnerID:user.UserID, | ||
| }).WithAgent(func(agents []*proto.Agent) []*proto.Agent { | ||
| returnagents | ||
| }).Do() | ||
| ) | ||
| // WebSocket limit is 4MiB, so we want to ensure we create _more_ than 4MiB worth of payload. | ||
| // Creating 20,000 fake containers creates a payload of roughly 7MiB. | ||
| varfakeContainers []codersdk.WorkspaceAgentContainer | ||
| forrange20_000 { | ||
| fakeContainers=append(fakeContainers, codersdk.WorkspaceAgentContainer{ | ||
| CreatedAt:time.Now(), | ||
| ID:uuid.NewString(), | ||
| FriendlyName:uuid.NewString(), | ||
| Image:"busybox:latest", | ||
| Labels:map[string]string{ | ||
| agentcontainers.DevcontainerLocalFolderLabel:"/home/coder/project", | ||
| agentcontainers.DevcontainerConfigFileLabel:"/home/coder/project/.devcontainer/devcontainer.json", | ||
| }, | ||
| Running:false, | ||
| Ports: []codersdk.WorkspaceAgentContainerPort{}, | ||
| Status:string(codersdk.WorkspaceAgentDevcontainerStatusRunning), | ||
| Volumes:map[string]string{}, | ||
| }) | ||
| } | ||
| mCCLI.EXPECT().List(gomock.Any()).Return(codersdk.WorkspaceAgentListContainersResponse{Containers:fakeContainers},nil) | ||
| mCCLI.EXPECT().DetectArchitecture(gomock.Any(),gomock.Any()).Return("<none>",nil).AnyTimes() | ||
| _=agenttest.New(t,client.URL,r.AgentToken,func(o*agent.Options) { | ||
| o.Logger=logger.Named("agent") | ||
| o.Devcontainers=true | ||
| o.DevcontainerAPIOptions= []agentcontainers.Option{ | ||
| agentcontainers.WithClock(mClock), | ||
| agentcontainers.WithContainerCLI(mCCLI), | ||
| agentcontainers.WithWatcher(watcher.NewNoop()), | ||
| } | ||
| }) | ||
| resources:=coderdtest.NewWorkspaceAgentWaiter(t,client,r.Workspace.ID).Wait() | ||
| require.Len(t,resources,1,"expected one resource") | ||
| require.Len(t,resources[0].Agents,1,"expected one agent") | ||
| agentID:=resources[0].Agents[0].ID | ||
| updaterTickerTrap.MustWait(ctx).MustRelease(ctx) | ||
| deferupdaterTickerTrap.Close() | ||
| containers,closer,err:=client.WatchWorkspaceAgentContainers(ctx,agentID) | ||
| require.NoError(t,err) | ||
| deferfunc() { | ||
| closer.Close() | ||
| }() | ||
| select { | ||
| case<-ctx.Done(): | ||
| t.Fail() | ||
| case_,ok:=<-containers: | ||
| require.False(t,ok) | ||
| } | ||
| }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This test case is now covered by the test incoderd/workspaceagents_internal_test.go.
We convert `workspacesdk.AgentConn` to an interface and generate a mockfor it. This allows writing `coderd` tests that rely on the agent's HTTPapi to not have to set up an entire tailnet networking stack.
d579e1d toea4de03CompareThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Refactor looks fine to me 👍 Thanks for de-flaking this!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The humongous new interface does not feel ideal, but I also don't have a better suggestion at the moment. One suggestion to improve the test but otherwise LGTM.
| decodeCh:=decoder.Chan() | ||
| // When: We close the `containersCh` | ||
| close(containersCh) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Should we have at least one test message sent first to confirm the base case works and then the exit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Sounds good to me
| // AgentConn represents a connection to a workspace agent. | ||
| // @typescript-ignore AgentConn | ||
| typeAgentConnstruct { | ||
| typeAgentConninterface { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Given this is an SDK are we really ready to commit to this just to fix a flake? It feels a little heavy handed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Without this or something like it it's basically impossible to test Coderd routes that need to interact with Agents without the whole baggage of a real tailnet and a real agent.
Ourtesting is straining under the weight of spinning up all this stuff to test individual API handlers.
SDKs that we develop need to be designed with mocking in mind, otherwise we can't effectively unit test code that depends on them.
@DanielleMaywood could you elaborate on the underlying cause of the flake? |
Sure thing. The underlying cause of the flake is that it is justincredibly slow. On my workspace it could take up to around 10 seconds, and that is dedicated hardware without contention. Running it in CI means results in it often exceeding the You can see in this flake that it took almost 8 seconds just for the tailnet to begin accepting TCP connections (test started at As you can see in the logs, most of the time is spent setting up the agent and the networking stack. We don't need any of this running to test for a specific behavior on The idea behind the change to make |
5e84d25 intomainUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Fixescoder/internal#907
We convert
workspacesdk.AgentConnto an interface and generate a mock for it. This allows writingcoderdtests that rely on the agent's HTTP api to not have to set up an entire tailnet networking stack.