From 96de7d3d111fe6aa919c8a23ab34eae83a354369 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Fri, 3 Mar 2023 10:41:55 -0600 Subject: [PATCH 1/4] test: Check created_at for prepareData to ensure user order --- cli/root_test.go | 32 ++++++++++++++++++++++++++------ 1 file changed, 26 insertions(+), 6 deletions(-) diff --git a/cli/root_test.go b/cli/root_test.go index 525b305e1bf2c..56c3589408e30 100644 --- a/cli/root_test.go +++ b/cli/root_test.go @@ -13,6 +13,7 @@ import ( "runtime" "strings" "testing" + "time" "github.com/spf13/cobra" "github.com/stretchr/testify/assert" @@ -197,13 +198,32 @@ func prepareTestData(t *testing.T) (*codersdk.Client, map[string]string) { IncludeProvisionerDaemon: true, }) firstUser := coderdtest.CreateFirstUser(t, rootClient) - secondUser, err := rootClient.CreateUser(ctx, codersdk.CreateUserRequest{ - Email: "testuser2@coder.com", - Username: "testuser2", - Password: coderdtest.FirstUserParams.Password, - OrganizationID: firstUser.OrganizationID, - }) + + firstUserData, err := rootClient.User(ctx, firstUser.UserID.String()) require.NoError(t, err) + + var secondUser codersdk.User + for { + secondUser, err = rootClient.CreateUser(ctx, codersdk.CreateUserRequest{ + Email: "testuser2@coder.com", + Username: "testuser2", + Password: coderdtest.FirstUserParams.Password, + OrganizationID: firstUser.OrganizationID, + }) + require.NoError(t, err) + + // If the second user has the same timestamp as the first user, delete it and try again. + // This is because user order is determined by creation time, and we want to ensure + // second user is listed after first user. + // Round to microsecond since that is the precision of the database. + if secondUser.CreatedAt.Round(time.Microsecond).Equal(firstUserData.CreatedAt.Round(time.Microsecond)) { + err = rootClient.DeleteUser(ctx, secondUser.ID) + require.NoError(t, err) + continue + } + break + } + version := coderdtest.CreateTemplateVersion(t, rootClient, firstUser.OrganizationID, nil) version = coderdtest.AwaitTemplateVersionJob(t, rootClient, version.ID) template := coderdtest.CreateTemplate(t, rootClient, firstUser.OrganizationID, version.ID, func(req *codersdk.CreateTemplateRequest) { From e1dd559f93848e647ebdbd35605fcc83d08b1f46 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Fri, 3 Mar 2023 10:52:42 -0600 Subject: [PATCH 2/4] test: Consistent user ordering in dbfake --- cli/root_test.go | 32 ++++----------------- coderd/database/dbfake/databasefake.go | 15 ++++++++++ coderd/database/dbfake/databasefake_test.go | 25 ++++++++++++++++ 3 files changed, 46 insertions(+), 26 deletions(-) diff --git a/cli/root_test.go b/cli/root_test.go index 56c3589408e30..525b305e1bf2c 100644 --- a/cli/root_test.go +++ b/cli/root_test.go @@ -13,7 +13,6 @@ import ( "runtime" "strings" "testing" - "time" "github.com/spf13/cobra" "github.com/stretchr/testify/assert" @@ -198,32 +197,13 @@ func prepareTestData(t *testing.T) (*codersdk.Client, map[string]string) { IncludeProvisionerDaemon: true, }) firstUser := coderdtest.CreateFirstUser(t, rootClient) - - firstUserData, err := rootClient.User(ctx, firstUser.UserID.String()) + secondUser, err := rootClient.CreateUser(ctx, codersdk.CreateUserRequest{ + Email: "testuser2@coder.com", + Username: "testuser2", + Password: coderdtest.FirstUserParams.Password, + OrganizationID: firstUser.OrganizationID, + }) require.NoError(t, err) - - var secondUser codersdk.User - for { - secondUser, err = rootClient.CreateUser(ctx, codersdk.CreateUserRequest{ - Email: "testuser2@coder.com", - Username: "testuser2", - Password: coderdtest.FirstUserParams.Password, - OrganizationID: firstUser.OrganizationID, - }) - require.NoError(t, err) - - // If the second user has the same timestamp as the first user, delete it and try again. - // This is because user order is determined by creation time, and we want to ensure - // second user is listed after first user. - // Round to microsecond since that is the precision of the database. - if secondUser.CreatedAt.Round(time.Microsecond).Equal(firstUserData.CreatedAt.Round(time.Microsecond)) { - err = rootClient.DeleteUser(ctx, secondUser.ID) - require.NoError(t, err) - continue - } - break - } - version := coderdtest.CreateTemplateVersion(t, rootClient, firstUser.OrganizationID, nil) version = coderdtest.AwaitTemplateVersionJob(t, rootClient, version.ID) template := coderdtest.CreateTemplate(t, rootClient, firstUser.OrganizationID, version.ID, func(req *codersdk.CreateTemplateRequest) { diff --git a/coderd/database/dbfake/databasefake.go b/coderd/database/dbfake/databasefake.go index 5590f498e3283..567705c1ec42c 100644 --- a/coderd/database/dbfake/databasefake.go +++ b/coderd/database/dbfake/databasefake.go @@ -2922,6 +2922,21 @@ func (q *fakeQuerier) InsertUser(_ context.Context, arg database.InsertUserParam return database.User{}, err } + // There is a common bug when using dbfake that 2 inserted users have the + // same created_at time. This causes user order to not be deterministic, + // which breaks some unit tests. + // To fix this, we make sure that the created_at time is always greater + // than the last user's created_at time. + allUsers, _ := q.GetUsers(context.Background(), database.GetUsersParams{}) + if len(allUsers) > 0 { + lastUser := allUsers[len(allUsers)-1] + if arg.CreatedAt.Before(lastUser.CreatedAt) || + arg.CreatedAt.Equal(lastUser.CreatedAt) { + // 1 ms is a good enough buffer. + arg.CreatedAt = lastUser.CreatedAt.Add(time.Millisecond) + } + } + q.mutex.Lock() defer q.mutex.Unlock() diff --git a/coderd/database/dbfake/databasefake_test.go b/coderd/database/dbfake/databasefake_test.go index 923b404cf768d..579295731e006 100644 --- a/coderd/database/dbfake/databasefake_test.go +++ b/coderd/database/dbfake/databasefake_test.go @@ -8,6 +8,10 @@ import ( "testing" "time" + "github.com/stretchr/testify/require" + + "github.com/coder/coder/coderd/database/dbgen" + "github.com/stretchr/testify/assert" "github.com/coder/coder/coderd/database" @@ -105,6 +109,27 @@ func TestExactMethods(t *testing.T) { } } +// TestUserOrder ensures that the fake database returns users in the order they +// were created. +func TestUserOrder(t *testing.T) { + db := dbfake.New() + now := database.Now() + count := 10 + for i := 0; i < count; i++ { + dbgen.User(t, db, database.User{ + Username: fmt.Sprintf("user%d", i), + CreatedAt: now, + }) + } + + users, err := db.GetUsers(context.Background(), database.GetUsersParams{}) + require.NoError(t, err) + require.Lenf(t, users, count, "expected %d users", count) + for i, user := range users { + require.Equal(t, fmt.Sprintf("user%d", i), user.Username) + } +} + func methods(rt reflect.Type) map[string]bool { methods := make(map[string]bool) for i := 0; i < rt.NumMethod(); i++ { From 7972c23fd543ddfc7d716d2bcbace8d14301cc0c Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Fri, 3 Mar 2023 10:53:16 -0600 Subject: [PATCH 3/4] import order --- coderd/database/dbfake/databasefake_test.go | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/coderd/database/dbfake/databasefake_test.go b/coderd/database/dbfake/databasefake_test.go index 579295731e006..e6f7a7cb1acd2 100644 --- a/coderd/database/dbfake/databasefake_test.go +++ b/coderd/database/dbfake/databasefake_test.go @@ -8,15 +8,12 @@ import ( "testing" "time" - "github.com/stretchr/testify/require" - - "github.com/coder/coder/coderd/database/dbgen" - "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" "github.com/coder/coder/coderd/database" - "github.com/coder/coder/coderd/database/dbfake" + "github.com/coder/coder/coderd/database/dbgen" ) // test that transactions don't deadlock, and that we don't see intermediate state. From 0c2921efb26f5d1c30d0af447f5baf6e8f33290b Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Fri, 3 Mar 2023 11:02:11 -0600 Subject: [PATCH 4/4] Linting --- coderd/database/dbfake/databasefake_test.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/coderd/database/dbfake/databasefake_test.go b/coderd/database/dbfake/databasefake_test.go index e6f7a7cb1acd2..daf1757b0a3fa 100644 --- a/coderd/database/dbfake/databasefake_test.go +++ b/coderd/database/dbfake/databasefake_test.go @@ -109,6 +109,8 @@ func TestExactMethods(t *testing.T) { // TestUserOrder ensures that the fake database returns users in the order they // were created. func TestUserOrder(t *testing.T) { + t.Parallel() + db := dbfake.New() now := database.Now() count := 10