test: integration-testing-new

[rahulpinto19]

issue: The Toolbox integration tests are not cleaning up test databases correctly, causing resource leaks

Steps Taken

Explicit Error Returns: SetupPostgresSQLTable now returns an error, allowing tests to fail immediately on setup issues.

Resilient Teardown: Cleanup logic now uses DROP TABLE IF EXISTS to prevent crashes during teardown.

Test Suite Updates: All relevant integration tests updated to handle setup errors and ensure proper resource cleanup.

[gemini-code-assist]

Summary of Changes

Hello @rahulpinto19, I'm Gemini Code Assist¹! 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 significantly enhances the reliability and error handling of PostgreSQL-based integration tests. By modifying the core table setup utility to propagate errors and updating all relevant test suites to properly handle these errors, the changes ensure that test failures due to setup issues are caught promptly and that database resources are cleaned up effectively, leading to more stable and predictable test runs.

Highlights

• Improved Test Setup Robustness: The SetupPostgresSQLTable function now returns an error alongside its teardown function, allowing calling test functions to detect and handle setup failures more explicitly.
• Enhanced Error Handling in Integration Tests: Integration tests for AlloyDB, Cloud SQL, and generic PostgreSQL have been updated to check for errors returned by SetupPostgresSQLTable and will now fail early if table setup encounters an issue.
• More Resilient Teardown Logic: The SetupPostgresSQLTable function's cleanup mechanism now uses DROP TABLE IF EXISTS, making the teardown process more robust by preventing errors if the table does not exist. It also ensures teardown is attempted even if an insertion error occurs during setup.

Using Gemini Code Assist

The 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 /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	/gemini help	Displays a list of available commands.

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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.

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

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[gemini-code-assist]

Code Review

This pull request refactors the test setup for Postgres tables to improve error handling, which is a good direction. However, the implementation introduces a few issues. There's a critical bug in tests/common.go where pool.Query is used instead of pool.Exec for DDL/DML, which can lead to connection leaks. Additionally, there's a critical bug in tests/alloydbpg/alloydb_pg_integration_test.go that results in a resource leak because a teardown function isn't called on success. Other test files have functionally correct but verbose and redundant error handling logic that can be simplified for better readability and maintainability. My review includes suggestions to fix these issues.

[gemini-code-assist]

The teardown function teardownTable1 is only deferred in the error case. If SetupPostgresSQLTable succeeds, the teardown function is never called, leading to a resource leak where the test table is not dropped. This can cause issues in subsequent test runs.

The defer statement should be placed to ensure cleanup happens in both success and partial failure scenarios.

	teardownTable1, err := tests.SetupPostgresSQLTable(t, ctx, pool, createParamTableStmt, insertParamTableStmt, tableNameParam, paramTestParams)
	if teardownTable1 != nil {
		defer teardownTable1(t)
	}
	if err != nil {
		t.Fatalf("Setup failed: %v", err)
	}

[gemini-code-assist]

Using pool.Query for CREATE and INSERT statements is incorrect and can lead to connection leaks. pool.Query is for queries that return rows (like SELECT), and the returned pgx.Rows object must be closed to release the connection back to the pool. For statements that don't return rows, pool.Exec should be used.

This applies to both the CREATE TABLE statement on line 624 and the INSERT statement on line 630.

	// Create table
	_, err = pool.Exec(ctx, createStatement)
	if err != nil {
		return nil, fmt.Errorf("unable to create test table %s: %w", tableName, err)
	}

	// Insert test data
	_, err = pool.Exec(ctx, insertStatement, params...)

[gemini-code-assist]

The error handling pattern here can be simplified. The teardownTable2 function is deferred in both the error case and the success case, which is redundant. A cleaner pattern is to defer the teardown if it exists, and then check for the error. This avoids code duplication and is easier to read.

	teardownTable2, err := tests.SetupPostgresSQLTable(t, ctx, pool, createAuthTableStmt, insertAuthTableStmt, tableNameAuth, authTestParams)
	if teardownTable2 != nil {
		defer teardownTable2(t)
	}
	if err != nil {
		t.Fatalf("Setup failed: %v", err)
	}

[gemini-code-assist]

The error handling pattern here can be simplified. The teardownTable1 function is deferred in both the error case and the success case, which is redundant. A cleaner pattern is to defer the teardown if it exists, and then check for the error. This avoids code duplication and is easier to read.

	teardownTable1, err := tests.SetupPostgresSQLTable(t, ctx, pool, createParamTableStmt, insertParamTableStmt, tableNameParam, paramTestParams)
	if teardownTable1 != nil {
		defer teardownTable1(t)
	}
	if err != nil {
		t.Fatalf("Setup failed: %v", err)
	}

[gemini-code-assist]

The error handling pattern here can be simplified. The teardownTable2 function is deferred in both the error case and the success case, which is redundant. A cleaner pattern is to defer the teardown if it exists, and then check for the error. This avoids code duplication and is easier to read.

	teardownTable2, err := tests.SetupPostgresSQLTable(t, ctx, pool, createAuthTableStmt, insertAuthTableStmt, tableNameAuth, authTestParams)
	if teardownTable2 != nil {
		defer teardownTable2(t)
	}
	if err != nil {
		t.Fatalf("Setup failed: %v", err)
	}

[gemini-code-assist]

The error handling pattern here can be simplified. The teardownTable1 function is deferred in both the error case and the success case, which is redundant. A cleaner pattern is to defer the teardown if it exists, and then check for the error. This avoids code duplication and is easier to read.

Also, the commented-out code on line 106 should be removed.

	teardownTable1, err := tests.SetupPostgresSQLTable(t, ctx, pool, createParamTableStmt, insertParamTableStmt, tableNameParam, paramTestParams)
	if teardownTable1 != nil {
		defer teardownTable1(t)
	}
	if err != nil {
		t.Fatalf("Setup failed: %v", err)
	}

[gemini-code-assist]

The error handling pattern here can be simplified. The teardownTable2 function is deferred in both the error case and the success case, which is redundant. A cleaner pattern is to defer the teardown if it exists, and then check for the error. This avoids code duplication and is easier to read.

	teardownTable2, err := tests.SetupPostgresSQLTable(t, ctx, pool, createAuthTableStmt, insertAuthTableStmt, tableNameAuth, authTestParams)
	if teardownTable2 != nil {
		defer teardownTable2(t)
	}
	if err != nil {
		t.Fatalf("Setup failed: %v", err)
	}