Skip to content

Put db in UTC+1 for integration tests #4224

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 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 3 commits into from
Apr 21, 2025

Conversation

tjenkinson
Copy link
Contributor

Followup for #4216

I tested (zql-integration-tests) with the patch in the above PR reverted and things broke, so this adds some coverage

Copy link

vercel bot commented Apr 14, 2025

@tjenkinson is attempting to deploy a commit to the Rocicorp Team on Vercel.

A member of the Team first needs to authorize it.

@@ -198,6 +198,7 @@ CREATE TABLE IF NOT EXISTS "comments" (
"authorId" TEXT NOT NULL,
"issue_id" TEXT NOT NULL,
"text" TEXT NOT NULL,
-- not TIMESTAMPTZ so that we're checking both TIMESTAMP and TIMESTAMPTZ behaviour
"createdAt" TIMESTAMP NOT NULL
Copy link
Contributor Author

Choose a reason for hiding this comment

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

seems weird to have 2 different types for time in the same db, but maybe this is fine for testing?

Otherwise maybe it should be a new column, but then more tests would need updating

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I think this is fine for the test schema since we want to try various types.

@tjenkinson tjenkinson marked this pull request as ready for review April 14, 2025 07:47
@tjenkinson
Copy link
Contributor Author

Hmm I think the failures might be real.

Bit confused because in sql.ts createPlaceholder there's also some handling where it's plural and that's missing the AT TIME ZONE 'UTC' for the timestamp cases. Maybe it needs it?

But making that change still doesn't fix the tests

@tantaman
Copy link
Contributor

Bit confused because in sql.ts createPlaceholder there's also some handling where it's plural and that's missing the AT TIME ZONE 'UTC' for the timestamp cases. Maybe it needs it?

Checking this out. Been on spring break over the last week.

@tantaman
Copy link
Contributor

Some of the test failures are having too much precision from Postgres. That's not too bad.

CleanShot 2025-04-21 at 12 19 34

The real worrying ones are the ones with completely different values before the decimal part.

CleanShot 2025-04-21 at 12 19 45

@tantaman tantaman force-pushed the tz-integration-test branch from 74a5e0d to 44c9eda Compare April 21, 2025 16:28
@tantaman
Copy link
Contributor

tantaman commented Apr 21, 2025

The real worrying ones are the ones with completely different values before the decimal part.

This was caused by the CVR tests not inserting with a timezone and so defaulting to the server's timezone.

44c9eda

Note to self (and @darkgnotic) cvr-store::putDesiredQuery may be incorrect if inserting a number does not also carry the timezone.

edit: looks like serializeTimestamp does the right thing by converting to toISOString which will carry the timezone.

function serializeTimestamp(val: unknown): string {
switch (typeof val) {
case 'string':
return val; // Let Postgres parse it
case 'number': {
if (Number.isInteger(val)) {
return new PreciseDate(val).toISOString();
}
// Convert floating point to bigint nanoseconds.
const nanoseconds =
1_000_000n * BigInt(Math.trunc(val)) +
BigInt(Math.trunc((val % 1) * 1e6));
return new PreciseDate(nanoseconds).toISOString();
}
// Note: Don't support bigint inputs until we decide what the semantics are (e.g. micros vs nanos)
// case 'bigint':
// return new PreciseDate(val).toISOString();
default:
if (val instanceof Date) {
return val.toISOString();
}
}
throw new Error(`Unsupported type "${typeof val}" for timestamp: ${val}`);
}

@tantaman tantaman merged commit 30695ef into rocicorp:main Apr 21, 2025
8 of 10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants