Skip to content

feat(tests): Add integration tests for pg_replicate #121

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 34 commits into from
May 21, 2025

Conversation

iambriccardo
Copy link
Contributor

@iambriccardo iambriccardo commented May 19, 2025

This PR adds a new integrations tests suite for pg_replicate, which allows the writing of integration tests for synchronous and asynchronous pipelines.

As part of this integration tests suite, a new module postgres was added, to share common PostgreSQL functionality (for both tokio_postgres and sqlx) that can be shared across crates (e.g., the configuration object).

Comment on lines +6 to +10
///
/// Similar to [`create_pg_database`], but additionally runs all database migrations
/// from the "./migrations" directory after creation. Returns a [`PgPool`]
/// connected to the newly created and migrated database. Panics if database
/// creation or migration fails.
Copy link
Contributor

Choose a reason for hiding this comment

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

Move panic docs to the Panics section

Suggested change
///
/// Similar to [`create_pg_database`], but additionally runs all database migrations
/// from the "./migrations" directory after creation. Returns a [`PgPool`]
/// connected to the newly created and migrated database. Panics if database
/// creation or migration fails.
///
/// Similar to [`create_pg_database`], but additionally runs all database migrations
/// from the "./migrations" directory after creation. Returns a [`PgPool`]
/// connected to the newly created and migrated database.
///
/// # Panics
///
/// Panics if database creation or migration fails.

@iambriccardo iambriccardo marked this pull request as ready for review May 21, 2025 10:12
@iambriccardo iambriccardo requested a review from a team as a code owner May 21, 2025 10:12
@iambriccardo iambriccardo requested a review from imor May 21, 2025 10:12

async fn double_users_ages(database: &PgDatabase) {
database
.update_values(test_table_name("users"), &["age"], &[&"age * 2"])
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
.update_values(test_table_name("users"), &["age"], &[&"age * 2"])
.update_values(test_table_name("users"), &["age"], &["age * 2"])

.await
.expect("Failed to create database");

// Create a connection pool to the database and run the migration.
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: migrations don't run here as per the comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! I left it from a previous impl.

);
self.client.execute(&create_publication_query, &[]).await?;

Ok(publication_name.to_string())
Copy link
Contributor

Choose a reason for hiding this comment

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

We can avoid returning the publication name string here as the caller already knows it.

Comment on lines 128 to 133
let query = format!(
"SELECT {} FROM {}{}",
column,
table_name.as_quoted_identifier(),
where_str
);
Copy link
Contributor

Choose a reason for hiding this comment

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

For queries like these we can use sqlx macros which are much nicer than manually formatting values into a string. Another minor point is about using uppercase in SQL: we use lowercase as a convention for SQL at supabase, but not a big deal here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh nice, good to know! I was wondering why everything was lowercase.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Regarding sqlx, we can definitely unify things for later, I initially had a split database and everything just to not have any sqlx internals within pg_replicate but the ergonomics of sqlx are definitely better, will create a ticket.

@iambriccardo iambriccardo merged commit 9d51d25 into main May 21, 2025
8 checks passed
@iambriccardo iambriccardo deleted the riccardo/feat/add-integration-test branch May 21, 2025 12:20
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants