- Notifications
You must be signed in to change notification settings - Fork928
fix(coderd): use stable sorting for insights and improve test coverage#9250
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
b4a50c3 tobd15af7Comparebd15af7 to73367d1CompareI think this ready now, I might add some more test further down the line but I'd like to start with optimizations next. |
| // TODO(mafredri): This doesn't behave correctly right now | ||
| // and will add more usage to the app. This could be | ||
| // considered both correct and incorrect behavior. | ||
| // { // One hour of usage, but same user and same template app, only count once. | ||
| // app: users[0].workspaces[1].apps[0], | ||
| // startedAt: frozenWeekAgo, | ||
| // endedAt: frozenWeekAgo.Add(time.Hour), | ||
| // requests: 1, | ||
| // }, |
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 something I will fix later on, I wanted to keep query logic changes minimal for now.
Essentially this boils down to:
- User A has two workspaces in template X
- User A opens code-server in both workspaces
- User A closes both code-servers ater an hour
- We should only show 1h of code-server usage in template X
Currently we show two hours. Motivation for this is that a user can't effectively use multiple code-servers simultaneously. Imagine if this was 10 workspaces, the user would have to have very many skill points in parallel thinking and perhaps get a few extra keyboards and grow some extra limbs to produce 10h of usage in 1h. 😄
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.
Better test coverage is always welcome!
Uh oh!
There was an error while loading.Please reload this page.
| @@ -2480,6 +2489,8 @@ func (q *FakeQuerier) GetTemplateParameterInsights(ctx context.Context, arg data | |||
| } | |||
| } | |||
| // NOTE(mafredri): Add sorting if we decide on how to handle PostgreSQL collations. | |||
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.
out of curiosity: what is the default PostgreSQL behavior considering the "most default" configuration?
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 will depend on the systems locale. Essentially if e.g. ifLC_ALL=C orLC_ALL=en_US.UTF-8 is set when callinginitdb, it will produce different results.
Uh oh!
There was an error while loading.Please reload this page.
| @@ -0,0 +1,44 @@ | |||
| { | |||
| "report": { | |||
| "start_time": "0001-01-01T00:00:00Z", | |||
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.
should we populate this with real timestamps? just to prevent UI from breaking down accidentally
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 only for the test and to ensure the golden file isn't updated every day.
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.
Yes, I was rather thinking about the case when somebody forgets to set this field, and a unit test could potentially catch it.
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 will be caught by the other tests in this suite 👍🏻.
Withoutdbgen, builds are generated "now", so we must request "now" to check them. But "now" keeps changing and can't be used in the golden file. The other data is fixed-in-time and we can request fixed-in-time so the times get tested that way.
Uh oh!
There was an error while loading.Please reload this page.
coderd/testdata/insights/multiple_users_and_workspaces_week_second_template.json.goldenShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
| } | ||
| // Create the template version and template. | ||
| version := coderdtest.CreateTemplateVersion(t, client, firstUser.OrganizationID, &echo.Responses{ |
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.
nit: maybe we should depend ondbgen as much as possible to shorten the test execution and prevent potential flakes
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 evaluated doing that, but we didn't have any methods for generating template/workspace build parameters. Perhaps it could be a future improvement?
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.
👍
| makeRequest func([]*testTemplate) codersdk.TemplateInsightsRequest | ||
| ignoreTimes bool | ||
| } | ||
| tests := []struct { |
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.
Do you think that we can convert-to-table/remove some other insights tests?
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 we can delete at least one or two. But depending on if we rely primarily on dbgen here or not, we might want to keep some "whole tests" around.
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 deleted one testTestTemplateInsights, its test cases are now covered by the golden files and then some.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
This PR introduces a new test suite for insights and makes it easy to add new test scenarios which can then be compared against golden file changes.
A few bugfixes found by the test suite are included as well.
Fixes#9213