HYPERFLEET-302 - Add dynamic table discovery for test cleanup #32
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
WalkthroughThe test database cleanup was changed from a static list of tables to dynamic discovery and dependency-aware ordering. New helpers query PostgreSQL's information_schema to list public tables, fetch foreign-key dependencies for each table, detect cycles, and perform a DFS-based topological sort. CleanDB now drops tables in the computed safe order (skipping configured system tables and non-existent tables) and surfaces errors from discovery, ordering, or drop operations. GORM was added for DB interactions. Sequence Diagram(s)sequenceDiagram
participant TestHelper
participant GORM
participant Postgres
TestHelper->>GORM: Open connection
TestHelper->>GORM: getAllTables()
GORM->>Postgres: query pg_tables (exclude system)
Postgres-->>GORM: list of tables
GORM-->>TestHelper: tables
loop for each table
TestHelper->>GORM: getTableDependencies(table)
GORM->>Postgres: query information_schema for FK refs
Postgres-->>GORM: referenced tables
GORM-->>TestHelper: dependencies
end
TestHelper->>TestHelper: orderTablesByDependencies(tables, deps)
TestHelper->>GORM: drop table in safe order (IF EXISTS ... CASCADE)
GORM->>Postgres: execute DROP TABLE
Postgres-->>GORM: result
GORM-->>TestHelper: success / error
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (1)**/*.go📄 CodeRabbit inference engine (AGENTS.md)
Files:
🧠 Learnings (8)📓 Common learnings📚 Learning: 2026-01-08T01:23:20.634ZApplied to files:
📚 Learning: 2026-01-08T01:23:20.634ZApplied to files:
📚 Learning: 2026-01-08T01:23:20.634ZApplied to files:
📚 Learning: 2026-01-08T01:23:20.634ZApplied to files:
📚 Learning: 2026-01-08T01:23:20.634ZApplied to files:
📚 Learning: 2026-01-08T01:23:20.634ZApplied to files:
📚 Learning: 2026-01-08T01:23:20.634ZApplied to files:
🔇 Additional comments (5)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @test/helper.go:
- Around line 384-432: orderTablesByDependencies builds dependency edges from
getTableDependencies but does not ignore excluded tables (e.g., "migrations"),
so extraneous tables can be added to the DFS and returned; change the function
to first build a set of input tables and when populating dependencies for each
table (and when traversing deps in visit) only include dependency names that
exist in that input set (i.e., filter deps returned by getTableDependencies
against the original tables slice/set), ensuring only tables present in the
original input are considered and preventing excluded tables from being
ordered/dropped.
- Around line 400-418: The DFS closure visit currently silently returns nil when
a cycle is detected (when visiting[table] is true); change this to surface the
issue by returning an error (e.g., fmt.Errorf("cycle detected at table %s",
table)) so callers see circular foreign-key references and the drop order isn't
silently wrong; update the visit closure (and any callers that expect an error)
to propagate this error and consider including context such as the current
dependency path (using visiting or a stack) for clearer diagnostics while
leaving visited and ordered logic intact.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/helper.go
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Use Go 1.24.9 with FIPS-compliant crypto enabled (CGO_ENABLED=1, GOEXPERIMENT=boringcrypto)
Use structured logging via logger.NewOCMLogger(ctx) with context-aware log fields including operation ID, account ID, and transaction ID
Files:
test/helper.go
🧠 Learnings (7)
📓 Common learnings
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-08T01:23:20.634Z
Learning: Applies to pkg/db/**/*.go : Use PostgreSQL 14.2 with GORM ORM for database operations
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-08T01:23:20.634Z
Learning: Applies to test/integration/**/*_test.go : Integration tests must use Testcontainers to create isolated PostgreSQL instances for each test suite to ensure no state leakage
📚 Learning: 2026-01-08T01:23:20.634Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-08T01:23:20.634Z
Learning: Applies to pkg/db/**/*.go : Use PostgreSQL 14.2 with GORM ORM for database operations
Applied to files:
test/helper.go
📚 Learning: 2026-01-08T01:23:20.634Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-08T01:23:20.634Z
Learning: Applies to pkg/{dao,db}/**/*.go : Use polymorphic status tables with owner_type + owner_id to support multiple resource types without creating separate status tables
Applied to files:
test/helper.go
📚 Learning: 2026-01-08T01:23:20.634Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-08T01:23:20.634Z
Learning: Applies to test/integration/**/*_test.go : Integration tests must use Testcontainers to create isolated PostgreSQL instances for each test suite to ensure no state leakage
Applied to files:
test/helper.go
📚 Learning: 2026-01-08T01:23:20.634Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-08T01:23:20.634Z
Learning: Applies to pkg/db/migrations/**/*.go : Store spec and conditions as JSONB in PostgreSQL to enable complex field queries
Applied to files:
test/helper.go
📚 Learning: 2026-01-08T01:23:20.634Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-08T01:23:20.634Z
Learning: Applies to cmd/hyperfleet/migrate/**/*.go : Use GORM AutoMigrate at startup via ./bin/hyperfleet-api migrate command; migrations are idempotent and track applied migrations
Applied to files:
test/helper.go
📚 Learning: 2026-01-08T01:23:20.634Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-08T01:23:20.634Z
Learning: Applies to pkg/api/**/*.go : Embed OpenAPI specification at compile time using Go 1.16+ //go:embed directive for self-contained binary deployment
Applied to files:
test/helper.go
🔇 Additional comments (4)
test/helper.go (4)
21-21: LGTM!The GORM import is necessary for the new helper methods that accept
*gorm.DBas a parameter. This is consistent with the project's established use of GORM ORM.
342-366: LGTM!The refactored
CleanDBfunction properly handles dynamic table discovery with dependency-aware ordering. The defensiveHasTablecheck beforeDropTableand consistent error handling pattern are good practices.
368-382: LGTM!The query correctly discovers user tables in the public schema while preserving the migrations table. This is PostgreSQL-specific but appropriate given the project uses PostgreSQL 14.2 per the coding guidelines.
434-454: LGTM!The query correctly retrieves foreign key dependencies from
information_schema. Using a parameterized query prevents SQL injection, andDISTINCThandles multi-column foreign keys appropriately.
Mischulee
force-pushed
the
HYPERFLEET-302
branch
from
a9c2f0f to
c960353
Compare
| return false | ||
| } | ||
|
|
||
| func (helper *Helper) getAllTables(g2 *gorm.DB) ([]string, error) { |
There was a problem hiding this comment.
Why not use helper.DBFactory creates sessions internally instead of pass the g2 variable?
There was a problem hiding this comment.
@yasun1 I think reusing same session for multiple related queries is more efficient than creating new one each time and it gives more control over the context
2 participants
https://issues.redhat.com/browse/HYPERFLEET-302
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.