test(spanner): restrict pagination test to explicitly created database - #14043
Conversation
There was a problem hiding this comment.
Code Review
This pull request updates the testPagination method in ITBackupTest.java to filter backups by database name and asserts that the number of paginated backups matches the initial backup count. The reviewer identified a logic issue where the assertFalse(page.hasNextPage()) assertion prevents the pagination loop from executing or causes failures when multiple backups exist. Additionally, the reviewer recommended extracting the repeated database filter option into a local variable to improve readability.
0194748 to
114f3cd
Compare
The `ITBackupTest.testPagination` method previously listed all backups on the instance and asserted that only a single page of results would be returned (`assertFalse(page.hasNextPage())`). When running on shared integration test instances (`spanner.testenv.instance`), this assertion would fail if backups from other test runs were present on the instance.
This commit updates the pagination API calls to use `Options.filter("database:" + databaseId)`, ensuring that the test only paginates over the backups associated with the uniquely generated database for the active test run. This prevents the test from failing due to unrelated backups in the shared test environment.
114f3cd to
1a263f2
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request updates the testPagination method in ITBackupTest.java to accept a Database object and filter backups by the database ID. It also replaces the assertion on the number of backups with an exact match check against the initial backups size. A review comment points out a compilation error where database.getId().getName() is used instead of database.getId().getDatabase(), which needs to be resolved.
| private void testPagination(Database database) { | ||
| logger.info("Listing backups using pagination"); | ||
| Options.ListOption filter = | ||
| Options.filter(String.format("database:%s", database.getId().getName())); |
There was a problem hiding this comment.
The DatabaseId class does not have a getName() method. Calling database.getId().getName() will result in a compilation error. It should be replaced with database.getId().getDatabase() to correctly retrieve the database name.
| Options.filter(String.format("database:%s", database.getId().getName())); | |
| Options.filter(String.format("database:%s", database.getId().getDatabase())); |
There was a problem hiding this comment.
I double-checked this, and DatabaseId actually does have a getName() method. It retrieves the fully-qualified Spanner resource name (projects/%s/instances/%s/databases/%s), whereas getDatabase() only returns the short string ID.
Since the Options.filter("database:" + ...) API natively accepts the fully-qualified resource name, database.getId().getName() works perfectly here
The
ITBackupTest.testPaginationmethod previously listed all backups on the instance and asserted that only a single page of results would be returned (assertFalse(page.hasNextPage())). When running on shared integration test instances (spanner.testenv.instance), this assertion would fail if backups from other test runs were present on the instance.This commit updates the pagination API calls to use
Options.filter("database:" + databaseId), ensuring that the test only paginates over the backups associated with the uniquely generated database for the active test run. This prevents the test from failing due to unrelated backups in the shared test environment.