Skip to content

fix: add timeouts to test telemetry snapshot #17879

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
May 22, 2025
Merged

fix: add timeouts to test telemetry snapshot #17879

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5d10e11
telemetry snapshot timeouts
hugodutka May 16, 2025
cc56091
use helper functions
hugodutka May 16, 2025
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
44 changes: 31 additions & 13 deletions coderd/telemetry/telemetry_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package telemetry_test

import (
"context"
"database/sql"
"encoding/json"
"net/http"
Expand Down Expand Up @@ -115,7 +116,7 @@ func TestTelemetry(t *testing.T) {
_ = dbgen.WorkspaceAgentMemoryResourceMonitor(t, db, database.WorkspaceAgentMemoryResourceMonitor{})
_ = dbgen.WorkspaceAgentVolumeResourceMonitor(t, db, database.WorkspaceAgentVolumeResourceMonitor{})

_, snapshot := collectSnapshot(t, db, nil)
_, snapshot := collectSnapshot(ctx, t, db, nil)
require.Len(t, snapshot.ProvisionerJobs, 1)
require.Len(t, snapshot.Licenses, 1)
require.Len(t, snapshot.Templates, 1)
Expand Down Expand Up @@ -168,17 +169,19 @@ func TestTelemetry(t *testing.T) {
})
t.Run("HashedEmail", func(t *testing.T) {
t.Parallel()
ctx := testutil.Context(t, testutil.WaitMedium)
db := dbmem.New()
_ = dbgen.User(t, db, database.User{
Email: "kyle@coder.com",
})
_, snapshot := collectSnapshot(t, db, nil)
_, snapshot := collectSnapshot(ctx, t, db, nil)
require.Len(t, snapshot.Users, 1)
require.Equal(t, snapshot.Users[0].EmailHashed, "bb44bf07cf9a2db0554bba63a03d822c927deae77df101874496df5a6a3e896d@coder.com")
})
t.Run("HashedModule", func(t *testing.T) {
t.Parallel()
db, _ := dbtestutil.NewDB(t)
ctx := testutil.Context(t, testutil.WaitMedium)
pj := dbgen.ProvisionerJob(t, db, nil, database.ProvisionerJob{})
_ = dbgen.WorkspaceModule(t, db, database.WorkspaceModule{
JobID: pj.ID,
Expand All @@ -190,7 +193,7 @@ func TestTelemetry(t *testing.T) {
Source: "https://internal-url.com/some-module",
Version: "1.0.0",
})
_, snapshot := collectSnapshot(t, db, nil)
_, snapshot := collectSnapshot(ctx, t, db, nil)
require.Len(t, snapshot.WorkspaceModules, 2)
modules := snapshot.WorkspaceModules
sort.Slice(modules, func(i, j int) bool {
Expand Down Expand Up @@ -286,11 +289,11 @@ func TestTelemetry(t *testing.T) {
db, _ := dbtestutil.NewDB(t)

// 1. No org sync settings
deployment, _ := collectSnapshot(t, db, nil)
deployment, _ := collectSnapshot(ctx, t, db, nil)
require.False(t, *deployment.IDPOrgSync)

// 2. Org sync settings set in server flags
deployment, _ = collectSnapshot(t, db, func(opts telemetry.Options) telemetry.Options {
deployment, _ = collectSnapshot(ctx, t, db, func(opts telemetry.Options) telemetry.Options {
opts.DeploymentConfig = &codersdk.DeploymentValues{
OIDC: codersdk.OIDCConfig{
OrganizationField: "organizations",
Expand All @@ -312,16 +315,17 @@ func TestTelemetry(t *testing.T) {
AssignDefault: true,
})
require.NoError(t, err)
deployment, _ = collectSnapshot(t, db, nil)
deployment, _ = collectSnapshot(ctx, t, db, nil)
require.True(t, *deployment.IDPOrgSync)
})
}

// nolint:paralleltest
func TestTelemetryInstallSource(t *testing.T) {
t.Setenv("CODER_TELEMETRY_INSTALL_SOURCE", "aws_marketplace")
ctx := testutil.Context(t, testutil.WaitMedium)
db := dbmem.New()
deployment, _ := collectSnapshot(t, db, nil)
deployment, _ := collectSnapshot(ctx, t, db, nil)
require.Equal(t, "aws_marketplace", deployment.InstallSource)
}

Expand Down Expand Up @@ -436,7 +440,7 @@ func TestRecordTelemetryStatus(t *testing.T) {
}
}

func mockTelemetryServer(t *testing.T) (*url.URL, chan *telemetry.Deployment, chan *telemetry.Snapshot) {
func mockTelemetryServer(ctx context.Context, t *testing.T) (*url.URL, chan *telemetry.Deployment, chan *telemetry.Snapshot) {
t.Helper()
deployment := make(chan *telemetry.Deployment, 64)
snapshot := make(chan *telemetry.Snapshot, 64)
Expand All @@ -446,7 +450,11 @@ func mockTelemetryServer(t *testing.T) (*url.URL, chan *telemetry.Deployment, ch
dd := &telemetry.Deployment{}
err := json.NewDecoder(r.Body).Decode(dd)
require.NoError(t, err)
deployment <- dd
ok := testutil.AssertSend(ctx, t, deployment, dd)
if !ok {
w.WriteHeader(http.StatusInternalServerError)
return
}
// Ensure the header is sent only after deployment is sent
w.WriteHeader(http.StatusAccepted)
})
Expand All @@ -455,7 +463,11 @@ func mockTelemetryServer(t *testing.T) (*url.URL, chan *telemetry.Deployment, ch
ss := &telemetry.Snapshot{}
err := json.NewDecoder(r.Body).Decode(ss)
require.NoError(t, err)
snapshot <- ss
ok := testutil.AssertSend(ctx, t, snapshot, ss)
if !ok {
w.WriteHeader(http.StatusInternalServerError)
return
}
// Ensure the header is sent only after snapshot is sent
w.WriteHeader(http.StatusAccepted)
})
Expand All @@ -467,10 +479,15 @@ func mockTelemetryServer(t *testing.T) (*url.URL, chan *telemetry.Deployment, ch
return serverURL, deployment, snapshot
}

func collectSnapshot(t *testing.T, db database.Store, addOptionsFn func(opts telemetry.Options) telemetry.Options) (*telemetry.Deployment, *telemetry.Snapshot) {
func collectSnapshot(
ctx context.Context,
t *testing.T,
db database.Store,
addOptionsFn func(opts telemetry.Options) telemetry.Options,
) (*telemetry.Deployment, *telemetry.Snapshot) {
t.Helper()

serverURL, deployment, snapshot := mockTelemetryServer(t)
serverURL, deployment, snapshot := mockTelemetryServer(ctx, t)

options := telemetry.Options{
Database: db,
Expand All @@ -485,5 +502,6 @@ func collectSnapshot(t *testing.T, db database.Store, addOptionsFn func(opts tel
reporter, err := telemetry.New(options)
require.NoError(t, err)
t.Cleanup(reporter.Close)
return <-deployment, <-snapshot

return testutil.RequireReceive(ctx, t, deployment), testutil.RequireReceive(ctx, t, snapshot)
}
58 changes: 58 additions & 0 deletions testutil/chan.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,3 +55,61 @@ func RequireSend[A any](ctx context.Context, t testing.TB, c chan<- A, a A) {
// OK!
}
}

// SoftTryReceive will attempt to receive a value from the chan and return it. If
// the context expires before a value can be received, it will mark the test as
// failed but continue execution. If the channel is closed, the zero value of the
// channel type will be returned.
// The second return value indicates whether the receive was successful. In
// particular, if the channel is closed, the second return value will be true.
//
// Safety: can be called from any goroutine.
func SoftTryReceive[A any](ctx context.Context, t testing.TB, c <-chan A) (A, bool) {
t.Helper()
select {
case <-ctx.Done():
t.Error("timeout")
var a A
return a, false
case a := <-c:
return a, true
}
}

// AssertReceive will receive a value from the chan and return it. If the
// context expires or the channel is closed before a value can be received,
// it will mark the test as failed but continue execution.
// The second return value indicates whether the receive was successful.
//
// Safety: can be called from any goroutine.
func AssertReceive[A any](ctx context.Context, t testing.TB, c <-chan A) (A, bool) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: could we abstract the logic and either use t.Fatal or t.Error in either case? That seems to be the only delta between the implementations.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's also the extra return value. I suppose we could create internal helper functions which would be called with a bool param by the Require/Assert variants. Not sure if it's worth it here - what do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

Nah, :shipit:

t.Helper()
select {
case <-ctx.Done():
t.Error("timeout")
var a A
return a, false
case a, ok := <-c:
if !ok {
t.Error("channel closed")
}
return a, ok
}
}

// AssertSend will send the given value over the chan and then return. If
// the context expires before the send succeeds, it will mark the test as failed
// but continue execution.
// The second return value indicates whether the send was successful.
//
// Safety: can be called from any goroutine.
func AssertSend[A any](ctx context.Context, t testing.TB, c chan<- A, a A) bool {
t.Helper()
select {
case <-ctx.Done():
t.Error("timeout")
return false
case c <- a:
return true
}
}
Loading