- Notifications
You must be signed in to change notification settings - Fork928
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.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Talked with@Emyrk and I'm gonna take a stab at speeding this test up, he had some ideas here. |
I looked at the test data in CI and interestingly this test averages very fast, despite the comments in the code saying otherwise. That being said, I still think if we are timing out at 50 seconds something on the CI box is running out of resources, and moving this to the database will still reduce the test startup burden for this test. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This is probably a good amount faster 👍
coderd/users_test.go Outdated
| for i := range allUsers[:limit] { | ||
| require.Equalf(t, page.Users[i].Username, allUsers[i].Username, "first page, limit=%d", limit) | ||
| } |
There was a problem hiding this comment.
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 toassert feels like it might be spammy.
Can we just make a test helper func like:
funconlyUsernames[U codersdk.User| database.User](users []U) []string {varout []stringfor_,u:=rangeusers {switchu:= (any(u)).(type) {case codersdk.User:out=append(out,u.Username)case database.User:out=append(out,u.Username)}}returnout}
Then we can bring back the old requires.
require.Equalf(t,onlyUsernames(page.Users),onlyUsernames(allUsers[:limit]),"first page, limit=%d",limit)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 toany in the function 😄
It just gives type safety on the call arguments.
This is definitely subjective, but I think >100ms is on the slower side for our tests. Is it something to be concerned about? Idk, probably not. |
Uh oh!
There was an error while loading.Please reload this page.
After speaking with@Emyrk I moved the user creation of these tests to use the database directly to speed things up and do less http requests in order to stay under the 50 second timeout.
Closes#10978