fix(check): integration tests cleanup failure #2276
Conversation
Summary of ChangesHello @rahulpinto19, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a fix primarily focused on enhancing the reliability and thoroughness of PostgreSQL test environment cleanup. It streamlines the process by adopting a more aggressive schema reset mechanism and removes an apparently unused or deprecated tool configuration. These changes aim to prevent test flakiness and ensure consistent database states across test runs. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request appears to remove the postgres-list-stored-procedure tool and refactors the CleanupPostgresTables test helper function. The changes to remove the tool seem correct based on the provided diff. However, the new implementation of CleanupPostgresTables can be improved. Specifically, it leaves behind a large block of commented-out code that should be removed, and the new logic for cleaning the database schema could be made more robust by executing SQL statements individually for better error handling.
tests/common.go
Outdated
| // query := ` | ||
| // SELECT table_name FROM information_schema.tables | ||
| // WHERE table_schema = 'public' AND table_type = 'BASE TABLE';` | ||
|
|
||
| // rows, err := pool.Query(ctx, query) | ||
| // if err != nil { | ||
| // t.Fatalf("Failed to query for all tables in 'public' schema: %v", err) | ||
| // } | ||
| // defer rows.Close() | ||
|
|
||
| // var tablesToDrop []string | ||
| // for rows.Next() { | ||
| // var tableName string | ||
| // if err := rows.Scan(&tableName); err != nil { | ||
| // t.Errorf("Failed to scan table name: %v", err) | ||
| // continue | ||
| // } | ||
| // tablesToDrop = append(tablesToDrop, fmt.Sprintf("public.%q", tableName)) | ||
| // } | ||
|
|
||
| // if len(tablesToDrop) == 0 { | ||
| // return | ||
| // } | ||
|
|
||
| // dropQuery := fmt.Sprintf("DROP TABLE IF EXISTS %s CASCADE;", strings.Join(tablesToDrop, ", ")) | ||
|
|
||
| // if _, err := pool.Exec(ctx, dropQuery); err != nil { | ||
| // t.Fatalf("Failed to drop all tables in 'public' schema: %v", err) | ||
| // } |
There was a problem hiding this comment.
This large block of commented-out code from the previous implementation should be removed to improve readability. Git history can be used to review the previous logic if needed.
tests/common.go
Outdated
| // 1. Drop the entire public schema (this kills tables, views, types, etc.) | ||
| dropSchema := "DROP SCHEMA public CASCADE;" | ||
| // 2. Recreate the empty public schema | ||
| createSchema := "CREATE SCHEMA public;" | ||
| // 3. Grant permissions back (Postgres default) | ||
| grantPublic := "GRANT ALL ON SCHEMA public TO public;" | ||
|
|
||
| _, err := pool.Exec(ctx, dropSchema + createSchema + grantPublic) | ||
| if err != nil { | ||
| t.Fatalf("Failed to nuclear-wipe the public schema: %v", err) | ||
| } |
There was a problem hiding this comment.
While pgx can execute multiple statements in a single Exec call, it's better practice to execute each statement separately. This improves error handling by pinpointing which statement failed and makes the code easier to read and debug.
// 1. Drop the entire public schema (this kills tables, views, types, etc.)
if _, err := pool.Exec(ctx, "DROP SCHEMA public CASCADE;"); err != nil {
t.Fatalf("Failed to drop public schema: %v", err)
}
// 2. Recreate the empty public schema
if _, err := pool.Exec(ctx, "CREATE SCHEMA public;"); err != nil {
t.Fatalf("Failed to create public schema: %v", err)
}
// 3. Grant permissions back (Postgres default)
if _, err := pool.Exec(ctx, "GRANT ALL ON SCHEMA public TO public;"); err != nil {
t.Fatalf("Failed to grant permissions on public schema: %v", err)
}
rahulpinto19
force-pushed
the
integration-test-cleanup-testing
branch
from
d2be5f5 to
1bfe394
Compare
2 similar comments
rahulpinto19
changed the title
|
🧨 Preview deployments removed. |
rahulpinto19
force-pushed
the
integration-test-cleanup-testing
branch
from
5c19e20 to
5d99b7e
Compare
1 participant
Description
PR Checklist
CONTRIBUTING.md
bug/issue
before writing your code! That way we can discuss the change, evaluate
designs, and agree on the general idea
!if this involve a breaking change🛠️ Fixes #<issue_number_goes_here>