chore: Add test time measurements #3411
Conversation
| @@ -1,6 +1,6 @@ | |||
| module github.com/Snowflake-Labs/terraform-provider-snowflake | |||
|
|
|||
| go 1.22.10 | |||
| go 1.24.0 | |||
There was a problem hiding this comment.
We're upgrading to 1.23 in another PR.
There was a problem hiding this comment.
Yeah, I know; this should be reversed to the one used in dev. I had to bump to see if t.Context() would be helpful.
pkg/sdk/accounts_test.go
Outdated
| @@ -10,8 +10,64 @@ import ( | |||
| "github.com/stretchr/testify/require" | |||
| ) | |||
|
|
|||
| func TestATest(t *testing.T) { | |||
There was a problem hiding this comment.
How about moving to pkg/internal/measurement/
There was a problem hiding this comment.
I'll either move or remove it completely. It was used only for testing purposes.
pkg/sdk/setup_test.go
Outdated
|
|
||
| var testMeasurements = measurement.NewTestMeasurements() | ||
|
|
||
| func measureTest(t *testing.T) { |
There was a problem hiding this comment.
Shouldn't it belong to measurement package?
There was a problem hiding this comment.
No, because you want to have separate lists for unit, integration, and acceptance tests. Separate method ensures the right default container is used (the body is in measurement package anyway, so most of the part is extracted).
| } | ||
| } | ||
| } | ||
| t.Cleanup(func() { m.Duration = time.Now().Sub(start) }) |
There was a problem hiding this comment.
Pls add a method in TestMeasurement for this.
| if value, ok := currentMeasurement.SubMeasurements[part]; ok { | ||
| currentMeasurement = value | ||
| } else { | ||
| m.TestName = strings.Join(testNameParts[index+1:], "/") |
There was a problem hiding this comment.
This is not clear, why we're omitting the previous parts and joining the next ones?
There was a problem hiding this comment.
It's only for visualization purposes, instead of seeing
Test
-> Test/Sub-Test
you'll see
Test
-> Sub-Test
| start := time.Now() | ||
| m := NewTestMeasurement(t) | ||
| testNameParts := strings.Split(m.TestName, "/") | ||
| if len(testNameParts) == 1 { |
There was a problem hiding this comment.
Nit: IMO this whole block could be extracted and simplified as a recursion. Also, why we're not doing a map check with panic in else?
There was a problem hiding this comment.
It could be done with recursion, but this also works. I'll see next why I didn't check for panic (or value presence in the map).
| return m | ||
| } | ||
|
|
||
| type TestMeasurements struct { |
There was a problem hiding this comment.
I'm wondering if we really need this. Can we just pass []TestMeasurement to the functions?
There was a problem hiding this comment.
Maybe it could be like that, I'll take a look next time.
|
Integration tests failure for 54be08ad23a2f9f1ca3549c88d4b5958ca0f7b69 |
Changes
Tests
/sign which shouldn't be used in any test name (I checked that none of our test is using it, so we should be fine).Next steps
measureTestfunction at the start of one of them (we can agree in which cases we would like to skip measuring sub-tests, e.g., in the case of unit tests, which are really fast).References
Alternatives considered: