-
Notifications
You must be signed in to change notification settings - Fork 3.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
test: fix failing gitsync tests #34214
Conversation
WalkthroughThe recent changes focus on improving the Git synchronization process within the Cypress test suite. The primary enhancement involves replacing the old merge method with a new, streamlined function. Additionally, the spec configuration for limited tests has been updated, and the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CypressTest
participant GitSync
User->>CypressTest: Run Git Sync Test
CypressTest->>GitSync: MergeToMaster()
GitSync->>GitSync: Handle merge to master
GitSync-->>CypressTest: Return merge status
CypressTest-->>User: Display results
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
/ci-test-limit |
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/9484186748. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Outside diff range and nitpick comments (2)
app/client/cypress/limited-tests.txt (1)
Line range hint
7-7
: Consider revising the comment for clarity:- #ci-test-limit uses this file to run minimum of specs. Do not run entire suite with this command. + #ci-test-limit uses this file to run a minimum set of specs. Do not run the entire suite with this command.app/client/cypress/e2e/Regression/ClientSide/Git/GitSync/GitSyncedApps_spec.js (1)
Line range hint
44-448
: Consider converting function expressions to arrow functions for consistency and to align with modern JavaScript practices:- it("1. Generate postgreSQL crud page , connect to git, clone the page, rename page with special character in it", function () { + it("1. Generate postgreSQL crud page , connect to git, clone the page, rename page with special character in it", () => {Repeat this change for all similar instances in this file.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- app/client/cypress/e2e/Regression/ClientSide/Git/GitSync/GitSyncedApps_spec.js (1 hunks)
- app/client/cypress/limited-tests.txt (1 hunks)
- app/client/cypress/support/Pages/GitSync.ts (2 hunks)
Additional context used
LanguageTool
app/client/cypress/limited-tests.txt
[uncategorized] ~7-~7: You might be missing the article “the” here. (AI_EN_LECTOR_MISSING_DETERMINER_THE)
Context: ...ile to run minimum of specs. Do not run entire suite with this command.
Biome
app/client/cypress/e2e/Regression/ClientSide/Git/GitSync/GitSyncedApps_spec.js
[error] 44-448: This function expression can be turned into an arrow function. (lint/complexity/useArrowFunction)
Function expressions that don't use this can be turned into arrow functions.
Safe fix: Use an arrow function instead.
Additional comments not posted (4)
app/client/cypress/limited-tests.txt (1)
2-2
: Updated spec name correctly focuses on Git synchronization tests.app/client/cypress/e2e/Regression/ClientSide/Git/GitSync/GitSyncedApps_spec.js (1)
403-403
: The methodgitSync.MergeToMaster()
is used correctly to streamline the merging process.app/client/cypress/support/Pages/GitSync.ts (2)
40-41
: Updated_mergeBranchDropdownmenu
selector correctly targets the new element for merge operations.
385-389
: Enhancements to theCheckMergeConflicts
method improve the robustness of merge conflict checking by waiting for elements to appear or disappear and asserting network status.
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/9484186748. |
app/client/cypress/limited-tests.txt
Outdated
@@ -1,5 +1,5 @@ | |||
# To run only limited tests - give the spec names in below format: | |||
cypress/e2e/Regression/ClientSide/Templates/Fork_Template_spec.js | |||
cypress/e2e/Regression/ClientSide/Git/GitSync/GitSyncedApps_spec.js |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
revert this please!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let me run few times and then remove and send for review, it has automatically added you as reviewer @ApekshaBhosale
/ci-test-limit runId=9484186748 |
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/9484950782. |
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/9484950782. |
/ci-test-limit runId=9484186748 |
13 similar comments
/ci-test-limit runId=9484186748 |
/ci-test-limit runId=9484186748 |
/ci-test-limit runId=9484186748 |
/ci-test-limit runId=9484186748 |
/ci-test-limit runId=9484186748 |
/ci-test-limit runId=9484186748 |
/ci-test-limit runId=9484186748 |
/ci-test-limit runId=9484186748 |
/ci-test-limit runId=9484186748 |
/ci-test-limit runId=9484186748 |
/ci-test-limit runId=9484186748 |
/ci-test-limit runId=9484186748 |
/ci-test-limit runId=9484186748 |
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/9486863753. |
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/9486865029. |
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/9486865438. |
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/9486866423. |
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/9486867802. |
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/9486867959. |
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/9486868369. |
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/9486868097. |
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/9486869178. |
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/9486869842. |
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/9486869824. |
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/9486870624. |
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/9486863753. |
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/9486864620. |
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/9486867959. |
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/9486867802. |
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/9486869842. |
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/9486870624. |
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/9486868369. |
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/9486865438. |
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/9486865026. |
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/9486869824. |
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/9486868097. |
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/9486865029. |
RCA:
Test was failing because of hard wait
Also the dropdown element which was being clicked was failing
Solution:
Removed hard waits and replaced with intercept
Updated locator and better method to fix tests
EE PR: https://github.com/appsmithorg/appsmith-ee/pull/4413
Summary by CodeRabbit
Refactor
Tests
CheckMergeConflicts
method to improve test accuracy and reliability by including element visibility checks and network status assertions.