Enhanced test coverage for database interactions with diverse key configurations.#1724
Conversation
📝 WalkthroughWalkthroughThe change adds test utility functions and types for creating PostgreSQL test tables with various primary key schemas. It includes factory functions for simple keys (auto-increment, UUID, date) and composite keys (UUID, UUID+integer, date+integer), along with metadata types capturing table configuration. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR expands the Postgres E2E test suite by adding new table fixtures covering additional primary-key / foreign-key type combinations (UUID, DATE, auto-increment integer, and composite variants) and adding many new endpoint assertions that exercise filtering and CRUD behavior across those schemas.
Changes:
- Added typed table-creation helpers for simple and composite primary key configurations (UUID/DATE/INTEGER).
- Extended the Postgres “complex table” E2E test suite with many new cases for
/table/rows/find,/table/row, and/table/structureacross the new schemas. - Added auto-increment PK + FK scenarios and corresponding CRUD tests.
Reviewed changes
Copilot reviewed 1 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| backend/test/utils/test-utilities/create-test-postgres-tables.ts | Adds new fixture builders returning metadata + sample PK values/rows for UUID/DATE/auto-increment/composite key tables. |
| backend/test/ava-tests/complex-table-tests/complex-postgres-table-e2e.test.ts | Adds extensive new E2E coverage for filters and CRUD on the new typed-key tables. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const insertedProductIds = await knex(mainTableName).insert(mainTableInserts).returning('product_id'); | ||
| const productIds = insertedProductIds.map((r: any) => (typeof r === 'object' ? r.product_id : r)); |
There was a problem hiding this comment.
insert(...).returning('product_id') is assumed to return one id per inserted row, but for some dialects (notably MySQL) Knex returns only the first insert id (or ignores returning). That would make productIds[i] undefined for most rows and can cause the subsequent insert into ProductReviews_AutoInc_Simple to violate the NOT NULL FK. Consider either (a) avoiding returning here by inserting deterministic product_id values, or (b) inserting rows one-by-one / querying back the inserted ids in a dialect-safe way, or (c) branching by connectionParams.type similarly to other helpers.
| const insertedProductIds = await knex(mainTableName).insert(mainTableInserts).returning('product_id'); | |
| const productIds = insertedProductIds.map((r: any) => (typeof r === 'object' ? r.product_id : r)); | |
| const productIds: number[] = []; | |
| for (const mainTableInsert of mainTableInserts) { | |
| const insertedProductId = await knex(mainTableName).insert(mainTableInsert).returning('product_id'); | |
| const productId = Array.isArray(insertedProductId) | |
| ? typeof insertedProductId[0] === 'object' | |
| ? insertedProductId[0].product_id | |
| : insertedProductId[0] | |
| : insertedProductId; | |
| productIds.push(productId); | |
| } |
| t.is(findRowsResponse.status, 200); | ||
| const findRowsRO = JSON.parse(findRowsResponse.text); | ||
| t.truthy(findRowsRO.rows); | ||
| for (const row of findRowsRO.rows) { | ||
| t.truthy(row.status.startsWith('P')); | ||
| } |
There was a problem hiding this comment.
This test can pass even if the startswith filter is broken: findRowsRO.rows is always truthy (even when empty), and the loop does nothing on an empty result set. Also, because the seeded status values are randomized, it’s possible (though rare) for the filter to return 0 rows, making the test non-deterministic. To make this test meaningful and non-flaky, assert rows.length > 0 and/or seed at least one row with a known matching status (e.g., insert a row with status: 'Pending' before calling the endpoint).
|
|
||
| t.is(findRowsResponse.status, 200); | ||
| const findRowsRO = JSON.parse(findRowsResponse.text); | ||
| t.truthy(findRowsRO.rows); |
There was a problem hiding this comment.
Same issue as the startswith test: if contains filtering is broken and the endpoint returns an empty array, this test still passes because it doesn’t assert rows.length > 0. Since the seeded status values are randomized, an empty result is also (rarely) possible, making the test less reliable. Consider asserting rows.length > 0 and/or inserting a deterministic row that should match contains: 'end' before querying.
| t.truthy(findRowsRO.rows); | |
| t.truthy(findRowsRO.rows); | |
| t.truthy(findRowsRO.rows.length > 0); |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
backend/test/utils/test-utilities/create-test-postgres-tables.ts (1)
4-34: Tighten the exported contracts instead of leakingany.The new public result shapes and factory signatures expose
any, and the object shapes are declared astypealiases. Please use interfaces for these shapes and a narrow union for primary-key sample values / connection params.♻️ Suggested shape cleanup
+export type PrimaryKeyType = 'uuid' | 'date' | 'integer'; +export type PrimaryKeySampleValue = string | number; + +export interface CompositePrimaryKeySampleRow { + [columnName: string]: PrimaryKeySampleValue; +} + +export interface TestConnectionParams { + type?: string; + [key: string]: unknown; +} + -export type SimpleTypedKeyTableCreationResult = { +export interface SimpleTypedKeyTableCreationResult { main_table: { table_name: string; column_names: string[]; primary_key_column_names: string[]; - primary_key_type: 'uuid' | 'date' | 'integer'; - sample_primary_key_values: any[]; + primary_key_type: PrimaryKeyType; + sample_primary_key_values: PrimaryKeySampleValue[]; }; first_referenced_table: { table_name: string; column_names: string[]; primary_key_column_names: string[]; foreign_key_column_names: string[]; }; -}; +} -export type CompositeTypedKeyTableCreationResult = { +export interface CompositeTypedKeyTableCreationResult { main_table: { table_name: string; column_names: string[]; primary_key_column_names: string[]; - primary_key_types: Array<'uuid' | 'date' | 'integer'>; - sample_primary_key_rows: Array<Record<string, any>>; + primary_key_types: PrimaryKeyType[]; + sample_primary_key_rows: CompositePrimaryKeySampleRow[]; }; first_referenced_table: { table_name: string; column_names: string[]; primary_key_column_names: string[]; foreign_key_column_names: string[]; }; -}; +}Then use
connectionParams: TestConnectionParamsin the new exported helpers and replace(r: any)with a narrow returned-id type.As per coding guidelines, “Always add type annotations to function parameters and return types in TypeScript”, “Avoid any types - use specific types or generics instead”, and “Use interfaces for object shapes and type for unions and primitives”.
Also applies to: 333-335, 411-413, 487-489, 560-562, 648-650, 734-736, 374-375
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/test/utils/test-utilities/create-test-postgres-tables.ts` around lines 4 - 34, The exported result shapes SimpleTypedKeyTableCreationResult and CompositeTypedKeyTableCreationResult currently leak any and use type aliases; change them to interfaces, replace fields like sample_primary_key_values and sample_primary_key_rows with a narrow union type for primary key values/rows (e.g., id values as 'uuid' | 'date' | 'integer' union or a typed Record<string, PrimaryKeyValue>), update all factory/function signatures to use a concrete TestConnectionParams type for connectionParams, and replace occurrences of (r: any) with the appropriate returned-id type (e.g., GeneratedId | GeneratedRow) so no any is exported; update referenced helpers and usages (look for functions that return SimpleTypedKeyTableCreationResult / CompositeTypedKeyTableCreationResult and parameters named connectionParams) to use the new interfaces and unions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/test/utils/test-utilities/create-test-postgres-tables.ts`:
- Around line 337-360: The helper mistakenly normalizes MySQL by mapping
connectionParams.type 'mysql' → 'mysql2' but then uses Postgres-only semantics
when collecting inserted IDs via batch .returning('product_id') (see
connectionParams/connectionParamsCopy, getTestKnex, and the batch insert that
fills productIds), which breaks MySQL ID collection; remove the normalization
and mark this utility as Postgres-only (or assert/throw if connectionParams.type
!== 'pg') so callers know it's Postgres-only, and keep the Postgres-style batch
.returning usage unchanged; alternatively, if you want cross-dialect support,
replace the batch .insert(...).returning(...) with a dialect-safe ID collection
(do per-row inserts and push each insert result into productIds) but prefer
removing the mysql->mysql2 mapping and documenting/asserting Postgres-only in
this helper.
---
Nitpick comments:
In `@backend/test/utils/test-utilities/create-test-postgres-tables.ts`:
- Around line 4-34: The exported result shapes SimpleTypedKeyTableCreationResult
and CompositeTypedKeyTableCreationResult currently leak any and use type
aliases; change them to interfaces, replace fields like
sample_primary_key_values and sample_primary_key_rows with a narrow union type
for primary key values/rows (e.g., id values as 'uuid' | 'date' | 'integer'
union or a typed Record<string, PrimaryKeyValue>), update all factory/function
signatures to use a concrete TestConnectionParams type for connectionParams, and
replace occurrences of (r: any) with the appropriate returned-id type (e.g.,
GeneratedId | GeneratedRow) so no any is exported; update referenced helpers and
usages (look for functions that return SimpleTypedKeyTableCreationResult /
CompositeTypedKeyTableCreationResult and parameters named connectionParams) to
use the new interfaces and unions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 83d90cfd-38c2-4825-8dd5-c43a598e04c7
📒 Files selected for processing (2)
backend/test/ava-tests/complex-table-tests/complex-postgres-table-e2e.test.tsbackend/test/utils/test-utilities/create-test-postgres-tables.ts
| if (connectionParams.type === 'mysql') { | ||
| connectionParamsCopy.type = 'mysql2'; | ||
| } | ||
| const knex = getTestKnex(connectionParamsCopy); | ||
|
|
||
| const mainTableName = 'Products_AutoInc_Simple'; | ||
| const firstReferencedTableName = 'ProductReviews_AutoInc_Simple'; | ||
|
|
||
| await knex.schema.dropTableIfExists(firstReferencedTableName); | ||
| await knex.schema.dropTableIfExists(mainTableName); | ||
|
|
||
| await knex.schema.createTable(mainTableName, (table) => { | ||
| table.increments('product_id').primary(); | ||
| table.string('product_name', 100); | ||
| table.string('sku', 50); | ||
| table.decimal('price', 10, 2); | ||
| }); | ||
|
|
||
| await knex.schema.createTable(firstReferencedTableName, (table) => { | ||
| table.increments('review_id').primary(); | ||
| table.integer('product_id').notNullable(); | ||
| table.integer('rating'); | ||
| table.text('comment'); | ||
| table.foreign('product_id').references('product_id').inTable(mainTableName); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify whether the new auto-increment helper is intended to run with MySQL/mysql2.
# Expected result: if MySQL connections call this helper, the helper needs dialect-safe id collection
# and matching FK column types; if not, the helper should reject/narrow to Postgres-only.
rg -n -C3 'createTestTablesWithSimpleAutoIncrementKeys\s*\('
rg -n -C3 'type:\s*[\"\047]mysql[\"\047]|mysql2'Repository: rocket-admin/rocketadmin
Length of output: 1195
🏁 Script executed:
#!/bin/bash
# First, get the full function implementation
sed -n '320,395p' backend/test/utils/test-utilities/create-test-postgres-tables.ts | cat -nRepository: rocket-admin/rocketadmin
Length of output: 3228
🏁 Script executed:
#!/bin/bash
# Find all callers of createTestTablesWithSimpleAutoIncrementKeys
rg -n 'createTestTablesWithSimpleAutoIncrementKeys' --type tsRepository: rocket-admin/rocketadmin
Length of output: 513
🏁 Script executed:
#!/bin/bash
# Check what connection types are used in test files
rg -n "type:\s*['\"]mysql['\"]|type:\s*['\"]postgres['\"]" backend/test --type ts | head -20Repository: rocket-admin/rocketadmin
Length of output: 284
🏁 Script executed:
#!/bin/bash
# Check what connectionToTestDB is in the test file
sed -n '1,100p' backend/test/ava-tests/complex-table-tests/complex-postgres-table-e2e.test.ts | cat -nRepository: rocket-admin/rocketadmin
Length of output: 5046
🏁 Script executed:
#!/bin/bash
# Check if there are any MySQL test files that use this helper
fd -t f -e test.ts . backend/test/ava-tests | xargs rg -l 'mysql|MySQL' | head -10Repository: rocket-admin/rocketadmin
Length of output: 452
🏁 Script executed:
#!/bin/bash
# Look for all test files that might call createTestTablesWithSimpleAutoIncrementKeys
rg -l 'createTestTablesWithSimpleAutoIncrementKeys' backend/testRepository: rocket-admin/rocketadmin
Length of output: 210
🏁 Script executed:
#!/bin/bash
# Check if other helper functions in the same file also have mysql normalization
rg -n "mysql2|connectionParams.type === 'mysql'" backend/test/utils/test-utilities/create-test-postgres-tables.tsRepository: rocket-admin/rocketadmin
Length of output: 785
🏁 Script executed:
#!/bin/bash
# Get list of all exported helper functions in this file
rg -n "^export const create" backend/test/utils/test-utilities/create-test-postgres-tables.tsRepository: rocket-admin/rocketadmin
Length of output: 637
🏁 Script executed:
#!/bin/bash
# Check the first few helper functions to see if they also have mysql normalization
sed -n '1,150p' backend/test/utils/test-utilities/create-test-postgres-tables.ts | grep -A 15 "export const create"Repository: rocket-admin/rocketadmin
Length of output: 621
🏁 Script executed:
#!/bin/bash
# Check if any MySQL test files import these helper functions
rg -l "createTestTablesWithSimpleAutoIncrementKeys|from.*create-test-postgres-tables" backend/test/ava-tests --type tsRepository: rocket-admin/rocketadmin
Length of output: 296
🏁 Script executed:
#!/bin/bash
# Look at the beginning of a MySQL test file to see what helpers it uses
sed -n '1,50p' backend/test/ava-tests/saas-tests/table-mysql-e2e.test.ts | grep -A 5 -B 5 "import.*create"Repository: rocket-admin/rocketadmin
Length of output: 771
🏁 Script executed:
#!/bin/bash
# Check if mysql test files use any helpers from create-test-postgres-tables.ts
rg "import.*from.*create-test-postgres-tables" backend/test/ava-tests/saas-tests/table-mysql-e2e.test.tsRepository: rocket-admin/rocketadmin
Length of output: 50
🏁 Script executed:
#!/bin/bash
# Check what the second location mentioned in the review is (lines 374-389)
sed -n '370,395p' backend/test/utils/test-utilities/create-test-postgres-tables.ts | cat -nRepository: rocket-admin/rocketadmin
Length of output: 1113
Remove the MySQL normalization from this helper—it's postgres-only and the code lacks dialect-safe id collection.
This function declares MySQL support via the mysql → mysql2 normalization but actually assumes PostgreSQL semantics for multi-row .returning('product_id') at line 355 (batch insert). PostgreSQL returns every inserted row; MySQL-style clients return only the first generated ID, leaving productIds[i] undefined for review rows 2+. The helper is never called with MySQL connections in the test suite (only Postgres, Oracle, and MSSQL tests use it), and MySQL tests use different utilities entirely.
Either:
- Remove the normalization and document this as Postgres-only, or
- Collect IDs dialect-safely per the suggested pattern (loop single inserts for small fixture sizes).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/test/utils/test-utilities/create-test-postgres-tables.ts` around
lines 337 - 360, The helper mistakenly normalizes MySQL by mapping
connectionParams.type 'mysql' → 'mysql2' but then uses Postgres-only semantics
when collecting inserted IDs via batch .returning('product_id') (see
connectionParams/connectionParamsCopy, getTestKnex, and the batch insert that
fills productIds), which breaks MySQL ID collection; remove the normalization
and mark this utility as Postgres-only (or assert/throw if connectionParams.type
!== 'pg') so callers know it's Postgres-only, and keep the Postgres-style batch
.returning usage unchanged; alternatively, if you want cross-dialect support,
replace the batch .insert(...).returning(...) with a dialect-safe ID collection
(do per-row inserts and push each insert result into productIds) but prefer
removing the mysql->mysql2 mapping and documenting/asserting Postgres-only in
this helper.
Summary by CodeRabbit