fix: add fixes to make e2e shard 8 be less prone to failures#2091
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughFrontend Playwright tests broaden response assertions, configure model providers explicitly, synchronize document activation, normalize streamed tool calls, extend chat waits, and report Watsonx/OpenAI connection errors. ChangesFrontend test reliability
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
frontend/tests/core/z_update_model_providers.spec.ts (1)
68-73: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider wrapping
deleteDocumentin atry...catchblock to prevent test flakiness.If the document does not exist (e.g., during a fresh test run or if a previous test failed before ingestion),
deleteDocumentmay throw an error when attempting to locate the document row, which will fail the test. Wrapping it in atry...catchblock, similar to how it is handled inurl_ingestion.spec.ts, ensures the cleanup step is idempotent and resilient.♻️ Proposed fix
await settings.clickTab("Langflow"); await settings.selectModel("Language model", "gpt-4o-mini"); await settings.selectModel("Embedding model", "text-embedding-3-small"); - await knowledge.deleteDocument(testDocumentName); + try { + await knowledge.deleteDocument(testDocumentName); + } catch (_error) { + // Document doesn't exist, safe to proceed + } await knowledge.ingestFile(testDocumentPath); await knowledge.verifyDocumentActive(testDocumentName);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/tests/core/z_update_model_providers.spec.ts` around lines 68 - 73, Make the cleanup call to knowledge.deleteDocument(testDocumentName) in the test setup resilient to a missing document by wrapping it in a try...catch, matching the existing pattern in url_ingestion.spec.ts. Allow the test to continue when deletion fails because the document is absent, then preserve the subsequent ingestFile and verifyDocumentActive steps.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@frontend/tests/core/z_update_model_providers.spec.ts`:
- Around line 68-73: Make the cleanup call to
knowledge.deleteDocument(testDocumentName) in the test setup resilient to a
missing document by wrapping it in a try...catch, matching the existing pattern
in url_ingestion.spec.ts. Allow the test to continue when deletion fails because
the document is absent, then preserve the subsequent ingestFile and
verifyDocumentActive steps.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5727c775-c2bb-4d8c-b052-52570797030e
📒 Files selected for processing (5)
frontend/tests/core/basic-questions.spec.tsfrontend/tests/core/url_ingestion.spec.tsfrontend/tests/core/z_update_model_providers.spec.tsfrontend/tests/pages/Chat.tsfrontend/tests/pages/Settings.ts
|
React Doctor found 1 new issue in 1 file · 1 warning · score 70 / 100 (Needs work) · 7 fixed · vs 1 warning
Reviewed by React Doctor for commit |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@frontend/tests/pages/Chat.ts`:
- Around line 213-234: Update the streamed tool-call handling around
capturedToolData so entries from chunk.delta.tool_calls are accumulated across
chunks rather than locked on the first partial chunk. Preserve the initial tool
metadata, append each subsequent tc.function.arguments fragment to the existing
arguments, and continue processing until the complete streamed arguments are
available for downstream JSON.parse.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e923a2ec-fb99-47b2-b23c-52182da9813e
📒 Files selected for processing (1)
frontend/tests/pages/Chat.ts
mpawlow
left a comment
There was a problem hiding this comment.
Code Review 1
- ✅ Approved / LGTM 🚀
This pull request makes several improvements to the Playwright test suite for model provider configuration, chat behavior, and URL ingestion. The main focus is on making tests more robust, expanding coverage for error cases, and improving reliability and error messaging.
Test robustness and reliability:
Chat.deleteChatto handle slower environments, improving test reliability.Model provider configuration and validation:
Settings, including checking for success or error toasts and throwing descriptive errors on failure. [1] [2] [3]z_update_model_providers.spec.tsto select specific language and embedding models after switching providers, and to skip tests if Watsonx credentials are not set. [1] [2] [3]Test coverage and flexibility improvements:
Summary by CodeRabbit