NotificationsYou must be signed in to change notification settings
Fork1k
Star11.1k

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

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.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

johnstcn merged 17 commits intomainfromcj/dbtestutil-rand-tz

Sep 15, 2023

Merged

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

johnstcn merged 17 commits intomainfromcj/dbtestutil-rand-tz

Sep 15, 2023

Conversation

Copy link

Member

johnstcn commentedSep 13, 2023•
edited
Loading

This PR:

Addsdbtestutil.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 facilityrandtz.Name() to pick a random timezone which is consistent across subtests (viasync.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 ofdbtestutil.WithTimezone("UTC").

johnstcn self-assigned this

Sep 13, 2023

johnstcn commented

Sep 13, 2023

View reviewed changes

coderd/database/dbtestutil/db.go OutdatedShow resolvedHide resolved

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))

Copy link

MemberAuthor

Choose a reason for hiding this comment

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

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

Copy link

Member

Choose a reason for hiding this comment

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

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

Copy link

Member

Emyrk commentedSep 13, 2023

Some chaos could find a decent amount of TZ bugs

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

355b16b

johnstcn force-pushed thecj/dbtestutil-rand-tz branch fromaa070c4 to355b16bCompare

September 14, 2023 08:15

johnstcn added6 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

coderd/database/queries/activitybump.sql

Copy link

MemberAuthor

Choose a reason for hiding this comment

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

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

coderd/database/dbtestutil/db.go Outdated

Comment on lines 32 to 40

		// 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.
		funcWithTimezone(tzstring)Option {
		returnfunc(o*options) {
		o.fixedTimezone=tz
		}
		}

Copy link

MemberAuthor

Choose a reason for hiding this comment

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

review: I originally exposed this asWithFixedTimezone() 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.

coderd/workspaces_test.go


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

Copy link

MemberAuthor

johnstcnSep 14, 2023

Choose a reason for hiding this comment

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

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

Copy link

Member

mafredriSep 14, 2023•
edited
Loading

Choose a reason for hiding this comment

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

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 usingtimestamp vstimestamptz.

000043_workspace_last_used.up.sql2:    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 oftimestamp columns unless//nolinted.

Copy link

Member

mafredriSep 14, 2023

Choose a reason for hiding this comment

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

Same bug inusers table withlast_seen_at.

Copy link

MemberAuthor

johnstcnSep 14, 2023

Choose a reason for hiding this comment

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

Filed#9682 to follow up.

coderd/database/dbtestutil/randtz/randtz.go

Copy link

MemberAuthor

johnstcnSep 14, 2023

Choose a reason for hiding this comment

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

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.

johnstcn marked this pull request as ready for review

September 14, 2023 11:09

johnstcn requested review frommafredri,spikecurtis,Emyrk andmtojek

September 14, 2023 11:09

Copy link

Member

mtojek commentedSep 14, 2023

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

Copy link

MemberAuthor

johnstcn commentedSep 14, 2023

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

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

Copy link

Contributor

spikecurtis commentedSep 14, 2023

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.

Copy link

MemberAuthor

johnstcn commentedSep 14, 2023

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 reviewed

Sep 14, 2023

View reviewed changes

Copy link

Member

mafredri left a comment

Choose a reason for hiding this comment

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

Nice addition!

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))

Copy link

Member

mafredriSep 14, 2023

Choose a reason for hiding this comment

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

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

coderd/database/queries/activitybump.sql OutdatedShow resolvedHide resolved

johnstcn added3 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 OutdatedShow resolvedHide resolved

coderd/database/dbtestutil/db.goShow resolvedHide resolved

enterprise/coderd/workspaces_test.goShow resolvedHide resolved

coderd/activitybump_internal_test.goShow resolvedHide resolved

Emyrk approved these changes

Sep 14, 2023

View reviewed changes

coderd/database/dbtestutil/db.go Outdated

		require.NoError(t,err)

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

Copy link

Member

EmyrkSep 13, 2023

Choose a reason for hiding this comment

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

👍

clarify use of multiple timezones in test

7680984

johnstcn added3 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

Copy link

Member

mtojek left a comment

Choose a reason for hiding this comment

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

Thanks for addressing the feedback!

johnstcn added3 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 commit65db7a7 intomain

Sep 15, 2023

johnstcn deleted the cj/dbtestutil-rand-tz branch

September 15, 2023 08:01

github-actionsbot locked and limited conversation to collaborators

Sep 15, 2023

Labels

None yet

5 participants

Movatterモバイル変換

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

Uh oh!

Conversation

johnstcn commentedSep 13, 2023• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Emyrk commentedSep 13, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mafredriSep 14, 2023• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mtojek commentedSep 14, 2023

Uh oh!

johnstcn commentedSep 14, 2023

Uh oh!

spikecurtis commentedSep 14, 2023

Uh oh!

johnstcn commentedSep 14, 2023

Uh oh!

mafredri left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mtojek left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

johnstcn commentedSep 13, 2023•
edited
Loading

mafredriSep 14, 2023•
edited
Loading