Skip to content

refactor(agent): add agenttest.New helper function #9812

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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 11 commits into from
Sep 26, 2023
Merged

refactor(agent): add agenttest.New helper function #9812

merged 11 commits into from
Sep 26, 2023

Conversation

johnstcn
Copy link
Member

@johnstcn johnstcn commented Sep 21, 2023 •
edited
Loading

Following #9789 I noticed we stand up a test agent in a number of places. This seems ripe for a test helper.

Before:

agentClient := agentsdk.New(client.URL)
agentClient.SetSessionToken(authToken)
agentCloser := agent.New(agent.Options{
	Client: agentClient,
	Logger: slogtest.Make(t, nil).Named("agent"),
	Foo: "bar",
})
defer func() {
	_ = agentCloser.Close()
}()
resources := coderdtest.AwaitWorkspaceAgents(t, client, workspace.ID, agentName)

After:

_ = agenttest.New(t, client.URL, authToken, func(o *agent.Options{
	o.Foo = "bar"
})
resources := coderdtest.AwaitWorkspaceAgents(t, client, workspace.ID, agentName)

Rationale:

  • Makes sure agent gets closed on test cleanup
  • Makes sure you don't forget to set session token
  • Sets the agent and client logger automatically
  • Auto-complete gives you a nudge to call .Wait().

I haven't moved all existing use of agent.New() in *_test.go as there are some edge cases that it doesn't seem to make sense to try capturing.

@johnstcn johnstcn self-assigned this Sep 21, 2023
@johnstcn johnstcn changed the title [WIP] refactor(agent): add agenttest.New helper function refactor(agent): add agenttest.New helper function Sep 21, 2023
@johnstcn johnstcn marked this pull request as ready for review September 21, 2023 15:30
mafredri
mafredri approved these changes Sep 21, 2023
View reviewed changes
Copy link
Member

@mafredri mafredri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, love me some functional opts!

Had a few comments and suggestions, but otherwise I like the new helper. 👍🏻

kylecarbs
kylecarbs reviewed Sep 21, 2023
View reviewed changes
spikecurtis
spikecurtis reviewed Sep 22, 2023
View reviewed changes
spikecurtis
spikecurtis reviewed Sep 22, 2023
View reviewed changes
mtojek
mtojek reviewed Sep 22, 2023
View reviewed changes
Copy link
Member

@mtojek mtojek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 for noticing the need of refactoring. I see that you received a lot of comments, so I will not leave yet another evaluation. Just a few words:

Based on the part that you want to deduplicate and refactor, I would start with a basic function wrapper or function to create the agent instance, but would leave AwaitWorkspaceAgents as is. I have a feeling that an artificial agent exposing Agent() and Client() is more than we currently need.

@johnstcn
Copy link
Member Author

johnstcn commented Sep 26, 2023 •
edited
Loading

Based on the above feedback, I made the following changes:

  • Now just returns the agent directly with the close on cleanup.
  • The client can be passed in the config if required.
  • .Wait() is gone, just use coderdtest.AwaitWorkspaceAgents
  • Set a logger on the client by default

@mafredri
Copy link
Member

SGTM!

One addition that could still be made is agenttest.Await, mainly since it's shorter and part of the same imported package. It could just call out to coderdtest.AwaitWorkspaceAgents directly.

@johnstcn
Copy link
Member Author

SGTM!

One addition that could still be made is agenttest.Await, mainly since it's shorter and part of the same imported package. It could just call out to coderdtest.AwaitWorkspaceAgents directly.

All of the stuff in coderdtest calls out to the coder API, and I think it makes sense to keep that distinction. I don't think it makes sense to add that indirection for the sake of a few characters.

mtojek
mtojek approved these changes Sep 26, 2023
View reviewed changes
Copy link
Member

@mtojek mtojek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@johnstcn johnstcn merged commit 93ef696 into main Sep 26, 2023
@johnstcn johnstcn deleted the cj/agenttest branch September 26, 2023 11:05
@github-actions github-actions bot locked and limited conversation to collaborators Sep 26, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@mafredri mafredri mafredri approved these changes

@kylecarbs kylecarbs kylecarbs left review comments

@deansheather deansheather deansheather left review comments

@mtojek mtojek mtojek approved these changes

@spikecurtis spikecurtis Awaiting requested review from spikecurtis

Assignees

@johnstcn johnstcn

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@johnstcn @mafredri @spikecurtis @kylecarbs @deansheather @mtojek