Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
7 Skipped Deployments
|
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
🎭 Playwright Test Results
Details
Skipped testsFeatures › sql-editor.spec.ts › SQL Editor › snippet favourite works as expected |
|
We can remove all the tests related to the banner, I don't think that needs to be tested IMO. Just enabling and viewing the index in query performance |
…abase into e2e/add-index-advisor-tests
WalkthroughIntroduces a new end-to-end test suite for the Index Advisor feature that verifies extension enablement and warning detection. The suite includes helper functions for extension management, test table creation, query execution, and anti-flakiness measures using explicit waits and API response hooks. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
e2e/studio/features/index-advisor.spec.ts (1)
448-458: Test silently passes on failure, defeating its purpose.Wrapping the assertion in try-catch means this test will always pass, even when no Index Advisor warnings are found. This hides actual failures and provides false confidence.
Consider one of these approaches:
- Let the assertion fail naturally if warnings are expected
- Use
test.fail()ortest.fixme()if the feature is known to be flaky- Use a soft assertion with proper reporting if this is intentionally lenient
- try { - await expect( - queryRows, - 'At least one query with Index Advisor warning should be visible' - ).toBeVisible({ timeout: 10000 }) - console.log('Successfully found queries with Index Advisor recommendations') - } catch (e) { - console.log( - 'No queries with Index Advisor warnings found - may need more query executions or time' - ) - } + // Use soft assertion if warnings may not always appear immediately + await expect + .soft(queryRows, 'At least one query with Index Advisor warning should be visible') + .toBeVisible({ timeout: 10000 })Alternatively, if warnings are truly expected, remove the try-catch entirely:
- try { - await expect( - queryRows, - 'At least one query with Index Advisor warning should be visible' - ).toBeVisible({ timeout: 10000 }) - console.log('Successfully found queries with Index Advisor recommendations') - } catch (e) { - console.log( - 'No queries with Index Advisor warnings found - may need more query executions or time' - ) - } + await expect( + queryRows, + 'At least one query with Index Advisor warning should be visible' + ).toBeVisible({ timeout: 10000 })
🧹 Nitpick comments (1)
e2e/studio/features/index-advisor.spec.ts (1)
395-401: Consider adding assertion or documenting this as a setup step.This test runs queries but has no assertions, making it effectively a setup step for the final test. Consider either:
- Adding a basic assertion (e.g., verify no SQL errors occurred)
- Moving this logic into the final test or a helper called by the final test
This is minor since the test serves as preparation for the warnings verification test.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
e2e/studio/features/index-advisor.spec.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
e2e/studio/features/index-advisor.spec.ts (3)
e2e/studio/utils/to-url.ts (1)
toUrl(3-5)e2e/studio/utils/wait-for-response.ts (1)
waitForApiResponse(13-21)e2e/studio/utils/test.ts (1)
test(17-21)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: format
- GitHub Check: test
- GitHub Check: typecheck
🔇 Additional comments (8)
e2e/studio/features/index-advisor.spec.ts (8)
1-29: LGTM!Well-documented test file with clear explanations of the anti-flakiness measures. The imports are appropriate for the test requirements.
35-89: LGTM!The helper function has good defensive error handling with the try-catch pattern for API waits and graceful handling of missing UI elements. The dual check (waitFor + count) provides resilience when the waitFor times out.
94-176: LGTM!Good implementation with the API response waiter set up before triggering the click action. The 30-second timeout for extension initialization is appropriate. The duplicated code for enabling both extensions could be extracted into a shared helper, but it's acceptable for test clarity.
181-209: LGTM!The function correctly waits for API response before continuing. The
waitForTimeoutfallback in the catch block (line 207) is acceptable as a last resort when the API times out, though a more deterministic approach (e.g., waiting for a success indicator in the UI) would be preferable if available.
214-241: LGTM!Running the query multiple times to ensure visibility in Query Performance is a reasonable approach. The API wait pattern is consistent with other helpers.
243-263: LGTM!Serial execution is the correct choice for these stateful tests that share a page instance. The
refreshIndexAdvisorStatehelper appropriately synchronizes the local state with the actual extension status.
265-290: LGTM!Proper cleanup with defensive error handling ensures test artifacts are removed without failing the suite if cleanup encounters issues.
292-370: LGTM!Well-structured tests with appropriate conditional skipping based on the extension state. The verification of both extensions in the UI and the Warnings filter visibility are good functional checks.
2 participants
I have read the CONTRIBUTING.md file.
YES
What kind of change does this PR introduce?
Adds a series of tests to make sure Index Advisor is enabled and warnings show in Query Performance.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.