Skip to content

Feature/integration tests 7#21

Merged
mikkeldamsgaard merged 8 commits intomainfrom
feature/integration-tests-7
Feb 25, 2026
Merged

Feature/integration tests 7#21
mikkeldamsgaard merged 8 commits intomainfrom
feature/integration-tests-7

Conversation

@mikkeldamsgaard
Copy link
Contributor

No description provided.

Copilot AI review requested due to automatic review settings February 25, 2026 07:34
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Adds an end-to-end integration test suite (Docker Compose + GitHub Actions services) and extends the seed engine to better support reset/id generation during seeding.

Changes:

  • Introduces Docker-backed integration tests covering wait-for, render, fetch, exec, and seed workflows against real Postgres/MySQL/nginx.
  • Adds CI workflow to run the integration suite with service containers, plus test inputs/specs under tests/input/.
  • Updates seed execution to support phase-level reset and passes an auto_id column hint through to DB insert_row.

Reviewed changes

Copilot reviewed 17 out of 17 changed files in this pull request and generated 6 comments.

Show a summary per file
File Description
tests/integration_test.rs New Docker-dependent integration suite invoking the built initium binary and validating behavior against live services
tests/input/template.conf.tmpl Template fixture for render integration test
tests/input/seed-postgres.yaml Postgres seeding fixture with refs + unique keys
tests/input/seed-mysql.yaml MySQL seeding fixture with refs + unique keys
tests/input/create-schema-postgres.yaml Seed spec fixture for schema creation scenarios
tests/input/create-db-postgres.yaml Seed spec fixture for Postgres database creation scenario
tests/input/create-db-mysql.yaml Seed spec fixture for MySQL database creation scenario
tests/input/create-nonexistent-db-*-postgres.yaml Seed spec fixtures for “create DB if missing” Postgres cases
tests/input/create-nonexistent-db-*-mysql.yaml Seed spec fixtures for “create DB if missing” MySQL cases
tests/docker-compose.yml Local developer Compose stack for integration tests
tests/README.md Documentation for running unit vs integration tests and scenario list
src/seed/executor.rs Moves reset behavior to phase level and threads auto_id column into inserts
src/seed/db.rs Extends Database::insert_row signature; updates Postgres insert/exists logic
CHANGELOG.md Documents new integration suite and related seed behavior
.github/workflows/integration.yml Adds a dedicated workflow to run integration tests with service containers

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

src/seed/db.rs Outdated
Comment on lines 321 to 335
let value_list: Vec<String> = values.iter().map(|v| escape_sql_value(v)).collect();
let returning_col = sanitize_identifier(auto_id_column.unwrap_or("id"));
let sql = format!(
"INSERT INTO \"{}\" ({}) VALUES ({}) RETURNING COALESCE(CAST(\"{}\" AS BIGINT), 0)",
sanitize_identifier(table),
col_list.join(", "),
placeholders.join(", "),
sanitize_identifier(columns.first().map(|s| s.as_str()).unwrap_or("id"))
value_list.join(", "),
returning_col
);
let params: Vec<&(dyn postgres::types::ToSql + Sync)> = values
.iter()
.map(|v| v as &(dyn postgres::types::ToSql + Sync))
.collect();
let row = self
.client
.query_one(&sql, params.as_slice())
.query_one(&sql, &[])
.map_err(|e| format!("inserting row into '{}': {}", table, e))?;
let id: i64 = row.get(0);
Ok(Some(id))
Copy link

Copilot AI Feb 25, 2026

Choose a reason for hiding this comment

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

PostgresDb::insert_row always adds a RETURNING ... CAST(... AS BIGINT) clause and defaults auto_id_column to "id". If a seed spec omits auto_id (valid per schema) or the table’s PK isn’t a BIGINT-compatible column named id, this will fail even though callers may not need a generated id. Consider only using RETURNING when auto_id_column is Some(_) (or when a row _ref requires it) and otherwise execute a plain INSERT and return Ok(None).

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed: insert_row already only uses RETURNING when auto_id_column is Some(_). The current code correctly returns Ok(None) for plain INSERTs without auto_id.

