From b862d506918e6672e8159aefe9ef41058fcdbe93 Mon Sep 17 00:00:00 2001 From: Garrett Delfosse Date: Fri, 1 Dec 2023 17:41:49 +0000 Subject: [PATCH 1/5] fix: prevent flake by increasing context deadline --- coderd/users_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/coderd/users_test.go b/coderd/users_test.go index 2755a7491a28e..523a2697dcd84 100644 --- a/coderd/users_test.go +++ b/coderd/users_test.go @@ -1789,7 +1789,7 @@ func TestPaginatedUsers(t *testing.T) { t.Parallel() // This test takes longer than a long time. - ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong*2) + ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong*3) defer cancel() assertPagination(ctx, t, client, tt.limit, tt.allUsers, tt.opt) From 859b6b676adc5a8287cfcc7915ad30bbd68c5d5a Mon Sep 17 00:00:00 2001 From: Garrett Delfosse Date: Fri, 1 Dec 2023 19:25:02 +0000 Subject: [PATCH 2/5] use db over API to speed up user creation --- coderd/users_test.go | 73 ++++++++++++++++++++++++++------------------ 1 file changed, 43 insertions(+), 30 deletions(-) diff --git a/coderd/users_test.go b/coderd/users_test.go index 523a2697dcd84..74b18b825100f 100644 --- a/coderd/users_test.go +++ b/coderd/users_test.go @@ -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" @@ -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. @@ -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 @@ -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 @@ -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" @@ -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}, @@ -1789,7 +1785,7 @@ func TestPaginatedUsers(t *testing.T) { t.Parallel() // This test takes longer than a long time. - ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong*3) + ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong*2) defer cancel() assertPagination(ctx, t, client, tt.limit, tt.allUsers, tt.opt) @@ -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 @@ -1817,7 +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) + for i := range allUsers[:limit] { + require.Equalf(t, page.Users[i].Username, allUsers[i].Username, "first page, limit=%d", limit) + } + // require.NoError(t, err, "first page") + // require.Equalf(t, page.Users, allUsers[:limit], "first page, limit=%d", limit) count += len(page.Users) for { @@ -1846,14 +1846,18 @@ 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) + } + // 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) // Also check the before prevPage, err := client.Users(ctx, opt(codersdk.UsersRequest{ @@ -1863,7 +1867,10 @@ 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) + } + // require.Equal(t, allUsers[count-limit:count], prevPage.Users, "prev users") count += len(page.Users) } } @@ -1875,6 +1882,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) From 37a6c10961751cf129d107eff96b3958279b9136 Mon Sep 17 00:00:00 2001 From: Garrett Delfosse Date: Fri, 1 Dec 2023 19:44:59 +0000 Subject: [PATCH 3/5] remove comments --- coderd/users_test.go | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/coderd/users_test.go b/coderd/users_test.go index 74b18b825100f..1f50d5c635a82 100644 --- a/coderd/users_test.go +++ b/coderd/users_test.go @@ -1816,10 +1816,8 @@ func assertPagination(ctx context.Context, t *testing.T, client *codersdk.Client for i := range allUsers[:limit] { require.Equalf(t, page.Users[i].Username, allUsers[i].Username, "first page, limit=%d", limit) } - // require.NoError(t, err, "first page") - // require.Equalf(t, page.Users, allUsers[:limit], "first page, limit=%d", limit) - count += len(page.Users) + count += len(page.Users) for { if len(page.Users) == 0 { break @@ -1856,8 +1854,6 @@ func assertPagination(ctx context.Context, t *testing.T, client *codersdk.Client 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) } - // 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) // Also check the before prevPage, err := client.Users(ctx, opt(codersdk.UsersRequest{ From 39cd0f7a2fea1af5fcee0696b21bf48d3fc00e52 Mon Sep 17 00:00:00 2001 From: Garrett Delfosse Date: Fri, 1 Dec 2023 20:00:10 +0000 Subject: [PATCH 4/5] remove comment --- coderd/users_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/coderd/users_test.go b/coderd/users_test.go index 1f50d5c635a82..8ccc7d80f250c 100644 --- a/coderd/users_test.go +++ b/coderd/users_test.go @@ -1866,7 +1866,6 @@ func assertPagination(ctx context.Context, t *testing.T, client *codersdk.Client 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) } - // require.Equal(t, allUsers[count-limit:count], prevPage.Users, "prev users") count += len(page.Users) } } From d4d2317976eb1a7706b8609ad3f84fbe5601a122 Mon Sep 17 00:00:00 2001 From: Garrett Delfosse Date: Mon, 4 Dec 2023 14:03:06 +0000 Subject: [PATCH 5/5] use generics --- coderd/users_test.go | 29 ++++++++++++++++++----------- 1 file changed, 18 insertions(+), 11 deletions(-) diff --git a/coderd/users_test.go b/coderd/users_test.go index 8ccc7d80f250c..8cbd69308e61f 100644 --- a/coderd/users_test.go +++ b/coderd/users_test.go @@ -1813,11 +1813,9 @@ func assertPagination(ctx context.Context, t *testing.T, client *codersdk.Client }, })) require.NoError(t, err, "first page") - for i := range allUsers[:limit] { - require.Equalf(t, page.Users[i].Username, allUsers[i].Username, "first page, limit=%d", limit) - } - + require.Equalf(t, onlyUsernames(page.Users), onlyUsernames(allUsers[:limit]), "first page, limit=%d", limit) count += len(page.Users) + for { if len(page.Users) == 0 { break @@ -1850,10 +1848,8 @@ func assertPagination(ctx context.Context, t *testing.T, client *codersdk.Client } else { expected = allUsers[count : 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) - } + require.Equalf(t, onlyUsernames(page.Users), onlyUsernames(expected), "next users, after=%s, limit=%d", afterCursor, limit) + require.Equalf(t, onlyUsernames(offsetPage.Users), onlyUsernames(expected), "offset users, offset=%d, limit=%d", count, limit) // Also check the before prevPage, err := client.Users(ctx, opt(codersdk.UsersRequest{ @@ -1863,9 +1859,7 @@ func assertPagination(ctx context.Context, t *testing.T, client *codersdk.Client }, })) require.NoError(t, err, "prev page") - 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) - } + require.Equal(t, onlyUsernames(allUsers[count-limit:count]), onlyUsernames(prevPage.Users), "prev users") count += len(page.Users) } } @@ -1883,6 +1877,19 @@ func sortDatabaseUsers(users []database.User) { }) } +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 +} + func BenchmarkUsersMe(b *testing.B) { client := coderdtest.New(b, nil) _ = coderdtest.CreateFirstUser(b, client)