ci(playwright): split E2E tests into AI and non-AI groups#522
Open
ci(playwright): split E2E tests into AI and non-AI groups#522
Conversation
Completed Working on "Code Review"✅ Review submitted successfully: COMMENT. Total comments: 2 across 2 files. ✅ Workflow completed successfully. |
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Snapshot WarningsEnsure that dependencies are being submitted on PR branches and consider enabling retry-on-snapshot-warnings. See the documentation for more information and troubleshooting advice. Scanned FilesNone |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughCI workflow now computes whether Azure LLM secrets (AZURE_API_KEY, AZURE_API_BASE, AZURE_API_VERSION) are present, gates AI-dependent setup (Docker Compose and AI tests) on that flag, splits Playwright into a non-AI run (always) and an AI run (only when secrets exist, using !cancelled()), test specs were tagged Changes
Sequence Diagram(s)sequenceDiagram
participant CI as CI Workflow
participant S as Secrets Store
participant DB as Docker Compose DB
participant Pno as Playwright (non-AI)
participant Pai as Playwright (AI)
CI->>S: check AZURE_API_KEY, AZURE_API_BASE, AZURE_API_VERSION
alt has-ai-secrets == true
CI->>CI: set has-ai-secrets = true
CI->>DB: start test databases
DB-->>CI: DB ready
CI->>Pno: run Playwright (--grep-invert `@requires-ai`)
Pno-->>CI: non-AI results
CI->>Pai: run Playwright (--grep `@requires-ai`, !cancelled())
Pai-->>CI: AI-test results
else has-ai-secrets == false
CI->>CI: set has-ai-secrets = false
CI->>Pno: run Playwright (--grep-invert `@requires-ai`)
Pno-->>CI: non-AI results
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
🚅 Deployed to the QueryWeaver-pr-522 environment in queryweaver
|
railway-app
bot
temporarily deployed
to
queryweaver / QueryWeaver-pr-522
![overcut-ai[bot]](https://avatars.githubusercontent.com/in/1367736?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Summary
Found 2 comments total: 1 MAJOR, 1 MINOR.
Key themes
- Test classification accuracy: One UI-only test appears incorrectly tagged as AI-dependent, reducing baseline coverage when secrets are unavailable.
- CI behavior documentation clarity: Contributor-facing docs should better explain when AI-tagged tests are skipped (including fork PR scenarios).
Next steps
- Reclassify the
empty query submission is preventedtest so non-AI baseline validation always runs. - Update
AGENTS.mdto explicitly state AI-tagged tests depend on secret availability and are usually skipped for fork/Dependabot PRs.
gkorland
force-pushed
the
ci/split-e2e-ai-tests
branch
from
d9524e5 to
806a53f
Compare
railway-app
bot
temporarily deployed
to
queryweaver / QueryWeaver-pr-522
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/playwright.yml:
- Around line 29-40: The gate step with id "check-secrets" only checks
AZURE_API_KEY but should verify the full Azure config (AZURE_API_KEY,
AZURE_API_BASE, AZURE_API_VERSION) before setting has-ai-secrets; update the
check-secrets run logic to ensure all three environment variables are non-empty
and only then emit "has-ai-secrets=true" (otherwise emit false) so the AI shard
is skipped when any of AZURE_API_KEY, AZURE_API_BASE, or AZURE_API_VERSION is
missing.
- Around line 187-196: Replace the two sequential Playwright steps ("Run
Playwright tests (no AI)" and "Run Playwright tests (AI)") with a single job
using a strategy.matrix that includes two entries (e.g., {name: "no-ai", grep:
"--grep-invert `@requires-ai`"} and {name: "ai", grep: "--grep `@requires-ai`"}),
set strategy.fail-fast: false, and run npx playwright test using matrix.grep;
guard the AI shard by adding an if on the run step like: if: matrix.name != 'ai'
|| steps.check-secrets.outputs.has-ai-secrets == 'true' so both shards execute
independently and the job still fails if any matrix run fails.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6d1acb5f-a7d7-46b0-9269-f28eba088335
📒 Files selected for processing (4)
.github/workflows/playwright.ymlAGENTS.mde2e/tests/chat.spec.tse2e/tests/database.spec.ts
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
e2e/tests/chat.spec.ts (1)
144-153:⚠️ Potential issue | 🟠 MajorKeep empty-input validation in the non-AI test lane.
At Line 144, this test checks client-side disabled-state behavior for empty input. Tagging it
@requires-aidrops baseline UI validation coverage whenever secrets are unavailable. Please remove the AI tag (and avoid AI-dependent setup in this test).Suggested fix
- test('empty query submission is prevented', { tag: '@requires-ai' }, async () => { + test('empty query submission is prevented', async () => { const homePage = await browser.createNewPage(HomePage, getBaseUrl(), 'e2e/.auth/user.json'); await browser.setPageToFullScreen(); - - // Ensure database is connected (will skip if already connected) - await homePage.ensureDatabaseConnected(apiCall); // Verify send button is disabled with empty input const isSendButtonDisabled = await homePage.isSendQueryButtonDisabled(); expect(isSendButtonDisabled).toBeTruthy(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/tests/chat.spec.ts` around lines 144 - 153, The test "empty query submission is prevented" should not be in the AI-dependent lane: remove the `@requires-ai` tag from the test declaration and eliminate AI-dependent setup usage — specifically, change the ensureDatabaseConnected(apiCall) call so it does not depend on the AI fixture (e.g., call ensureDatabaseConnected() or a non-AI variant) and avoid passing or referencing apiCall; keep the rest of the test (HomePage creation, setPageToFullScreen, isSendQueryButtonDisabled assertion) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@e2e/tests/chat.spec.ts`:
- Around line 144-153: The test "empty query submission is prevented" should not
be in the AI-dependent lane: remove the `@requires-ai` tag from the test
declaration and eliminate AI-dependent setup usage — specifically, change the
ensureDatabaseConnected(apiCall) call so it does not depend on the AI fixture
(e.g., call ensureDatabaseConnected() or a non-AI variant) and avoid passing or
referencing apiCall; keep the rest of the test (HomePage creation,
setPageToFullScreen, isSendQueryButtonDisabled assertion) unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 796ae3ae-b88d-44f2-96c1-bd8b68653f44
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (4)
.github/workflows/playwright.ymlAGENTS.mde2e/tests/chat.spec.tse2e/tests/database.spec.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- .github/workflows/playwright.yml
- AGENTS.md
- e2e/tests/database.spec.ts
Tag tests requiring LLM secrets (Azure OpenAI) with @requires-ai. The workflow now runs in two phases: - Non-AI tests (sidebar, userProfile, error paths) always run - AI tests (database connection, chat queries) only run when secrets are available This replaces the previous blanket skip for Dependabot PRs with a granular approach that retains smoke test coverage even when secrets are unavailable. Docker Compose test databases are also conditionally started only when AI tests will run. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Remove @requires-ai tag from 'empty query submission is prevented' test since it only validates UI state (send button disabled) without needing a database connection or AI embeddings - Check all three Azure secrets (API_KEY, API_BASE, API_VERSION) in the secrets detection step, not just the API key fail, preserving test signal for both shards - Update AGENTS.md to mention fork PRs alongside Dependabot as contexts where secrets are unavailable Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
gkorland
force-pushed
the
ci/split-e2e-ai-tests
branch
from
806a53f to
4ad8f7c
Compare
Contributor
Author
Agent Review SummaryComments Fixed (agreed & resolved)
CI StatusAll 11 checks passing ✅ |
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
1 participant
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.
Summary
Split Playwright E2E tests into two groups based on whether they require LLM secrets (Azure OpenAI API). Non-AI tests always run; AI tests only run when secrets are available. This replaces the blanket skip for Dependabot PRs with granular coverage.
Changes
e2e/tests/chat.spec.ts— Tagged 8 of 9 tests with@requires-ai(all exceptchat controls disabled without database connection)e2e/tests/database.spec.ts— Tagged 8 of 9 tests with@requires-ai(all exceptinvalid connection string -> shows error)e2e/tests/sidebar.spec.ts— No changes (all tests are non-AI)e2e/tests/userProfile.spec.ts— No changes (all tests are non-AI).github/workflows/playwright.yml:ifskip conditionCheck for AI secretsstep that detects secret availabilityRun Playwright tests (no AI)(always) andRun Playwright tests (AI)(conditional)AGENTS.md— Updated Testing and CI/CD sectionsTest Split (chromium in CI)
@requires-ai)Testing
--grep @requires-aiand--grep-invert @requires-ailist correct tests vianpx playwright test --listMemory / Performance Impact
N/A — CI workflow change only. Dependabot PRs will be faster (no Docker Compose, no AI test wait).
Related Issues
Replaces the blanket skip from #518 with granular test splitting.
Summary by CodeRabbit
Tests
Chores
Documentation