feat(coderd/database/dbtestutil): set default database timezone to non-UTC in unit tests (#9672)

- Adds dbtestutil.WithTimezone(tz) to allow setting the timezone for a test database.
- Modifies our test database setup code to pick a consistently weird timezone for the database.
- Adds the facility randtz.Name() to pick a random timezone which is consistent across subtests (via sync.Once).
- Adds a linter rule to warn against setting the test database timezone to UTC.
This commit is contained in:
Cian Johnston 2023-09-15 09:01:32 +01:00 committed by GitHub
parent 281faf9ccd
commit 65db7a71b7
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
12 changed files with 1270 additions and 131 deletions

View File

@ -26,7 +26,7 @@ import (
// make update-golden-files
var UpdateGoldenFiles = flag.Bool("update", false, "update .golden files")
var timestampRegex = regexp.MustCompile(`(?i)\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}(.\d+)?Z`)
var timestampRegex = regexp.MustCompile(`(?i)\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}(.\d+)?(Z|[+-]\d+:\d+)`)
type CommandHelpCase struct {
Name string

View File

@ -2,7 +2,6 @@ package coderd
import (
"database/sql"
"runtime"
"testing"
"time"
@ -22,6 +21,16 @@ import (
func Test_ActivityBumpWorkspace(t *testing.T) {
t.Parallel()
// We test the below in multiple timezones specifically
// chosen to trigger timezone-related bugs.
timezones := []string{
"Asia/Kolkata", // No DST, positive fractional offset
"Canada/Newfoundland", // DST, negative fractional offset
"Europe/Paris", // DST, positive offset
"US/Arizona", // No DST, negative offset
"UTC", // Baseline
}
for _, tt := range []struct {
name string
transition database.WorkspaceTransition
@ -92,117 +101,124 @@ func Test_ActivityBumpWorkspace(t *testing.T) {
},
} {
tt := tt
t.Run(tt.name, func(t *testing.T) {
t.Parallel()
for _, tz := range timezones {
tz := tz
t.Run(tt.name+"/"+tz, func(t *testing.T) {
t.Parallel()
var (
now = dbtime.Now()
ctx = testutil.Context(t, testutil.WaitShort)
log = slogtest.Make(t, nil)
db, _ = dbtestutil.NewDB(t)
org = dbgen.Organization(t, db, database.Organization{})
user = dbgen.User(t, db, database.User{
Status: database.UserStatusActive,
})
_ = dbgen.OrganizationMember(t, db, database.OrganizationMember{
UserID: user.ID,
OrganizationID: org.ID,
})
templateVersion = dbgen.TemplateVersion(t, db, database.TemplateVersion{
OrganizationID: org.ID,
CreatedBy: user.ID,
})
template = dbgen.Template(t, db, database.Template{
OrganizationID: org.ID,
ActiveVersionID: templateVersion.ID,
CreatedBy: user.ID,
})
ws = dbgen.Workspace(t, db, database.Workspace{
OwnerID: user.ID,
OrganizationID: org.ID,
TemplateID: template.ID,
Ttl: sql.NullInt64{Valid: true, Int64: int64(tt.workspaceTTL)},
})
job = dbgen.ProvisionerJob(t, db, database.ProvisionerJob{
OrganizationID: org.ID,
CompletedAt: tt.jobCompletedAt,
})
_ = dbgen.WorkspaceResource(t, db, database.WorkspaceResource{
JobID: job.ID,
})
buildID = uuid.New()
)
var (
now = dbtime.Now()
ctx = testutil.Context(t, testutil.WaitShort)
log = slogtest.Make(t, nil)
db, _ = dbtestutil.NewDB(t, dbtestutil.WithTimezone(tz))
org = dbgen.Organization(t, db, database.Organization{})
user = dbgen.User(t, db, database.User{
Status: database.UserStatusActive,
})
_ = dbgen.OrganizationMember(t, db, database.OrganizationMember{
UserID: user.ID,
OrganizationID: org.ID,
})
templateVersion = dbgen.TemplateVersion(t, db, database.TemplateVersion{
OrganizationID: org.ID,
CreatedBy: user.ID,
})
template = dbgen.Template(t, db, database.Template{
OrganizationID: org.ID,
ActiveVersionID: templateVersion.ID,
CreatedBy: user.ID,
})
ws = dbgen.Workspace(t, db, database.Workspace{
OwnerID: user.ID,
OrganizationID: org.ID,
TemplateID: template.ID,
Ttl: sql.NullInt64{Valid: true, Int64: int64(tt.workspaceTTL)},
})
job = dbgen.ProvisionerJob(t, db, database.ProvisionerJob{
OrganizationID: org.ID,
CompletedAt: tt.jobCompletedAt,
})
_ = dbgen.WorkspaceResource(t, db, database.WorkspaceResource{
JobID: job.ID,
})
buildID = uuid.New()
)
var buildNumber int32 = 1
// Insert a number of previous workspace builds.
for i := 0; i < 5; i++ {
insertPrevWorkspaceBuild(t, db, org.ID, templateVersion.ID, ws.ID, database.WorkspaceTransitionStart, buildNumber)
buildNumber++
insertPrevWorkspaceBuild(t, db, org.ID, templateVersion.ID, ws.ID, database.WorkspaceTransitionStop, buildNumber)
buildNumber++
}
var buildNumber int32 = 1
// Insert a number of previous workspace builds.
for i := 0; i < 5; i++ {
insertPrevWorkspaceBuild(t, db, org.ID, templateVersion.ID, ws.ID, database.WorkspaceTransitionStart, buildNumber)
buildNumber++
insertPrevWorkspaceBuild(t, db, org.ID, templateVersion.ID, ws.ID, database.WorkspaceTransitionStop, buildNumber)
buildNumber++
}
// dbgen.WorkspaceBuild automatically sets deadline to now+1 hour if not set
var buildDeadline time.Time
if tt.buildDeadlineOffset != nil {
buildDeadline = now.Add(*tt.buildDeadlineOffset)
}
var maxDeadline time.Time
if tt.maxDeadlineOffset != nil {
maxDeadline = now.Add(*tt.maxDeadlineOffset)
}
err := db.InsertWorkspaceBuild(ctx, database.InsertWorkspaceBuildParams{
ID: buildID,
CreatedAt: dbtime.Now(),
UpdatedAt: dbtime.Now(),
BuildNumber: buildNumber,
InitiatorID: user.ID,
Reason: database.BuildReasonInitiator,
WorkspaceID: ws.ID,
JobID: job.ID,
TemplateVersionID: templateVersion.ID,
Transition: tt.transition,
Deadline: buildDeadline,
MaxDeadline: maxDeadline,
})
require.NoError(t, err, "unexpected error inserting workspace build")
bld, err := db.GetWorkspaceBuildByID(ctx, buildID)
require.NoError(t, err, "unexpected error fetching inserted workspace build")
// dbgen.WorkspaceBuild automatically sets deadline to now+1 hour if not set
var buildDeadline time.Time
if tt.buildDeadlineOffset != nil {
buildDeadline = now.Add(*tt.buildDeadlineOffset)
}
var maxDeadline time.Time
if tt.maxDeadlineOffset != nil {
maxDeadline = now.Add(*tt.maxDeadlineOffset)
}
err := db.InsertWorkspaceBuild(ctx, database.InsertWorkspaceBuildParams{
ID: buildID,
CreatedAt: dbtime.Now(),
UpdatedAt: dbtime.Now(),
BuildNumber: buildNumber,
InitiatorID: user.ID,
Reason: database.BuildReasonInitiator,
WorkspaceID: ws.ID,
JobID: job.ID,
TemplateVersionID: templateVersion.ID,
Transition: tt.transition,
Deadline: buildDeadline,
MaxDeadline: maxDeadline,
})
require.NoError(t, err, "unexpected error inserting workspace build")
bld, err := db.GetWorkspaceBuildByID(ctx, buildID)
require.NoError(t, err, "unexpected error fetching inserted workspace build")
// Validate our initial state before bump
require.Equal(t, tt.transition, bld.Transition, "unexpected transition before bump")
require.Equal(t, tt.jobCompletedAt.Time.UTC(), job.CompletedAt.Time.UTC(), "unexpected job completed at before bump")
require.Equal(t, buildDeadline.UTC(), bld.Deadline.UTC(), "unexpected build deadline before bump")
require.Equal(t, maxDeadline.UTC(), bld.MaxDeadline.UTC(), "unexpected max deadline before bump")
require.Equal(t, tt.workspaceTTL, time.Duration(ws.Ttl.Int64), "unexpected workspace TTL before bump")
// Validate our initial state before bump
require.Equal(t, tt.transition, bld.Transition, "unexpected transition before bump")
require.Equal(t, tt.jobCompletedAt.Time.UTC(), job.CompletedAt.Time.UTC(), "unexpected job completed at before bump")
require.Equal(t, buildDeadline.UTC(), bld.Deadline.UTC(), "unexpected build deadline before bump")
require.Equal(t, maxDeadline.UTC(), bld.MaxDeadline.UTC(), "unexpected max deadline before bump")
require.Equal(t, tt.workspaceTTL, time.Duration(ws.Ttl.Int64), "unexpected workspace TTL before bump")
workaroundWindowsTimeResolution(t)
// Wait a bit before bumping as dbtime is rounded to the nearest millisecond.
// This should also hopefully be enough for Windows time resolution to register
// a tick (win32 max timer resolution is apparently between 0.5 and 15.6ms)
<-time.After(testutil.IntervalFast)
// Bump duration is measured from the time of the bump, so we measure from here.
start := dbtime.Now()
activityBumpWorkspace(ctx, log, db, bld.WorkspaceID)
elapsed := time.Since(start)
if elapsed > 15*time.Second {
t.Logf("warning: activityBumpWorkspace took longer than 15 seconds: %s", elapsed)
}
// The actual bump could have happened anywhere in the elapsed time, so we
// guess at the approximate time of the bump.
approxBumpTime := start.Add(elapsed / 2)
// Bump duration is measured from the time of the bump, so we measure from here.
start := dbtime.Now()
activityBumpWorkspace(ctx, log, db, bld.WorkspaceID)
end := dbtime.Now()
// Validate our state after bump
updatedBuild, err := db.GetLatestWorkspaceBuildByWorkspaceID(ctx, bld.WorkspaceID)
require.NoError(t, err, "unexpected error getting latest workspace build")
if tt.expectedBump == 0 {
require.Equal(t, bld.UpdatedAt.UTC(), updatedBuild.UpdatedAt.UTC(), "should not have bumped updated_at")
require.Equal(t, bld.Deadline.UTC(), updatedBuild.Deadline.UTC(), "should not have bumped deadline")
} else {
// Validate our state after bump
updatedBuild, err := db.GetLatestWorkspaceBuildByWorkspaceID(ctx, bld.WorkspaceID)
require.NoError(t, err, "unexpected error getting latest workspace build")
require.Equal(t, bld.MaxDeadline.UTC(), updatedBuild.MaxDeadline.UTC(), "max_deadline should not have changed")
if tt.expectedBump == 0 {
require.Equal(t, bld.UpdatedAt.UTC(), updatedBuild.UpdatedAt.UTC(), "should not have bumped updated_at")
require.Equal(t, bld.Deadline.UTC(), updatedBuild.Deadline.UTC(), "should not have bumped deadline")
return
}
require.NotEqual(t, bld.UpdatedAt.UTC(), updatedBuild.UpdatedAt.UTC(), "should have bumped updated_at")
expectedDeadline := approxBumpTime.Add(tt.expectedBump).UTC()
// Note: if CI is especially slow, this test may fail. There is an internal 15-second
// deadline in activityBumpWorkspace, so we allow the same window here.
require.WithinDuration(t, expectedDeadline, updatedBuild.Deadline.UTC(), 15*time.Second, "unexpected deadline after bump")
}
})
if tt.maxDeadlineOffset != nil {
require.Equal(t, bld.MaxDeadline.UTC(), updatedBuild.MaxDeadline.UTC(), "new deadline must equal original max deadline")
return
}
// Assert that the bump occurred between start and end.
expectedDeadlineStart := start.Add(tt.expectedBump)
expectedDeadlineEnd := end.Add(tt.expectedBump)
require.GreaterOrEqual(t, updatedBuild.Deadline, expectedDeadlineStart, "new deadline should be greater than or equal to start")
require.LessOrEqual(t, updatedBuild.Deadline, expectedDeadlineEnd, "new deadline should be lesser than or equal to end")
})
}
}
}
@ -223,11 +239,3 @@ func insertPrevWorkspaceBuild(t *testing.T, db database.Store, orgID, tvID, work
Transition: transition,
})
}
func workaroundWindowsTimeResolution(t *testing.T) {
t.Helper()
if runtime.GOOS == "windows" {
t.Logf("workaround: sleeping for a short time to avoid time resolution issues on Windows")
<-time.After(testutil.IntervalSlow)
}
}

View File

@ -30,6 +30,7 @@ func TestWorkspaceActivityBump(t *testing.T) {
// doesn't use template autostop requirements and instead edits the
// max_deadline on the build directly in the database.
setupActivityTest := func(t *testing.T, deadline ...time.Duration) (client *codersdk.Client, workspace codersdk.Workspace, assertBumped func(want bool)) {
t.Helper()
const ttl = time.Minute
maxTTL := time.Duration(0)
if len(deadline) > 0 {
@ -120,6 +121,7 @@ func TestWorkspaceActivityBump(t *testing.T) {
_ = coderdtest.AwaitWorkspaceAgents(t, client, workspace.ID)
return client, workspace, func(want bool) {
t.Helper()
if !want {
// It is difficult to test the absence of a call in a non-racey
// way. In general, it is difficult for the API to generate
@ -134,24 +136,32 @@ func TestWorkspaceActivityBump(t *testing.T) {
return
}
var updatedAfter time.Time
// The Deadline bump occurs asynchronously.
require.Eventuallyf(t,
func() bool {
workspace, err = client.Workspace(ctx, workspace.ID)
require.NoError(t, err)
return workspace.LatestBuild.Deadline.Time != firstDeadline
updatedAfter = dbtime.Now()
if workspace.LatestBuild.Deadline.Time == firstDeadline {
updatedAfter = time.Now()
return false
}
return true
},
testutil.WaitLong, testutil.IntervalFast,
"deadline %v never updated", firstDeadline,
)
require.Greater(t, workspace.LatestBuild.Deadline.Time, updatedAfter)
// If the workspace has a max deadline, the deadline must not exceed
// it.
if maxTTL != 0 && dbtime.Now().Add(ttl).After(workspace.LatestBuild.MaxDeadline.Time) {
require.Equal(t, workspace.LatestBuild.Deadline.Time, workspace.LatestBuild.MaxDeadline.Time)
if workspace.LatestBuild.MaxDeadline.Valid {
require.LessOrEqual(t, workspace.LatestBuild.Deadline.Time, workspace.LatestBuild.MaxDeadline.Time)
return
}
require.WithinDuration(t, dbtime.Now().Add(ttl), workspace.LatestBuild.Deadline.Time, 3*time.Second)
require.WithinDuration(t, dbtime.Now().Add(ttl), workspace.LatestBuild.Deadline.Time, testutil.WaitShort)
}
}
@ -210,12 +220,6 @@ func TestWorkspaceActivityBump(t *testing.T) {
require.NoError(t, err)
_ = sshConn.Close()
assertBumped(true)
// Double check that the workspace build's deadline is equal to the
// max deadline.
workspace, err = client.Workspace(ctx, workspace.ID)
require.NoError(t, err)
require.Equal(t, workspace.LatestBuild.Deadline.Time, workspace.LatestBuild.MaxDeadline.Time)
assertBumped(true) // also asserts max ttl not exceeded
})
}

View File

@ -3,7 +3,10 @@ package dbtestutil
import (
"context"
"database/sql"
"fmt"
"net/url"
"os"
"strings"
"testing"
"github.com/stretchr/testify/require"
@ -19,9 +22,27 @@ func WillUsePostgres() bool {
return os.Getenv("DB") != ""
}
func NewDB(t testing.TB) (database.Store, pubsub.Pubsub) {
type options struct {
fixedTimezone string
}
type Option func(*options)
// WithTimezone sets the database to the defined timezone.
func WithTimezone(tz string) Option {
return func(o *options) {
o.fixedTimezone = tz
}
}
func NewDB(t testing.TB, opts ...Option) (database.Store, pubsub.Pubsub) {
t.Helper()
var o options
for _, opt := range opts {
opt(&o)
}
db := dbfake.New()
ps := pubsub.NewInMemory()
if WillUsePostgres() {
@ -35,6 +56,19 @@ func NewDB(t testing.TB) (database.Store, pubsub.Pubsub) {
require.NoError(t, err)
t.Cleanup(closePg)
}
if o.fixedTimezone == "" {
// To make sure we find timezone-related issues, we set the timezone
// of the database to a non-UTC one.
// The below was picked due to the following properties:
// - It has a non-UTC offset
// - It has a fractional hour UTC offset
// - It includes a daylight savings time component
o.fixedTimezone = "Canada/Newfoundland"
}
dbName := dbNameFromConnectionURL(t, connectionURL)
setDBTimezone(t, connectionURL, dbName, o.fixedTimezone)
sqlDB, err := sql.Open("postgres", connectionURL)
require.NoError(t, err)
t.Cleanup(func() {
@ -51,3 +85,28 @@ func NewDB(t testing.TB) (database.Store, pubsub.Pubsub) {
return db, ps
}
// setRandDBTimezone sets the timezone of the database to the given timezone.
// Note that the updated timezone only comes into effect on reconnect, so we
// create our own connection for this and close the DB after we're done.
func setDBTimezone(t testing.TB, dbURL, dbname, tz string) {
t.Helper()
sqlDB, err := sql.Open("postgres", dbURL)
require.NoError(t, err)
defer func() {
_ = sqlDB.Close()
}()
// nolint: gosec // This unfortunately does not work with placeholders.
_, err = sqlDB.Exec(fmt.Sprintf("ALTER DATABASE %s SET TIMEZONE TO %q", dbname, tz))
require.NoError(t, err, "failed to set timezone for database")
}
// dbNameFromConnectionURL returns the database name from the given connection URL,
// where connectionURL is of the form postgres://user:pass@host:port/dbname
func dbNameFromConnectionURL(t testing.TB, connectionURL string) string {
u, err := url.Parse(connectionURL)
require.NoError(t, err)
return strings.TrimPrefix(u.Path, "/")
}

File diff suppressed because it is too large Load Diff

View File

@ -19,10 +19,10 @@ const activityBumpWorkspace = `-- name: ActivityBumpWorkspace :exec
WITH latest AS (
SELECT
workspace_builds.id::uuid AS build_id,
workspace_builds.deadline::timestamp AS build_deadline,
workspace_builds.max_deadline::timestamp AS build_max_deadline,
workspace_builds.deadline::timestamp with time zone AS build_deadline,
workspace_builds.max_deadline::timestamp with time zone AS build_max_deadline,
workspace_builds.transition AS build_transition,
provisioner_jobs.completed_at::timestamp AS job_completed_at,
provisioner_jobs.completed_at::timestamp with time zone AS job_completed_at,
(workspaces.ttl / 1000 / 1000 / 1000 || ' seconds')::interval AS ttl_interval
FROM workspace_builds
JOIN provisioner_jobs

View File

@ -6,10 +6,10 @@
WITH latest AS (
SELECT
workspace_builds.id::uuid AS build_id,
workspace_builds.deadline::timestamp AS build_deadline,
workspace_builds.max_deadline::timestamp AS build_max_deadline,
workspace_builds.deadline::timestamp with time zone AS build_deadline,
workspace_builds.max_deadline::timestamp with time zone AS build_max_deadline,
workspace_builds.transition AS build_transition,
provisioner_jobs.completed_at::timestamp AS job_completed_at,
provisioner_jobs.completed_at::timestamp with time zone AS job_completed_at,
(workspaces.ttl / 1000 / 1000 / 1000 || ' seconds')::interval AS ttl_interval
FROM workspace_builds
JOIN provisioner_jobs

View File

@ -23,6 +23,7 @@ import (
"github.com/coder/coder/v2/agent"
"github.com/coder/coder/v2/coderd/coderdtest"
"github.com/coder/coder/v2/coderd/database"
"github.com/coder/coder/v2/coderd/database/dbtestutil"
"github.com/coder/coder/v2/coderd/database/dbtime"
"github.com/coder/coder/v2/codersdk"
"github.com/coder/coder/v2/codersdk/agentsdk"
@ -1184,8 +1185,12 @@ func TestWorkspaceAgent_LifecycleState(t *testing.T) {
func TestWorkspaceAgent_Metadata(t *testing.T) {
t.Parallel()
// nolint:gocritic // https://github.com/coder/coder/issues/9682
db, ps := dbtestutil.NewDB(t, dbtestutil.WithTimezone("UTC"))
client := coderdtest.New(t, &coderdtest.Options{
IncludeProvisionerDaemon: true,
Database: db,
Pubsub: ps,
})
user := coderdtest.CreateFirstUser(t, client)
authToken := uuid.NewString()

View File

@ -1514,8 +1514,13 @@ func TestWorkspaceFilterManual(t *testing.T) {
t.Run("LastUsed", func(t *testing.T) {
t.Parallel()
// nolint:gocritic // https://github.com/coder/coder/issues/9682
db, ps := dbtestutil.NewDB(t, dbtestutil.WithTimezone("UTC"))
client, _, api := coderdtest.NewWithAPI(t, &coderdtest.Options{
IncludeProvisionerDaemon: true,
Database: db,
Pubsub: ps,
})
user := coderdtest.CreateFirstUser(t, client)
authToken := uuid.NewString()

View File

@ -13,6 +13,7 @@ import (
"github.com/coder/coder/v2/coderd/audit"
"github.com/coder/coder/v2/coderd/coderdtest"
"github.com/coder/coder/v2/coderd/database"
"github.com/coder/coder/v2/coderd/database/dbtestutil"
"github.com/coder/coder/v2/coderd/rbac"
"github.com/coder/coder/v2/codersdk"
"github.com/coder/coder/v2/cryptorand"
@ -368,10 +369,14 @@ func TestTemplates(t *testing.T) {
t.Run("UpdateLastUsedAt", func(t *testing.T) {
t.Parallel()
// nolint:gocritic // https://github.com/coder/coder/issues/9682
db, ps := dbtestutil.NewDB(t, dbtestutil.WithTimezone("UTC"))
ctx := testutil.Context(t, testutil.WaitMedium)
client, user := coderdenttest.New(t, &coderdenttest.Options{
Options: &coderdtest.Options{
IncludeProvisionerDaemon: true,
Database: db,
Pubsub: ps,
},
LicenseOptions: &coderdenttest.LicenseOptions{
Features: license.Features{

View File

@ -15,6 +15,7 @@ import (
"github.com/coder/coder/v2/coderd/autobuild"
"github.com/coder/coder/v2/coderd/coderdtest"
"github.com/coder/coder/v2/coderd/database"
"github.com/coder/coder/v2/coderd/database/dbtestutil"
agplschedule "github.com/coder/coder/v2/coderd/schedule"
"github.com/coder/coder/v2/coderd/schedule/cron"
"github.com/coder/coder/v2/coderd/util/ptr"
@ -641,9 +642,14 @@ func TestWorkspacesFiltering(t *testing.T) {
dormantTTL := 24 * time.Hour
// nolint:gocritic // https://github.com/coder/coder/issues/9682
db, ps := dbtestutil.NewDB(t, dbtestutil.WithTimezone("UTC"))
client, user := coderdenttest.New(t, &coderdenttest.Options{
Options: &coderdtest.Options{
IncludeProvisionerDaemon: true,
Database: db,
Pubsub: ps,
},
LicenseOptions: &coderdenttest.LicenseOptions{
Features: license.Features{

View File

@ -395,3 +395,16 @@ func slogError(m dsl.Matcher) {
Where(m["name"].Const && m["value"].Type.Is("error") && !m["name"].Text.Matches(`^"internal_error"$`)).
Report(`Error should be logged using "slog.Error" instead.`)
}
// withTimezoneUTC ensures that we don't just sprinkle dbtestutil.WithTimezone("UTC") about
// to work around real timezone bugs in our code.
//
//nolint:unused,deadcode,varnamelen
func withTimezoneUTC(m dsl.Matcher) {
m.Match(
`dbtestutil.WithTimezone($tz)`,
).Where(
m["tz"].Text.Matches(`[uU][tT][cC]"$`),
).Report(`Setting database timezone to UTC may mask timezone-related bugs.`).
At(m["tz"])
}