Add e2e tests for adding postgres datasources#78
Merged
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Summary of Changes
Hello @kapicic, 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 comprehensive end-to-end tests for the process of adding PostgreSQL data sources. It includes significant refactoring of the backend logic to handle database types more robustly by directly using the connection string, and enhances UI components with data-testid attributes to improve testability.
Highlights
- End-to-End Testing for PostgreSQL Data Sources: New Playwright tests (add-postgres-datasource-flow.spec.ts) have been added to validate the entire flow of adding PostgreSQL data sources, covering both postgresql:// and postgres:// connection string formats.
- Refactored Database Type Detection: The generateSqlQueries function and related API endpoints now accept a full connectionString instead of a generic databaseType. This change allows for more precise database type identification directly from the connection protocol, simplifying the logic and removing the need for AI-based type detection fallbacks.
- Enhanced UI Testability: Several UI components, including BaseSelect, IconButton, Menu, and SettingsButton, have been augmented with data-testid attributes. This provides stable selectors for automated testing, making the UI more robust and easier to test.
- Improved Logging: Additional logging has been introduced in the root layout to provide better visibility into session and root data during application initialization.
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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| 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 issue 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
-
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. ↩
Contributor
There was a problem hiding this comment.
Code Review
This pull request adds end-to-end tests for the PostgreSQL data source creation flow. To support this, data-testid attributes have been added to several UI components. A significant backend refactoring was also done to consistently use the connectionString for identifying data source types, which simplifies the logic.
My review focuses on potential security risks from logging sensitive data, and on improving the new e2e tests for better maintainability and robustness. I've suggested refactoring the tests to reduce duplication and using more stable selectors.
| let pluginAccess = FREE_PLUGIN_ACCESS; | ||
| let dataSourceTypes: any[] = []; | ||
|
|
||
| logger.info(JSON.stringify({ headersList, session, user }, null, 2)); |
Contributor
There was a problem hiding this comment.
Logging headers and session objects at the info level can pose a security risk by exposing sensitive data like session tokens in logs. It's recommended to either use the debug level for such detailed logging or remove these logs if they were for temporary debugging purposes.
Also, JSON.stringify(headersList) will produce an empty object {} because headersList is a Headers object, not a plain object. If you need to log the headers, you should convert them to an object first, for example: Object.fromEntries(headersList.entries()).
| export default async function RootLayout({ children }: { children: ReactNode }) { | ||
| const rootData = await getRootData(); | ||
|
|
||
| logger.info('Root data', JSON.stringify(rootData)); |
Contributor
There was a problem hiding this comment.
Logging the entire rootData object at the info level can expose sensitive user data and data source information in your logs. This should be avoided in production environments. Please consider changing this to debug level or removing it if it's for debugging.
Comment on lines
+3
to
+255
| test.describe('Add PostgreSQL Data Source Flow', () => { | ||
| test('Create PostgreSQL data source when connection string starts with postgresql', async ({ | ||
| page, | ||
| }: { | ||
| page: Page; | ||
| }) => { | ||
| test.setTimeout(60000); // 1 minute for this specific test | ||
|
|
||
| // Enable browser console logging for debugging | ||
| page.on('console', (msg: ConsoleMessage) => console.log('🖥️ Browser console:', msg.text())); | ||
| page.on('pageerror', (error: Error) => console.log('🖥️ Browser error:', error.message)); | ||
|
|
||
| console.log('Starting PostgreSQL data source creation test...'); | ||
|
|
||
| console.log('🧭 Navigating to application...'); | ||
| await page.goto('/'); | ||
|
|
||
| // Wait for the page to load | ||
| await page.waitForLoadState('networkidle'); | ||
| console.log('✅ Page loaded successfully'); | ||
|
|
||
| try { | ||
| console.log('🔍 Checking for telemetry consent page...'); | ||
|
|
||
| const telemetryHeading = page.locator('h1:has-text("Help us improve liblab ai")'); | ||
| await telemetryHeading.waitFor({ state: 'visible', timeout: 5000 }); | ||
| console.log('📋 Found telemetry consent page, clicking Decline...'); | ||
|
|
||
| const declineButton = page.locator('button:has-text("Decline")'); | ||
| await declineButton.waitFor({ state: 'visible' }); | ||
| await declineButton.click(); | ||
|
|
||
| await page.waitForLoadState('networkidle'); | ||
| console.log('✅ Declined telemetry, waiting for redirect...'); | ||
| } catch { | ||
| console.warn('ℹ️ No telemetry consent page found, continuing...'); | ||
| } | ||
|
|
||
| try { | ||
| console.log('🔍 Checking for data source connection page...'); | ||
|
|
||
| const dataSourceHeading = page.locator('h1:has-text("Let\'s connect your data source")'); | ||
| await dataSourceHeading.waitFor({ state: 'visible', timeout: 5000 }); | ||
| console.log('💾 Found data source connection page, connecting to sample database...'); | ||
|
|
||
| const connectButton = page.locator('button:has-text("Connect")'); | ||
| await connectButton.waitFor({ state: 'visible', timeout: 10000 }); | ||
| console.log('🔗 Found Connect button, clicking...'); | ||
| await connectButton.click(); | ||
|
|
||
| await page.waitForLoadState('networkidle'); | ||
| console.log('✅ Connected to sample database, waiting for redirect...'); | ||
| } catch { | ||
| console.warn('ℹ️ No data source connection page found, continuing...'); | ||
| } | ||
|
|
||
| console.log('🔍 Looking for settings cog icon...'); | ||
|
|
||
| await page.mouse.move(0, 500); | ||
| await page.mouse.down(); // Move mouse to ensure the menu is visible | ||
|
|
||
| const menu = page.locator('[data-testid="menu"]'); | ||
| await menu.waitFor({ state: 'attached', timeout: 10000 }); | ||
| await menu.hover(); | ||
|
|
||
| const settingsButton = page.locator('[data-testid="settings-button"]'); | ||
| await settingsButton.waitFor({ state: 'attached', timeout: 10000 }); | ||
| await settingsButton.click(); | ||
|
|
||
| await page.getByRole('button', { name: 'Add Data Source' }).click(); | ||
|
|
||
| // Look for the database type selector | ||
| console.log('🔍 Looking for database type selector...'); | ||
|
|
||
| // help here | ||
| const dbTypeSelector = page.locator('select, [data-testid="add-data-source-select"]'); | ||
| await dbTypeSelector.waitFor({ state: 'visible', timeout: 10000 }); | ||
| console.log('✅ Found database type selector, selecting PostgreSQL...'); | ||
|
|
||
| // Click to open the dropdown and select PostgreSQL | ||
| await dbTypeSelector.click(); | ||
|
|
||
| await page.waitForLoadState('domcontentloaded'); | ||
|
|
||
| const postgresOption = page.locator('[id="postgres"]'); | ||
| await postgresOption.waitFor({ state: 'visible', timeout: 5000 }); | ||
| await postgresOption.click(); | ||
| console.log('✅ Selected PostgreSQL database type'); | ||
|
|
||
| console.log('🔍 Looking for database name input...'); | ||
|
|
||
| const dbNameInput = page.locator( | ||
| 'input[placeholder*="database name"], input[placeholder*="Database Name"], input[name*="dbName"], input[name*="name"]', | ||
| ); | ||
| await dbNameInput.waitFor({ state: 'visible', timeout: 10000 }); | ||
| console.log('✅ Found database name input, filling "foobar"...'); | ||
| await dbNameInput.fill('foobar'); | ||
|
|
||
| console.log('🔍 Looking for connection string input...'); | ||
|
|
||
| const connStrInput = page.locator('input[placeholder*="postgres(ql)://username:password@host:port/database"]'); | ||
| await connStrInput.waitFor({ state: 'visible', timeout: 10000 }); | ||
| console.log('✅ Found connection string input, filling connection string...'); | ||
| await connStrInput.fill('postgresql://user:pass@localhost:5432/db'); | ||
|
|
||
| console.log('🔍 Looking for save/create button...'); | ||
|
|
||
| const saveButton = page.locator('button:has-text("Create")'); | ||
| await saveButton.waitFor({ state: 'visible', timeout: 10000 }); | ||
| console.log('✅ Found save button, clicking...'); | ||
| await saveButton.click(); | ||
|
|
||
| console.log('💾 Waiting for data source creation to complete...'); | ||
| await page.waitForLoadState('networkidle'); | ||
|
|
||
| // Look for success message or redirect | ||
| try { | ||
| const successMessage = page.locator('text=successfully, text=Success, text=created, text=added'); | ||
| await successMessage.waitFor({ state: 'visible', timeout: 10000 }); | ||
| console.log('✅ Data source created successfully!'); | ||
| } catch { | ||
| console.log('ℹ️ No explicit success message found, but form submission completed'); | ||
| } | ||
|
|
||
| console.log('🎉 PostgreSQL data source creation test completed successfully!'); | ||
| }); | ||
|
|
||
| test('Create PostgreSQL data source when connection string starts with postgres', async ({ | ||
| page, | ||
| }: { | ||
| page: Page; | ||
| }) => { | ||
| test.setTimeout(60000); // 1 minute for this specific test | ||
|
|
||
| // Enable browser console logging for debugging | ||
| page.on('console', (msg: ConsoleMessage) => console.log('🖥️ Browser console:', msg.text())); | ||
| page.on('pageerror', (error: Error) => console.log('🖥️ Browser error:', error.message)); | ||
|
|
||
| console.log('Starting PostgreSQL data source creation test...'); | ||
|
|
||
| console.log('🧭 Navigating to application...'); | ||
| await page.goto('/'); | ||
|
|
||
| // Wait for the page to load | ||
| await page.waitForLoadState('networkidle'); | ||
| console.log('✅ Page loaded successfully'); | ||
|
|
||
| try { | ||
| console.log('🔍 Checking for telemetry consent page...'); | ||
|
|
||
| const telemetryHeading = page.locator('h1:has-text("Help us improve liblab ai")'); | ||
| await telemetryHeading.waitFor({ state: 'visible', timeout: 5000 }); | ||
| console.log('📋 Found telemetry consent page, clicking Decline...'); | ||
|
|
||
| const declineButton = page.locator('button:has-text("Decline")'); | ||
| await declineButton.waitFor({ state: 'visible' }); | ||
| await declineButton.click(); | ||
|
|
||
| await page.waitForLoadState('networkidle'); | ||
| console.log('✅ Declined telemetry, waiting for redirect...'); | ||
| } catch { | ||
| console.warn('ℹ️ No telemetry consent page found, continuing...'); | ||
| } | ||
|
|
||
| try { | ||
| console.log('🔍 Checking for data source connection page...'); | ||
|
|
||
| const dataSourceHeading = page.locator('h1:has-text("Let\'s connect your data source")'); | ||
| await dataSourceHeading.waitFor({ state: 'visible', timeout: 5000 }); | ||
| console.log('💾 Found data source connection page, connecting to sample database...'); | ||
|
|
||
| const connectButton = page.locator('button:has-text("Connect")'); | ||
| await connectButton.waitFor({ state: 'visible', timeout: 10000 }); | ||
| console.log('🔗 Found Connect button, clicking...'); | ||
| await connectButton.click(); | ||
|
|
||
| await page.waitForLoadState('networkidle'); | ||
| console.log('✅ Connected to sample database, waiting for redirect...'); | ||
| } catch { | ||
| console.warn('ℹ️ No data source connection page found, continuing...'); | ||
| } | ||
|
|
||
| console.log('🔍 Looking for settings cog icon...'); | ||
|
|
||
| await page.mouse.move(0, 500); | ||
| await page.mouse.down(); // Move mouse to ensure the menu is visible | ||
|
|
||
| const menu = page.locator('[data-testid="menu"]'); | ||
| await menu.waitFor({ state: 'attached', timeout: 10000 }); | ||
| await menu.hover(); | ||
|
|
||
| const settingsButton = page.locator('[data-testid="settings-button"]'); | ||
| await settingsButton.waitFor({ state: 'attached', timeout: 10000 }); | ||
| await settingsButton.click(); | ||
|
|
||
| await page.getByRole('button', { name: 'Add Data Source' }).click(); | ||
|
|
||
| // Look for the database type selector | ||
| console.log('🔍 Looking for database type selector...'); | ||
|
|
||
| // help here | ||
| const dbTypeSelector = page.locator('select, [data-testid="add-data-source-select"]'); | ||
| await dbTypeSelector.waitFor({ state: 'visible', timeout: 10000 }); | ||
| console.log('✅ Found database type selector, selecting PostgreSQL...'); | ||
|
|
||
| // Click to open the dropdown and select PostgreSQL | ||
| await dbTypeSelector.click(); | ||
|
|
||
| await page.waitForLoadState('domcontentloaded'); | ||
|
|
||
| const postgresOption = page.locator('[id="postgres"]'); | ||
| await postgresOption.waitFor({ state: 'visible', timeout: 5000 }); | ||
| await postgresOption.click(); | ||
| console.log('✅ Selected PostgreSQL database type'); | ||
|
|
||
| console.log('🔍 Looking for database name input...'); | ||
|
|
||
| const dbNameInput = page.locator( | ||
| 'input[placeholder*="database name"], input[placeholder*="Database Name"], input[name*="dbName"], input[name*="name"]', | ||
| ); | ||
| await dbNameInput.waitFor({ state: 'visible', timeout: 10000 }); | ||
| console.log('✅ Found database name input, filling "foobar"...'); | ||
| await dbNameInput.fill('baz'); | ||
|
|
||
| console.log('🔍 Looking for connection string input...'); | ||
|
|
||
| const connStrInput = page.locator('input[placeholder*="postgres(ql)://username:password@host:port/database"]'); | ||
| await connStrInput.waitFor({ state: 'visible', timeout: 10000 }); | ||
| console.log('✅ Found connection string input, filling connection string...'); | ||
| await connStrInput.fill('postgres://user:pass@localhost:5432/db'); | ||
|
|
||
| console.log('🔍 Looking for save/create button...'); | ||
|
|
||
| const saveButton = page.locator('button:has-text("Create")'); | ||
| await saveButton.waitFor({ state: 'visible', timeout: 10000 }); | ||
| console.log('✅ Found save button, clicking...'); | ||
| await saveButton.click(); | ||
|
|
||
| console.log('💾 Waiting for data source creation to complete...'); | ||
| await page.waitForLoadState('networkidle'); | ||
|
|
||
| // Look for success message or redirect | ||
| try { | ||
| const successMessage = page.locator('text=successfully, text=Success, text=created, text=added'); | ||
| await successMessage.waitFor({ state: 'visible', timeout: 10000 }); | ||
| console.log('✅ Data source created successfully!'); | ||
| } catch { | ||
| console.log('ℹ️ No explicit success message found, but form submission completed'); | ||
| } | ||
|
|
||
| console.log('🎉 PostgreSQL data source creation test completed successfully!'); | ||
| }); | ||
| }); |
Contributor
There was a problem hiding this comment.
This new test file has opportunities for improved maintainability:
- Code Duplication: The two test cases are nearly identical. This can be refactored into a single, data-driven test to reduce duplication and make maintenance easier. You could create an array of test cases and loop through them.
const testCases = [ { name: '...', dbName: 'foobar', connStr: 'postgresql://...' }, { name: '...', dbName: 'baz', connStr: 'postgres://...' }, ]; for (const tc of testCases) { test(tc.name, async ({ page }) => { // ... shared test logic using tc.dbName and tc.connStr }); }
- Debug Logs: The file contains many
console.logstatements. While useful for debugging, they should be removed before merging to keep test output clean.
Comment on lines
+94
to
+120
| const dbNameInput = page.locator( | ||
| 'input[placeholder*="database name"], input[placeholder*="Database Name"], input[name*="dbName"], input[name*="name"]', | ||
| ); | ||
| await dbNameInput.waitFor({ state: 'visible', timeout: 10000 }); | ||
| console.log('✅ Found database name input, filling "foobar"...'); | ||
| await dbNameInput.fill('foobar'); | ||
|
|
||
| console.log('🔍 Looking for connection string input...'); | ||
|
|
||
| const connStrInput = page.locator('input[placeholder*="postgres(ql)://username:password@host:port/database"]'); | ||
| await connStrInput.waitFor({ state: 'visible', timeout: 10000 }); | ||
| console.log('✅ Found connection string input, filling connection string...'); | ||
| await connStrInput.fill('postgresql://user:pass@localhost:5432/db'); | ||
|
|
||
| console.log('🔍 Looking for save/create button...'); | ||
|
|
||
| const saveButton = page.locator('button:has-text("Create")'); | ||
| await saveButton.waitFor({ state: 'visible', timeout: 10000 }); | ||
| console.log('✅ Found save button, clicking...'); | ||
| await saveButton.click(); | ||
|
|
||
| console.log('💾 Waiting for data source creation to complete...'); | ||
| await page.waitForLoadState('networkidle'); | ||
|
|
||
| // Look for success message or redirect | ||
| try { | ||
| const successMessage = page.locator('text=successfully, text=Success, text=created, text=added'); |
Contributor
There was a problem hiding this comment.
The test selectors could be more robust to prevent flaky tests:
- Input Selectors: The locators for database name and connection string inputs (lines 94-96, 103-106) rely on placeholder text. It's better to add
data-testidattributes to these inputs inAddDataSourceForm.tsxfor more stable selection. - Success Message Selector: The locator for the success message (line 120) is very broad and could match other text on the page. A more specific selector, like a
data-testidon the success toast, would be more reliable.
mlanlazc
reviewed
Aug 19, 2025
Contributor
There was a problem hiding this comment.
I think this endpoint is unused, could you please remove it?
mlanlazc
approved these changes
Aug 19, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
📋 Pull Request Summary
🔗 Related Issues
📝 Changes Made
🧪 Testing
Testing Details:
📚 Documentation
🔄 Type of Change
🚨 Breaking Changes
Breaking Change Details:
📸 Screenshots/Videos
📋 Additional Notes