Skip to content

chore: use dbfake for ssh tests rather than provisionerd #10812

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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Nov 21, 2023

Conversation

spikecurtis
Copy link
Contributor

Refactors SSH tests to skip provisionerd and instead use dbfake to insert workspaces and builds. This should make tests faster and more reliable.

dbfake.WorkspaceBuild is refactored to use a "builder" pattern with "fluent" options, as the number of options and variants was starting to get out of hand.

Copy link
Contributor Author

spikecurtis commented Nov 21, 2023

Comment on lines +70 to +81
type BuildBuilder struct {
t testing.TB
db database.Store
ps pubsub.Pubsub
ws database.Workspace
seed database.WorkspaceBuild
resources []*sdkproto.Resource
}

func WorkspaceBuildBuilder(t testing.TB, db database.Store, ws database.Workspace) BuildBuilder {
return BuildBuilder{t: t, db: db, ws: ws}
}
Copy link
Member

Choose a reason for hiding this comment

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

Naming suggestion

BuildBuilder sounds confusing, and may suggest that it creates a generic build that includes workspace build.

Maybe NewWorkspaceBuildBuilder(<required params>) and keep WorkspaceBuildBuilder as struct? It could be also a separate package dbbuilders if dbfake starts growing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd eventually like to make all of dbfake based on the builder pattern, and drop Builder and New from the public function names, but in the interim, we can go with the more-wordy NewXxxxxxBuilder() as the convention.

seed.ID = uuid.New()
seed.JobID = jobID
seed.WorkspaceID = ws.ID
b.seed.ID = uuid.New()
Copy link
Member

Choose a reason for hiding this comment

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

Is seed required or optional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's optional. Much of the code that this replaces just passes a zero value for the seed, which is equivalent to not calling Seed()

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 change!

workspace, agentToken := dbfake.WorkspaceWithAgent(t, store, database.Workspace{
OrganizationID: first.OrganizationID,
OwnerID: user.ID,
})
Copy link
Member

Choose a reason for hiding this comment

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

Unless I'm making the wrong assumption for this change/code, maybe setupWorkspaceForAgent could return a struct instead, with store/ps so we could avoid needing this ceremony when we require those. Just a thought.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could. I decided not to since there are only two places that require it right now.

@mtojek mtojek self-requested a review November 21, 2023 11:43
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.

👍

@spikecurtis spikecurtis merged commit 5d5b5aa into main Nov 21, 2023
@spikecurtis spikecurtis deleted the spike/ssh-tests-dbfake branch November 21, 2023 12:22
@github-actions github-actions bot locked and limited conversation to collaborators Nov 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants