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

johnstcn · 2023-09-13T16:51:21Z

This PR:

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 has already surfaced a few timezone-related bugs that we can fix in follow-up PRs, but which are now obvious and trackable via their use of dbtestutil.WithTimezone("UTC").

coderd/database/dbtestutil/db.go

johnstcn · 2023-09-13T16:59:12Z

coderd/database/dbtestutil/db.go

+	t.Logf("setting timezone of database %q to %q", dbname, tz)
+	// We apparently can't use placeholders here, sadly.
+	// nolint: gosec // This is not user input and this is only executed in tests
+	_, err = sqlDB.Exec(fmt.Sprintf("ALTER DATABASE %s SET TIMEZONE TO %q", dbname, tz))


self-review: I hate this but it appears necessary.

It's fine in tests, IMO. It's an unfortunate limitation of some PostgreSQL queries.

Emyrk · 2023-09-13T17:44:14Z

Some chaos could find a decent amount of TZ bugs

johnstcn · 2023-09-14T11:04:54Z

coderd/database/queries/activitybump.sql

It looks like I had timezone-related bug in #9652 that this surfaced.

johnstcn · 2023-09-14T11:05:47Z

coderd/database/dbtestutil/db.go

+// WithTimezone sets the database to the defined timezone instead of
+// to a random one.
+//
+// Deprecated: If you need to use this, you may have a timezone-related bug.
+func WithTimezone(tz string) Option {
+	return func(o *options) {
+		o.fixedTimezone = tz
+	}
+}


review: I originally exposed this as WithFixedTimezone() but I quickly realized that it's useful to be able to set a specific timezone in unit tests, especially when you're trying to narrow down the root cause of a timezone-related bug.

johnstcn · 2023-09-14T11:06:20Z

coderd/workspaces_test.go

@@ -1510,8 +1510,11 @@ func TestWorkspaceFilterManual(t *testing.T) {

 	t.Run("LastUsed", func(t *testing.T) {
 		t.Parallel()
+		db, ps := dbtestutil.NewDB(t, dbtestutil.WithTimezone("UTC"))


review: LastUsed is apparently TIMESTAMP WITHOUT TIME ZONE. Not sure why this is.

There's no comment on the DB field explaining the reasoning, so it's definitely a bug.

The migration that added it made a mistake of using timestamp vs timestamptz.

000043_workspace_last_used.up.sql 2: ADD COLUMN last_used_at timestamp NOT NULL DEFAULT '0001-01-01 00:00:00+00:00';

(See conflict between default value and field type.)

We should probably lint this somehow and prevent addition of timestamp columns unless //nolinted.

Same bug in users table with last_seen_at.

Filed #9682 to follow up.

johnstcn · 2023-09-14T11:08:37Z

coderd/database/dbtestutil/randtz/randtz.go

review: I initially had been setting a random timezone for each unit test, but I quickly found that way too chaotic to be useful.

It's more useful to pick random timezone once and going with that for all subsequent invocations. If things fail, they will at least fail consistently that run.

mtojek · 2023-09-14T11:10:42Z

@johnstcn Could you please provide some description so we can see the reasoning behind it?

johnstcn · 2023-09-14T11:11:10Z

@johnstcn Could you please provide some description so we can see the reasoning behind it?

Updated description, you beat me in the data race :-)

spikecurtis · 2023-09-14T11:16:43Z

I think it's generally a lot better to have deterministic tests than stochastic ones. Flaky tests lower the teams trust in our test suite, and eat a lot of time.

In features that have dependencies on the database time zone, we should be adding (possibly exhaustive) test cases that use different time zones.

johnstcn · 2023-09-14T11:24:17Z

I think it's generally a lot better to have deterministic tests than stochastic ones. Flaky tests lower the teams trust in our test suite, and eat a lot of time.

In features that have dependencies on the database time zone, we should be adding (possibly exhaustive) test cases that use different time zones.

That's a good point. Instead, I can set to a fixed non-UTC timezone by default that does have a DST component, and keep the option of having a random timezone.

mafredri

Nice addition!

mafredri · 2023-09-14T11:22:18Z

coderd/database/dbtestutil/db.go

+	t.Logf("setting timezone of database %q to %q", dbname, tz)
+	// We apparently can't use placeholders here, sadly.
+	// nolint: gosec // This is not user input and this is only executed in tests
+	_, err = sqlDB.Exec(fmt.Sprintf("ALTER DATABASE %s SET TIMEZONE TO %q", dbname, tz))


It's fine in tests, IMO. It's an unfortunate limitation of some PostgreSQL queries.

coderd/database/queries/activitybump.sql

coderd/database/dbtestutil/db.go

enterprise/coderd/workspaces_test.go

coderd/activitybump_internal_test.go

Emyrk · 2023-09-13T17:43:49Z

coderd/database/dbtestutil/db.go

+	require.NoError(t, err)
+
+	// Set the timezone for the database.
+	t.Logf("setting timezone of database %q to %q", dbname, tz)


mtojek

Thanks for addressing the feedback!

…ld hopefully be greater than win32 max timer resolution

…t equal max_deadline" to actually be "deadline must not exceed max_deadline" as comments suggest

johnstcn self-assigned this Sep 13, 2023

johnstcn commented Sep 13, 2023

View reviewed changes

feat(coderd/database/dbtestutil): set a random timezone in the db

355b16b

johnstcn force-pushed the cj/dbtestutil-rand-tz branch from aa070c4 to 355b16b Compare September 14, 2023 08:15

johnstcn added 6 commits September 14, 2023 09:25

remove check for tz after set

986db8b

clitest: fix timestamp regex to match non-UTC timezones

fc3ba52

set fixed timezone in some existing unit tests

b3c559a

fixup! set fixed timezone in some existing unit tests

f57dc9d

fix timezone bug in ActivityBumpWorkspace query

d3d1274

refactor to separate package, allow specifying tz

9f1f5c6

johnstcn commented Sep 14, 2023

View reviewed changes

johnstcn marked this pull request as ready for review September 14, 2023 11:09

johnstcn requested review from mafredri, spikecurtis, Emyrk and mtojek September 14, 2023 11:09

mafredri reviewed Sep 14, 2023

View reviewed changes

johnstcn added 3 commits September 14, 2023 12:46

set db timezone to canada/newfoundland by default

7c2ff2f

test activity bump in multiple timezones

eecfcdb

remove unnecessary IN TIME ZONE UTC in activitybump query

d327131

johnstcn changed the title ~~feat(coderd/database/dbtestutil): set a random timezone in the db~~ feat(coderd/database/dbtestutil): set default database timezone to non-UTC in unit tests Sep 14, 2023

johnstcn mentioned this pull request Sep 14, 2023

bug(coderd/database): workspaces.last_used_at is missing timezone component #9682

Closed

mtojek reviewed Sep 14, 2023

View reviewed changes

coderd/database/dbtestutil/db.go Outdated Show resolved Hide resolved

coderd/database/dbtestutil/db.go Show resolved Hide resolved

enterprise/coderd/workspaces_test.go Show resolved Hide resolved

coderd/activitybump_internal_test.go Show resolved Hide resolved

Emyrk approved these changes Sep 14, 2023

View reviewed changes

clarify use of multiple timezones in test

7680984

johnstcn added 3 commits September 14, 2023 15:21

remove deprecation and add linter rule

d051bbb

fix typo

da059dc

bump allowed withinDuration for TestWorkpaceActivityBump

0d425ba

mtojek self-requested a review September 14, 2023 14:59

mtojek approved these changes Sep 14, 2023

View reviewed changes

johnstcn added 3 commits September 14, 2023 16:34

make Test_ActivityBumpWorkspace more flake-resistant

d97ff2f

replace conditional sleep with a slower unconditional sleep that shou…

f18c6b5

…ld hopefully be greater than win32 max timer resolution

modify logic slightly in TestWorkspaceActivityBump from "deadline mus…

5a8aa8b

…t equal max_deadline" to actually be "deadline must not exceed max_deadline" as comments suggest

johnstcn merged commit 65db7a7 into main Sep 15, 2023

johnstcn deleted the cj/dbtestutil-rand-tz branch September 15, 2023 08:01

github-actions bot locked and limited conversation to collaborators Sep 15, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

johnstcn commented Sep 13, 2023 •

edited

Loading

johnstcn Sep 13, 2023

mafredri Sep 14, 2023

Emyrk commented Sep 13, 2023

johnstcn Sep 14, 2023

johnstcn Sep 14, 2023

johnstcn Sep 14, 2023

mafredri Sep 14, 2023 •

edited

Loading

mafredri Sep 14, 2023

johnstcn Sep 14, 2023

johnstcn Sep 14, 2023

mtojek commented Sep 14, 2023

johnstcn commented Sep 14, 2023

spikecurtis commented Sep 14, 2023

johnstcn commented Sep 14, 2023

mafredri left a comment

mafredri Sep 14, 2023

Emyrk Sep 13, 2023

mtojek left a comment

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

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

Conversation

johnstcn commented Sep 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Emyrk commented Sep 13, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mafredri Sep 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtojek commented Sep 14, 2023

johnstcn commented Sep 14, 2023

spikecurtis commented Sep 14, 2023

johnstcn commented Sep 14, 2023

mafredri left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtojek left a comment

Choose a reason for hiding this comment

johnstcn commented Sep 13, 2023 •

edited

Loading

mafredri Sep 14, 2023 •

edited

Loading