Comment on lines +321 to 360
let value_list: Vec<String> = values.iter().map(|v| escape_sql_value(v)).collect();
let returning_col = sanitize_identifier(auto_id_column.unwrap_or("id"));
let sql = format!(
"INSERT INTO \"{}\" ({}) VALUES ({}) RETURNING COALESCE(CAST(\"{}\" AS BIGINT), 0)",
sanitize_identifier(table),
col_list.join(", "),
placeholders.join(", "),
sanitize_identifier(columns.first().map(|s| s.as_str()).unwrap_or("id"))
value_list.join(", "),
returning_col
);
let params: Vec<&(dyn postgres::types::ToSql + Sync)> = values
.iter()
.map(|v| v as &(dyn postgres::types::ToSql + Sync))
.collect();
let row = self
.client
.query_one(&sql, params.as_slice())
.query_one(&sql, &[])
.map_err(|e| format!("inserting row into '{}': {}", table, e))?;
let id: i64 = row.get(0);
Ok(Some(id))
}

fn row_exists(
&mut self,
table: &str,
unique_columns: &[String],
unique_values: &[String],
) -> Result<bool, String> {
if unique_columns.is_empty() {
return Ok(false);
}
let conditions: Vec<String> = unique_columns
.iter()
.enumerate()
.map(|(i, c)| format!("\"{}\" = ${}", sanitize_identifier(c), i + 1))
.zip(unique_values.iter())
.map(|(c, v)| format!("\"{}\" = {}", sanitize_identifier(c), escape_sql_value(v)))
.collect();
let sql = format!(
"SELECT COUNT(*) FROM \"{}\" WHERE {}",
sanitize_identifier(table),
conditions.join(" AND ")
);
let params: Vec<&(dyn postgres::types::ToSql + Sync)> = unique_values
.iter()
.map(|v| v as &(dyn postgres::types::ToSql + Sync))
.collect();
let row = self
.client
.query_one(&sql, params.as_slice())
.query_one(&sql, &[])
.map_err(|e| format!("checking row existence in '{}': {}", table, e))?;
Copy link

Copilot AI Feb 25, 2026

Choose a reason for hiding this comment

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

PostgresDb::insert_row/row_exists switched from parameterized queries to interpolating escaped literals and calling query_one(..., &[]). This loses type-safe binding (e.g., NULL vs empty string), can cause subtle casting issues, and prevents query plan reuse. Prefer keeping $1..$n placeholders with a params slice, and only sanitize identifiers (table/column names).

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Acknowledged. Parameterized queries with $N placeholders send values as TEXT type, which postgres cannot implicitly cast to other column types (e.g. INTEGER). Since seed values are untyped strings targeting arbitrary columns, we must use escaped string literals (type unknown) which postgres auto-casts. Added a comment in the code explaining this rationale. Identifiers remain sanitized.

Comment on lines +26 to +43
const PG_URL: &str = "postgres://initium:initium@localhost:15432/initium_test";
const MYSQL_URL_STR: &str = "mysql://initium:initium@localhost:13306/initium_test";
const MYSQL_ROOT_URL_STR: &str = "mysql://root:rootpass@localhost:13306/initium_test";

fn pg_client() -> postgres::Client {
postgres::Client::connect(PG_URL, postgres::NoTls).expect("failed to connect to postgres")
}

fn mysql_conn() -> mysql::PooledConn {
let pool = mysql::Pool::new(MYSQL_URL_STR).expect("failed to connect to mysql");
pool.get_conn().expect("failed to get mysql connection")
}

fn mysql_root_conn() -> mysql::PooledConn {
let pool = mysql::Pool::new(MYSQL_ROOT_URL_STR).expect("failed to connect to mysql as root");
pool.get_conn()
.expect("failed to get mysql root connection")
}
Copy link

Copilot AI Feb 25, 2026

Choose a reason for hiding this comment

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

These integration tests directly reference the optional postgres and mysql crates/types. Because the project supports --no-default-features builds (e.g. --features sqlite), this test target will fail to compile when the DB driver features are disabled. Consider adding #[cfg(feature = "postgres")] / #[cfg(feature = "mysql")] guards (or splitting tests per feature) so non-default feature test runs can still compile.

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed: added #[cfg(feature = "postgres")] and #[cfg(feature = "mysql")] guards to all database-specific integration tests, helper functions, and constants.

tests/README.md Outdated
| `test_waitfor_tcp_postgres` | wait-for TCP against Postgres succeeds |
| `test_waitfor_tcp_mysql` | wait-for TCP against MySQL succeeds |
| `test_waitfor_http_server` | wait-for HTTP against nginx returns 200 |
| `test_waitfor_nonexistent_service` | wait-for against closed port fails with exit code 1 |
Copy link

Copilot AI Feb 25, 2026

Choose a reason for hiding this comment

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

The test list references test_waitfor_nonexistent_service, but the actual integration test function is named test_waitfor_nonexistent_service_timeout. Update the documentation to match the real test name (or rename the test) so readers can run/locate it easily.

Suggested change
| `test_waitfor_nonexistent_service` | wait-for against closed port fails with exit code 1 |
| `test_waitfor_nonexistent_service_timeout` | wait-for against closed port fails with exit code 1 |

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed: updated README to use the correct test name test_waitfor_nonexistent_service_timeout.

CHANGELOG.md Outdated
## [Unreleased]

### Changed
- Removed placeholder `seed_sets` from create-if-missing integration test YAML files; phases with only `create_if_missing` no longer need empty seed sets (`seed_sets` was already optional via `#[serde(default)]`)
Copy link

Copilot AI Feb 25, 2026

Choose a reason for hiding this comment

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

This changelog entry says placeholder seed_sets were removed from create-if-missing integration test YAML files, but in this PR those YAML specs are being added (and some create-if-missing specs still include placeholder seed_sets). Consider rewording this to describe what actually changed in this PR (e.g., that create-if-missing phases can omit seed_sets) or adjust the YAML files to match the statement.

Suggested change
- Removed placeholder `seed_sets` from create-if-missing integration test YAML files; phases with only `create_if_missing` no longer need empty seed sets (`seed_sets` was already optional via `#[serde(default)]`)
- Clarified in integration test YAML specs that phases with only `create_if_missing` no longer need an explicit (possibly empty) `seed_sets` field, and added coverage for both with- and without-`seed_sets` configurations (`seed_sets` was already optional via `#[serde(default)]`)

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed: reworded CHANGELOG entry to accurately describe that seed phases with only create_if_missing can omit the seed_sets field entirely.

@mikkeldamsgaard mikkeldamsgaard force-pushed the feature/integration-tests-7 branch from ca62422 to 7cf446e Compare February 25, 2026 08:02
mikkeldamsgaard and others added 3 commits February 25, 2026 12:52
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
The insert_row function always used RETURNING COALESCE(CAST(id AS BIGINT), 0)
even when no auto_id_column was specified, defaulting to the id column.
For tables with text primary keys (like NetBird's accounts, users, groups),
CAST to BIGINT fails with a generic db error.
Fix: Only use RETURNING clause when auto_id_column is explicitly set.
When no auto_id is needed, use a simple INSERT without RETURNING.
…, add cfg feature guards, fix README test name, document escaped literals rationale, reword CHANGELOG
@mikkeldamsgaard mikkeldamsgaard merged commit d2758e5 into main Feb 25, 2026
5 checks passed
@mikkeldamsgaard mikkeldamsgaard deleted the feature/integration-tests-7 branch February 26, 2026 19:01
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.

2 participants