Skip to content

fix: use database for user creation to prevent flake #10992

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

Merged
merged 5 commits into from
Dec 4, 2023
Merged

fix: use database for user creation to prevent flake #10992

Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b862d50
fix: prevent flake by increasing context deadline
f0ssel Dec 1, 2023
859b6b6
use db over API to speed up user creation
f0ssel Dec 1, 2023
37a6c10
remove comments
f0ssel Dec 1, 2023
39cd0f7
remove comment
f0ssel Dec 1, 2023
d4d2317
use generics
f0ssel Dec 4, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
68 changes: 38 additions & 30 deletions coderd/users_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import (
"github.com/coder/coder/v2/coderd/coderdtest"
"github.com/coder/coder/v2/coderd/database"
"github.com/coder/coder/v2/coderd/database/dbauthz"
"github.com/coder/coder/v2/coderd/database/dbgen"
"github.com/coder/coder/v2/coderd/database/dbtime"
"github.com/coder/coder/v2/coderd/rbac"
"github.com/coder/coder/v2/coderd/util/slice"
Expand Down Expand Up @@ -1699,7 +1700,7 @@ func TestSuspendedPagination(t *testing.T) {
// them using different page sizes.
func TestPaginatedUsers(t *testing.T) {
t.Parallel()
client := coderdtest.New(t, nil)
client, db := coderdtest.NewWithDatabase(t, nil)
coderdtest.CreateFirstUser(t, client)

// This test takes longer than a long time.
Expand All @@ -1708,15 +1709,17 @@ func TestPaginatedUsers(t *testing.T) {

me, err := client.User(ctx, codersdk.Me)
require.NoError(t, err)
orgID := me.OrganizationIDs[0]

// When 50 users exist
total := 50
allUsers := make([]codersdk.User, total+1) // +1 forme
allUsers[0] = me
specialUsers := make([]codersdk.User, total/2)
allUsers := make([]database.User, total+1)
allUsers[0] = database.User{
Email: me.Email,
Username: me.Username,
}
specialUsers := make([]database.User, total/2)

eg, egCtx := errgroup.WithContext(ctx)
eg, _ := errgroup.WithContext(ctx)
// Create users
for i := 0; i < total; i++ {
i := i
Expand All @@ -1730,21 +1733,14 @@ func TestPaginatedUsers(t *testing.T) {
if i%3 == 0 {
username = strings.ToUpper(username)
}
// One side effect of having to use the api vs the db calls directly, is you cannot
// mock time. Ideally I could pass in mocked times and space these users out.
//
// But this also serves as a good test. Postgres has microsecond precision on its timestamps.
// If 2 users share the same created_at, that could cause an issue if you are strictly paginating via
// timestamps. The pagination goes by timestamps and uuids.
newUser, err := client.CreateUser(egCtx, codersdk.CreateUserRequest{
Email: email,
Username: username,
Password: "MySecurePassword!",
OrganizationID: orgID,

// We used to use the API to ceate users, but that is slow.
// Instead, we create them directly in the database now
// to prevent timeout flakes.
newUser := dbgen.User(t, db, database.User{
Email: email,
Username: username,
})
if err != nil {
return err
}
allUsers[i+1] = newUser
if i%2 == 0 {
specialUsers[i/2] = newUser
Expand All @@ -1757,8 +1753,8 @@ func TestPaginatedUsers(t *testing.T) {
require.NoError(t, err, "create users failed")

// Sorting the users will sort by username.
sortUsers(allUsers)
sortUsers(specialUsers)
sortDatabaseUsers(allUsers)
sortDatabaseUsers(specialUsers)

gmailSearch := func(request codersdk.UsersRequest) codersdk.UsersRequest {
request.Search = "gmail"
Expand All @@ -1772,7 +1768,7 @@ func TestPaginatedUsers(t *testing.T) {
tests := []struct {
name string
limit int
allUsers []codersdk.User
allUsers []database.User
opt func(request codersdk.UsersRequest) codersdk.UsersRequest
}{
{name: "all users", limit: 10, allUsers: allUsers},
Expand Down Expand Up @@ -1800,7 +1796,7 @@ func TestPaginatedUsers(t *testing.T) {
// Assert pagination will page through the list of all users using the given
// limit for each page. The 'allUsers' is the expected full list to compare
// against.
func assertPagination(ctx context.Context, t *testing.T, client *codersdk.Client, limit int, allUsers []codersdk.User,
func assertPagination(ctx context.Context, t *testing.T, client *codersdk.Client, limit int, allUsers []database.User,
opt func(request codersdk.UsersRequest) codersdk.UsersRequest,
) {
var count int
Expand All @@ -1817,9 +1813,11 @@ func assertPagination(ctx context.Context, t *testing.T, client *codersdk.Client
},
}))
require.NoError(t, err, "first page")
require.Equalf(t, page.Users, allUsers[:limit], "first page, limit=%d", limit)
count += len(page.Users)
for i := range allUsers[:limit] {
require.Equalf(t, page.Users[i].Username, allUsers[i].Username, "first page, limit=%d", limit)
}
Copy link
Member

Choose a reason for hiding this comment

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

I think the original comparisons are better since the new comparisons fail at the first instance, and do not show the lists and the list lengths in the failure output. Switching to assert feels like it might be spammy.

Can we just make a test helper func like:

func onlyUsernames[U codersdk.User | database.User](users []U) []string {
	var out []string
	for _, u := range users {
		switch u := (any(u)).(type) {
		case codersdk.User:
			out = append(out, u.Username)
		case database.User:
			out = append(out, u.Username)
		}
	}
	return out
}

Then we can bring back the old requires.

require.Equalf(t, onlyUsernames(page.Users), onlyUsernames(allUsers[:limit]), "first page, limit=%d", limit)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good to me, good idea. I have yet to really use generics, this will be fun.

Copy link
Member

Choose a reason for hiding this comment

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

It's a bit of a hack since I cast to any in the function 😄

It just gives type safety on the call arguments.


count += len(page.Users)
for {
if len(page.Users) == 0 {
break
Expand All @@ -1846,14 +1844,16 @@ func assertPagination(ctx context.Context, t *testing.T, client *codersdk.Client
}))
require.NoError(t, err, "next offset page")

var expected []codersdk.User
var expected []database.User
if count+limit > len(allUsers) {
expected = allUsers[count:]
} else {
expected = allUsers[count : count+limit]
}
require.Equalf(t, page.Users, expected, "next users, after=%s, limit=%d", afterCursor, limit)
require.Equalf(t, offsetPage.Users, expected, "offset users, offset=%d, limit=%d", count, limit)
for i := range expected {
require.Equalf(t, page.Users[i].Username, expected[i].Username, "next users, after=%s, limit=%d", afterCursor, limit)
require.Equalf(t, offsetPage.Users[i].Username, expected[i].Username, "offset users, offset=%d, limit=%d", count, limit)
}

// Also check the before
prevPage, err := client.Users(ctx, opt(codersdk.UsersRequest{
Expand All @@ -1863,7 +1863,9 @@ func assertPagination(ctx context.Context, t *testing.T, client *codersdk.Client
},
}))
require.NoError(t, err, "prev page")
require.Equal(t, allUsers[count-limit:count], prevPage.Users, "prev users")
for i := range prevPage.Users {
require.Equalf(t, strings.ToLower(allUsers[count-limit : count][i].Username), strings.ToLower(prevPage.Users[i].Username), "prev users, offset=%d, limit=%d", count-limit, limit)
}
count += len(page.Users)
}
}
Expand All @@ -1875,6 +1877,12 @@ func sortUsers(users []codersdk.User) {
})
}

func sortDatabaseUsers(users []database.User) {
slices.SortFunc(users, func(a, b database.User) int {
return slice.Ascending(strings.ToLower(a.Username), strings.ToLower(b.Username))
})
}

func BenchmarkUsersMe(b *testing.B) {
client := coderdtest.New(b, nil)
_ = coderdtest.CreateFirstUser(b, client)
Expand Down