Skip to content

test: Fix user pagination sort order #1143

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 2 commits into from
Apr 25, 2022
Merged

test: Fix user pagination sort order #1143

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
dcac12c
test: Fix user pagination unit test
Emyrk Apr 25, 2022
d817adb
fixup! test: Fix user pagination unit test
Emyrk Apr 25, 2022
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
18 changes: 18 additions & 0 deletions coderd/users_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"context"
"fmt"
"net/http"
"sort"
"testing"

"github.com/google/uuid"
Expand Down Expand Up @@ -597,6 +598,13 @@ func TestPaginatedUsers(t *testing.T) {
}
}

// Sorting the users will sort by (created_at, uuid). This is to handle
// the off case that created_at is identical for 2 users.
// This is a really rare case in production, but does happen in unit tests
// due to the fake database being in memory and exceptionally quick.
sortUsers(allUsers)
sortUsers(specialUsers)

assertPagination(ctx, t, client, 10, allUsers, nil)
assertPagination(ctx, t, client, 5, allUsers, nil)
assertPagination(ctx, t, client, 3, allUsers, nil)
Expand Down Expand Up @@ -679,3 +687,13 @@ func assertPagination(ctx context.Context, t *testing.T, client *codersdk.Client
count += len(page)
}
}

// sortUsers sorts by (created_at, id)
func sortUsers(users []codersdk.User) {
sort.Slice(users, func(i, j int) bool {
if users[i].CreatedAt.Equal(users[j].CreatedAt) {
return users[i].ID.String() < users[j].ID.String()
Copy link
Member

Choose a reason for hiding this comment

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

Could we always just sort by UUID?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, because created_at ensures new users are always at the end meaning if you paginate by after_user, you are guaranteed to never miss a user even if one is inserted as you paginate.

If our uuids were strictly increasing, then yes.

The only reason I sort by (created_at, uuid) is to cover this small edge case of 2 users being created with the same time. Within 1 ms for postgres, and for the fake db it's a little more nuanced. Since we marshal over the API, our user's time's are not monotonic.

But tl'dr this fixes things in that edge case. In production, users will be sorted by time. Which is a good consistent ordering.

Copy link
Member

Choose a reason for hiding this comment

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

Ahh. Thank you for the explanation!

}
return users[i].CreatedAt.Before(users[j].CreatedAt)
})
}