ci: remove unnecessary tests and glific mocks#219
Open
rvignesh89 wants to merge 21 commits intomainfrom
Open
Conversation
📝 WalkthroughWalkthroughAdds a new E2E slow tests CI workflow and updates the existing E2E workflow to use ngrok tunneling for public API access, cache build artifacts, reduce Cypress shards to 2, and set Node.js version pinning. Refactors the Filesearch test spec to remove mocked API calls and test real backend integration instead of isolated mocked behavior. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
| GitGuardian id | GitGuardian status | Secret | Commit | Filename | |
|---|---|---|---|---|---|
| 17660795 | Triggered | Generic Password | d9dc9d1 | .github/workflows/e2e-slow.yml | View secret |
🛠 Guidelines to remediate hardcoded secrets
- Understand the implications of revoking this secret by investigating where it is used in your code.
- Replace and store your secret safely. Learn here the best practices.
- Revoke and rotate this secret.
- If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.
To avoid such incidents in the future consider
- following these best practices for managing and storing secrets including API keys and other credentials
- install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.
🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.
rvignesh89
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (5)
cypress/e2e/filesearch/Filesearch.spec.ts (3)
36-36: Replace staticcy.wait(500)with an assertion.Static waits are flaky and slow. Wait for a specific condition instead, such as the form becoming interactive.
Proposed fix
cy.get('[data-testid="headingButton"]').click(); - cy.wait(500); + cy.get('[data-testid="createAssistantContainer"]').should('be.visible'); cy.get('input[name="name"]').type(assistantName);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cypress/e2e/filesearch/Filesearch.spec.ts` at line 36, Replace the static cy.wait(500) in Filesearch.spec.ts with a deterministic assertion that the UI is ready; locate the cy.wait(500) call and change it to an assertion that the search form or input is present and interactive (for example assert the search input or submit button exists/visible/enabled via cy.get('<your-form-or-input-selector>').should('be.visible').and('be.enabled') or cy.get('<form-selector>').should('exist') so the test waits for a concrete condition instead of a fixed timeout.
116-116: Replace staticcy.wait(500)with an assertion.Same as line 36—prefer waiting for a UI condition.
Proposed fix
cy.get('input[placeholder="Search"]').type('SomeSearchTerm'); - cy.wait(500); + cy.get('[data-testid="listItem"]').should('exist'); // or appropriate assertion cy.get('[data-testid="CloseIcon"]').click();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cypress/e2e/filesearch/Filesearch.spec.ts` at line 116, Replace the static cy.wait(500) in Filesearch.spec.ts with a deterministic assertion: instead of sleeping, wait for a specific UI condition or network alias that indicates the state change (for example, assert that the file list element is visible, contains expected text, or that a stubbed XHR/route alias has completed); locate the second occurrence at the cy.wait(500) around line ~116 (same pattern as the earlier cy.wait at ~36) and change it to an assertion on the relevant DOM element or use cy.wait('@alias') so the test proceeds only when the UI or network is ready.
1-9: Test suite has implicit inter-test dependencies.Tests rely on state from previous tests (e.g.,
assistantNamecreated in one test is searched/updated/deleted in subsequent tests). If tests run in isolation or out of order, they will fail. While this is intentional for an end-to-end workflow, consider:
- Adding a comment at the top explaining the sequential dependency requirement
- Ensuring Cypress is configured to run specs in order (default behavior)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cypress/e2e/filesearch/Filesearch.spec.ts` around lines 1 - 9, This test suite relies on state carried between tests (assistantName / updatedName) and must be run sequentially; add a clear top-of-file comment above describe('File search', ...) stating that tests are inter-dependent and must run in order, and optionally reference Cypress run-order configuration, and also consider documenting that beforeEach (cy.login, cy.visit) does not reset assistant state—if you prefer isolation later, convert dependent tests to create and clean up their own assistant within their individual it() blocks or add a dedicated setup/teardown helper to create/delete the assistant rather than relying on a previously created assistant.README.md (1)
35-37: Add language identifier to fenced code block.The code block should specify a language for proper syntax highlighting.
Proposed fix
-``` +```bash yarn cypress run --config excludeSpecPattern=cypress/e2e/filesearch/Filesearch.spec.ts</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@README.mdaround lines 35 - 37, The fenced code block that shows the Cypress
command is missing a language identifier; update the block around the command
yarn cypress run --config excludeSpecPattern=cypress/e2e/filesearch/Filesearch.spec.tsto use a language
tag (e.g., add ```bash) so syntax highlighting works in README.md.</details> </blockquote></details> <details> <summary>.tool-versions (1)</summary><blockquote> `1-1`: **CI workflows do not consume `.tool-versions` for Node.js version.** The `setup-node@v6` action in both `e2e.yml` (line 48) and `e2e-slow.yml` (line 40) lacks a `node-version-file` parameter, so CI will use the runner's default Node.js version rather than `22.21.1`. To ensure consistency between local and CI environments: <details> <summary>Proposed fix for workflow files</summary> ```diff - name: Use latest Node.js uses: actions/setup-node@v6 + with: + node-version-file: '.tool-versions' ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In @.tool-versions at line 1, CI is not reading the .tool-versions Node.js value because the setup-node@v6 steps in e2e.yml and e2e-slow.yml omit the node-version-file parameter; update the GitHub Actions jobs that call actions/setup-node@v6 (search for the setup-node usage in e2e.yml and e2e-slow.yml) to add node-version-file: ".tool-versions" (or pass node-version: 22.21.1 if you prefer an explicit value) so the runner uses the repository's Node.js version. ``` </details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/e2e-slow.yml:
- Around line 6-9: The workflow's on: pull_request types list includes an
invalid activity "push" which fails validation; edit the on: block by removing
"push" from the pull_request.types array (reference: the pull_request.types
entry in the .github/workflows/e2e-slow.yml diff) and, if you intended to run
this workflow on push events as well, add a separate top-level "push:" trigger
instead of placing "push" inside pull_request.types.In @.github/workflows/e2e.yml:
- Line 102: The workflow is cloning a feature branch literal ("git clone
--branch rvignesh/seed-dev-kaapi ...") which can disappear after the PR; update
that command to clone a stable branch (e.g., replace the branch name with
"main") or make it dynamic by using the GitHub workflow variable (for example
use the repository default branch or "${{ github.ref }}" / "${{ github.head_ref
}}" as appropriate) so the CI does not depend on a temporary feature branch;
locate the git clone command string ("git clone --branch
rvignesh/seed-dev-kaapi") and change the branch argument accordingly.- Around line 71-91: The workflow exports GLIFIC_API_HOST_OVERRIDE but Cypress
uses hardcoded URLs (baseUrl and backendUrl), so update Cypress config (e.g.,
cypress.config.js or cypress.json) to read process.env.GLIFIC_API_HOST_OVERRIDE
and derive baseUrl and backendUrl when that env var is set (use
GLIFIC_API_HOST_OVERRIDE as the host/hostname and append the existing
ports/paths), ensuring symbols like baseUrl and backendUrl in the Cypress
configuration are assigned from process.env.GLIFIC_API_HOST_OVERRIDE if present;
alternatively remove the GLIFIC_API_HOST_OVERRIDE export from the workflow if
ngrok tunneling is not needed.In
@cypress/e2e/filesearch/Filesearch.spec.ts:
- Around line 69-96: The current recursive polling function checkStatusOrRetry
(with maxTries and tryCount) is fragile and the final return doesn't make
Cypress wait; replace this recursive pattern with Cypress' retryable commands or
the cypress-recurse utility: remove checkStatusOrRetry recursion and instead use
cy.recurse (or a cy.get(...).should(...) loop) to repeatedly read
'[data-testid="assistantStatus"] span' until its trimmed text equals 'Ready' (or
until maxTries/retry timeout is reached), performing cy.reload() between
attempts if needed and using configurable interval/timeout instead of a fixed
cy.wait(5000); ensure the test returns the Cypress chain (the cy.recurse /
cy.get(...).should(...) promise) so Cypress properly waits for the status to
become Ready and eliminate tryCount/return checkStatusOrRetry recursion.
Nitpick comments:
In @.tool-versions:
- Line 1: CI is not reading the .tool-versions Node.js value because the
setup-node@v6 steps in e2e.yml and e2e-slow.yml omit the node-version-file
parameter; update the GitHub Actions jobs that call actions/setup-node@v6
(search for the setup-node usage in e2e.yml and e2e-slow.yml) to add
node-version-file: ".tool-versions" (or pass node-version: 22.21.1 if you prefer
an explicit value) so the runner uses the repository's Node.js version.In
@cypress/e2e/filesearch/Filesearch.spec.ts:
- Line 36: Replace the static cy.wait(500) in Filesearch.spec.ts with a
deterministic assertion that the UI is ready; locate the cy.wait(500) call and
change it to an assertion that the search form or input is present and
interactive (for example assert the search input or submit button
exists/visible/enabled via
cy.get('').should('be.visible').and('be.enabled')
or cy.get('').should('exist') so the test waits for a concrete
condition instead of a fixed timeout.- Line 116: Replace the static cy.wait(500) in Filesearch.spec.ts with a
deterministic assertion: instead of sleeping, wait for a specific UI condition
or network alias that indicates the state change (for example, assert that the
file list element is visible, contains expected text, or that a stubbed
XHR/route alias has completed); locate the second occurrence at the cy.wait(500)
around line ~116 (same pattern as the earlier cy.wait at ~36) and change it to
an assertion on the relevant DOM element or use cy.wait('@alias') so the test
proceeds only when the UI or network is ready.- Around line 1-9: This test suite relies on state carried between tests
(assistantName / updatedName) and must be run sequentially; add a clear
top-of-file comment above describe('File search', ...) stating that tests are
inter-dependent and must run in order, and optionally reference Cypress
run-order configuration, and also consider documenting that beforeEach
(cy.login, cy.visit) does not reset assistant state—if you prefer isolation
later, convert dependent tests to create and clean up their own assistant within
their individual it() blocks or add a dedicated setup/teardown helper to
create/delete the assistant rather than relying on a previously created
assistant.In
@README.md:
- Around line 35-37: The fenced code block that shows the Cypress command is
missing a language identifier; update the block around the commandyarn cypress run --config excludeSpecPattern=cypress/e2e/filesearch/Filesearch.spec.tsto
use a language tag (e.g., add ```bash) so syntax highlighting works in
README.md.</details> <details> <summary>🪄 Autofix (Beta)</summary> Fix all unresolved CodeRabbit comments on this PR: - [ ] <!-- {"checkboxId": "4b0d0e0a-96d7-4f10-b296-3a18ea78f0b9"} --> Push a commit to this branch (recommended) - [ ] <!-- {"checkboxId": "ff5b1114-7d8c-49e6-8ac1-43f82af23a33"} --> Create a new PR with the fixes </details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: defaults **Review profile**: CHILL **Plan**: Pro **Run ID**: `44f4f897-5e38-4747-b954-a4a7c0475938` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 52e21a27d01225a600d3214ed5fed4509fc850b7 and eb403b667ac1c8201fcead89c3f575ec2bcee8ed. </details> <details> <summary>📒 Files selected for processing (5)</summary> * `.github/workflows/e2e-slow.yml` * `.github/workflows/e2e.yml` * `.tool-versions` * `README.md` * `cypress/e2e/filesearch/Filesearch.spec.ts` </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
Comment on lines
+6
to
+9
| on: | ||
| pull_request: | ||
| types: [opened, synchronize, reopened, labeled, unlabeled, push] | ||
| branches: [main] |
There was a problem hiding this comment.
Invalid activity type push for pull_request event.
The push activity type is not valid for the pull_request webhook event and will cause the workflow to fail validation.
Proposed fix
on:
pull_request:
- types: [opened, synchronize, reopened, labeled, unlabeled, push]
+ types: [opened, synchronize, reopened, labeled, unlabeled]
branches: [main]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| on: | |
| pull_request: | |
| types: [opened, synchronize, reopened, labeled, unlabeled, push] | |
| branches: [main] | |
| on: | |
| pull_request: | |
| types: [opened, synchronize, reopened, labeled, unlabeled] | |
| branches: [main] |
🧰 Tools
🪛 actionlint (1.7.11)
[error] 8-8: invalid activity type "push" for "pull_request" Webhook event. available types are "assigned", "auto_merge_disabled", "auto_merge_enabled", "closed", "converted_to_draft", "demilestoned", "dequeued", "edited", "enqueued", "labeled", "locked", "milestoned", "opened", "ready_for_review", "reopened", "review_request_removed", "review_requested", "synchronize", "unassigned", "unlabeled", "unlocked"
(events)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/e2e-slow.yml around lines 6 - 9, The workflow's on:
pull_request types list includes an invalid activity "push" which fails
validation; edit the on: block by removing "push" from the pull_request.types
array (reference: the pull_request.types entry in the
.github/workflows/e2e-slow.yml diff) and, if you intended to run this workflow
on push events as well, add a separate top-level "push:" trigger instead of
placing "push" inside pull_request.types.
There was a problem hiding this comment.
@rvignesh89, sounds good! Just a reminder that the fix is simply removing push from the pull_request.types array:
on:
pull_request:
types: [opened, synchronize, reopened, labeled, unlabeled]
branches: [main]If you also intended to trigger this workflow on direct pushes to main, that would need a separate top-level push: trigger block. Let me know if you need any help with that!
🧠 Learnings used
Learnt from: rvignesh89
Repo: glific/cypress-testing PR: 219
File: .github/workflows/e2e.yml:71-91
Timestamp: 2026-04-01T02:44:21.649Z
Learning: When reviewing e2e GitHub Actions workflows in this repo, don’t assume `GLIFIC_API_HOST_OVERRIDE` is unused just because Cypress tests/config contain hardcoded URLs. This variable is intended for the Glific backend commands executed in the workflow (e.g., `mix setup` and `mix phx.server`), not for Cypress itself. Only flag/consider changes to Cypress URLs if there’s evidence the backend/API host resolution behavior is affected.
rvignesh89
commented
Apr 1, 2026
| mkdir project | ||
| cd project | ||
| echo clone glific repo | ||
| git clone --branch rvignesh/seed-dev-kaapi https://github.com/glific/glific.git |
Author
There was a problem hiding this comment.
This branch will be changed once glific PR is merged to master
akanshaaa19
approved these changes
Apr 8, 2026
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
4 participants
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.
Filesearch tests were mocking requests to Glific BE which made these unit tests and not integrations tests. This PR removes those mocks and also connects to Kaapi staging to make this a proper E2E test. Other changes to make this work -