Unified Apis: fix fileserach spec #207
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplaces a minimal login in an E2E test with extensive Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 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 |
![coderabbitai[bot]](https://waps.l3s.uni-hannover.de/live/im_/https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cypress/e2e/filesearch/Filesearch.spec.ts (1)
257-266:⚠️ Potential issue | 🟡 MinorMissing
cy.wait('@uploadFile')after file selection.After
selectFileon line 257, the upload mock (@uploadFile) is never awaited. If the app fires anUploadFilesearchFilemutation on file selection, this wait is needed to ensure the mock has responded before proceeding. Currently the test relies on the implicitcy.wait(500)on line 263, which is fragile.♻️ Suggested fix
cy.get('input[type="file"]').selectFile('cypress/fixtures/sample.md', { force: true, }); + cy.wait('@uploadFile'); cy.get('div').should('contain', 'sample.md'); - cy.wait(500); - cy.get('[data-testid="ok-button"]').click();
🧹 Nitpick comments (2)
cypress/e2e/filesearch/Filesearch.spec.ts (2)
263-263: Avoid hardcodedcy.wait(500)— use an intercept wait or assertion instead.Hardcoded time waits are a Cypress anti-pattern that leads to flaky tests. This likely exists to wait for the upload to complete. If the
@uploadFilewait is added (as suggested above), this arbitrary wait can be removed.
98-130:vectorStore.idis set toassistantId— likely should be a distinct vector store ID.On line 119,
id: assistantIdsets the vector store'sidto the assistant's ID. While this works in a mock, it could mask bugs where the app incorrectly conflates these two IDs. Consider using a distinct value (e.g.,'vs_123') to better simulate the real API shape.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @.github/workflows/continuous-integration.yml:
- Line 90: The workflow currently hardcodes "git checkout feat/add_legacy_check"
which will break CI if that branch is deleted; replace this hardcoded checkout
with a guarded approach—either make the branch name a workflow input/variable
(e.g., "legacy_branch") and use that input instead of the literal, or perform a
conditional checkout that first tests for the branch's existence and falls back
to "main" (or aborts gracefully) if it doesn't exist; update the step that runs
"git checkout feat/add_legacy_check" to reference the parameter or to run a safe
existence check and fallback so CI won't fail when the feature branch is absent.
| git clone https://github.com/glific/glific-frontend.git | ||
| echo done. go to repo dir. | ||
| cd glific-frontend | ||
| git checkout feat/add_legacy_check |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check if the branch feat/add_legacy_check exists in glific-frontend
git ls-remote --heads https://github.com/glific/glific-frontend.git feat/add_legacy_checkRepository: glific/cypress-testing
Length of output: 139
🏁 Script executed:
# Examine the workflow file around line 90 to understand the context
cat -n .github/workflows/continuous-integration.yml | sed -n '80,100p'Repository: glific/cypress-testing
Length of output: 881
🏁 Script executed:
# Also check if there are any other related changes or error handling in the workflow
cat -n .github/workflows/continuous-integration.yml | head -100Repository: glific/cypress-testing
Length of output: 4033
Remove hardcoded feature branch or gate it behind a condition before merging to main.
The git checkout feat/add_legacy_check on line 90 pins the CI to a specific feature branch of glific-frontend. While this branch currently exists, once it's merged into main or deleted in the upstream repository, every CI run will fail. Either ensure feat/add_legacy_check is merged into main before this change reaches production, or make this checkout conditional (e.g., with error handling or a workflow parameter) to avoid breaking CI in the future.
🤖 Prompt for AI Agents
In @.github/workflows/continuous-integration.yml at line 90, The workflow
currently hardcodes "git checkout feat/add_legacy_check" which will break CI if
that branch is deleted; replace this hardcoded checkout with a guarded
approach—either make the branch name a workflow input/variable (e.g.,
"legacy_branch") and use that input instead of the literal, or perform a
conditional checkout that first tests for the branch's existence and falls back
to "main" (or aborts gracefully) if it doesn't exist; update the step that runs
"git checkout feat/add_legacy_check" to reference the parameter or to run a safe
existence check and fallback so CI won't fail when the feature branch is absent.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@cypress/e2e/filesearch/Filesearch.spec.ts`:
- Around line 80-107: The mock response in the GetAssistant stub is missing a
closing brace causing a parse error; update the req.reply block that builds the
mock (the object starting with req.reply({ body: { data: { assistant: {
assistant: { ... vectorStore: { ... } ) to add the missing closing brace(s) so
that all opened objects (req.reply, body, data, assistant, assistant,
vectorStore) are properly closed—ensure the final structure ends with the
correct sequence of closing braces and a closing parenthesis for req.reply, and
keep assistantId/vectorStore fields unchanged.
🧹 Nitpick comments (2)
cypress/e2e/filesearch/Filesearch.spec.ts (2)
6-238: Consider consolidating allcy.interceptcalls into a single handler.Registering 10 separate intercepts on the same
POST **/apiroute works because unmatched handlers pass through, but it's verbose and relies on Cypress's intercept stacking behavior. A single intercept with aswitchonreq.body.operationNamewould be easier to read and maintain, and avoids subtle ordering issues.♻️ Sketch of consolidated intercept
cy.intercept('POST', '**/api', (req) => { switch (req.body.operationName) { case 'Assistants': req.reply({ statusCode: 200, body: { data: { assistants: [/* ... */] } } }); break; case 'ListOpenaiModels': req.reply({ /* ... */ }); break; // ... remaining cases default: // Let unhandled operations pass through break; } }).as('graphql');You would then alias individual operations differently (e.g., via
req.alias = 'getAssistants'inside each case) to keep the per-operationcy.waitcalls.
274-279: Prefer deterministic waits overcy.wait(500).Hard-coded time waits are flaky in CI. If the intent is to wait for the upload mock to resolve, replace with
cy.wait('@uploadFile')or assert on a UI state that indicates readiness (e.g., a button becoming enabled).♻️ Suggested change
cy.get('div').should('contain', 'sample.md'); - - cy.wait(500); + cy.wait('@uploadFile');
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@cypress/e2e/filesearch/Filesearch.spec.ts`:
- Around line 6-251: Multiple cy.intercept calls all use the same route ('POST',
'**/api') so their aliases (getAssistants, listOpenaiModels, createAssistant,
etc.) misfire; replace the many intercepts with a single cy.intercept('POST',
'**/api', (req) => { ... }) handler that checks req.body.operationName and sets
req.alias accordingly (e.g., req.alias = 'getAssistants' when operationName ===
'Assistants'), then sends the appropriate mocked response via req.reply for each
case (match the existing response bodies from the current handlers such as the
assistants list, ListOpenaiModels, CreateAssistant, GetAssistant,
GetAssistantFiles, UploadFilesearchFile, AddAssistantFiles, RemoveAssistantFile,
UpdateAssistant, DeleteAssistant); ensure you keep the same alias strings
(getAssistants, listOpenaiModels, createAssistant, getAssistant,
getAssistantFiles, uploadFile, addAssistantFiles, removeAssistantFile,
updateAssistant, deleteAssistant) so existing cy.wait calls still work.
🧹 Nitpick comments (1)
cypress/e2e/filesearch/Filesearch.spec.ts (1)
286-286: Avoid hardcodedcy.wait(500); prefer an assertion-based wait.Hardcoded time waits make tests flaky across environments. If this wait is for the file to appear in the UI, the subsequent assertion on line 288 (
cy.get('[data-testid="ok-button"]').click()) or acy.containscheck should be sufficient without the sleep.- cy.wait(500); + // The file name assertion above already ensures the upload is reflected in the UI
| cy.login(); | ||
|
|
||
| // Mock API response for listing assistants | ||
| cy.intercept('POST', '**/api', (req) => { |
There was a problem hiding this comment.
why did we add this here? can't we test with the actual kaapi staging creds?
2 participants
Summary by CodeRabbit