fix(coderd): use stable sorting for insights and improve test coverage #9250
Conversation
b4a50c3 to
bd15af7
Compare
bd15af7 to
73367d1
Compare
|
I 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.
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. 😄
| @@ -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.
out of curiosity: what is the default PostgreSQL behavior considering the "most default" configuration?
There was a problem hiding this comment.
It will depend on the systems locale. Essentially if e.g. if LC_ALL=C or LC_ALL=en_US.UTF-8 is set when calling initdb, it will produce different results.
| @@ -0,0 +1,44 @@ | |||
| { | |||
| "report": { | |||
| "start_time": "0001-01-01T00:00:00Z", | |||
There was a problem hiding this comment.
should we populate this with real timestamps? just to prevent UI from breaking down accidentally
There was a problem hiding this comment.
This is only for the test and to ensure the golden file isn't updated every day.
There was a problem hiding this comment.
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.
It will be caught by the other tests in this suite 👍🏻.
Without dbgen, 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.
coderd/testdata/insights/multiple_users_and_workspaces_week_second_template.json.golden
Show resolved
Hide resolved
| } | ||
|
|
||
| // Create the template version and template. | ||
| version := coderdtest.CreateTemplateVersion(t, client, firstUser.OrganizationID, &echo.Responses{ |
There was a problem hiding this comment.
nit: maybe we should depend on dbgen as much as possible to shorten the test execution and prevent potential flakes
There was a problem hiding this comment.
I evaluated doing that, but we didn't have any methods for generating template/workspace build parameters. Perhaps it could be a future improvement?
| makeRequest func([]*testTemplate) codersdk.TemplateInsightsRequest | ||
| ignoreTimes bool | ||
| } | ||
| tests := []struct { |
There was a problem hiding this comment.
Do you think that we can convert-to-table/remove some other insights tests?
There was a problem hiding this comment.
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.
I deleted one test TestTemplateInsights, its test cases are now covered by the golden files and then some.
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