Add e2e test for building apps with Postgres datasource#177
Conversation
Summary of ChangesHello @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 a new end-to-end test to validate the functionality of building applications using a PostgreSQL data source. The test simulates the user journey from creating a new PostgreSQL data source with a connection string provided via environment variables, to generating a 'users dashboard' application based on that data source, and finally asserting that the application is correctly rendered within the UI. This ensures the core integration between the application builder and PostgreSQL data sources remains robust. Highlights
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 introduces a new end-to-end test for creating an application with a PostgreSQL data source. The changes include adding necessary dependencies (dotenv, @types/node), configuring Playwright to use environment variables, and implementing the test spec file. My review focuses on improving the new test's robustness, performance, and readability by suggesting changes to avoid anti-patterns like static waits, simplifying complex code, and optimizing element selection logic.
| import { navigateToDataSourceForm, navigateToSettings, performInitialSetup } from '../helpers/setup'; | ||
|
|
||
| test.describe('PostgreSQL Data Source App Creation Flow', () => { | ||
| test('Create PostgreSQL data source and build users dashboard app', async ({ page }: { page: Page }) => { |
There was a problem hiding this comment.
The type annotation for the page object is redundant. Playwright's test runner automatically infers the type of page as Page from the test function's arguments. Removing the explicit type makes the code cleaner and easier to maintain.
| test('Create PostgreSQL data source and build users dashboard app', async ({ page }: { page: Page }) => { | |
| test('Create PostgreSQL data source and build users dashboard app', async ({ page }) => { |
| const frame = await iframe | ||
| .elementHandle() | ||
| .then((handle: ElementHandle<SVGElement | HTMLElement> | null) => handle?.contentFrame()); |
There was a problem hiding this comment.
The way the iframe's content frame is accessed is unnecessarily complex. The iframe.contentFrame() method already returns a promise that resolves to the frame. You can directly await it for cleaner and more readable code.
const frame = await iframe.contentFrame();| for (const selector of dashboardElements) { | ||
| try { | ||
| const element = frame.locator(selector); | ||
| await element.waitFor({ state: 'visible', timeout: 10000 }); | ||
| console.log(`✅ Found dashboard element: ${selector}`); | ||
| foundDashboardElement = true; | ||
| break; | ||
| } catch { | ||
| // Continue to next selector | ||
| } | ||
| } |
There was a problem hiding this comment.
The current implementation to find a dashboard element iterates through a list of selectors and waits for each one sequentially with a 10-second timeout. This can be inefficient and slow down the test. A better approach is to combine all selectors into a single locator and wait for the first matching element to become visible. This simplifies the code and can make the test faster. While this loses the log of which specific selector matched, the trade-off for performance and simplicity is worthwhile.
const combinedSelector = dashboardElements.join(', ');
try {
await frame.locator(combinedSelector).first().waitFor({ state: 'visible', timeout: 30000 });
console.log(`✅ Found a dashboard element.`);
foundDashboardElement = true;
} catch {
// Continue to next selector
}
2 participants
📋 Pull Request Summary
🔗 Related Issues
📝 Changes Made
🧪 Testing
Testing Details:
📚 Documentation
🔄 Type of Change
🚨 Breaking Changes
Breaking Change Details:
📸 Screenshots/Videos
📋 Additional Notes