chore: fixes mergeviaremote_spec flakiness #38635
Conversation
WalkthroughThis pull request focuses on refactoring a Git synchronization test suite in Cypress, specifically the Changes
Possibly related PRs
Suggested Labels
Suggested Reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (7)
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 using PR comments)
Other keywords and placeholders
Documentation and Community
|
github-actions
bot
added
the
skip-changelog
|
/ci-test-limit-count run_count=5 |
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/12750203169. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/client/cypress/e2e/Regression/ClientSide/Git/GitSync/MergeViaRemote_spec.ts(5 hunks)app/client/cypress/limited-tests.txt(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
app/client/cypress/limited-tests.txt (1)
Pattern app/client/cypress/**/**.*: Review the following e2e test code written using the Cypress test library. Ensure that:
- Follow best practices for Cypress code and e2e automation.
- Avoid using cy.wait in code.
- Avoid using cy.pause in code.
- Avoid using agHelper.sleep().
- Use locator variables for locators and do not use plain strings.
- Use data-* attributes for selectors.
- Avoid Xpaths, Attributes and CSS path.
- Avoid selectors like .btn.submit or button[type=submit].
- Perform logins via API with LoginFromAPI.
- Perform logout via API with LogOutviaAPI.
- Perform signup via API with SignupFromAPI.
- Avoid using it.only.
- Avoid using after and aftereach in test cases.
- Use multiple assertions for expect statements.
- Avoid using strings for assertions.
- Do not use duplicate filenames even with different paths.
- Avoid using agHelper.Sleep, this.Sleep in any file in code.
app/client/cypress/e2e/Regression/ClientSide/Git/GitSync/MergeViaRemote_spec.ts (1)
Pattern app/client/cypress/**/**.*: Review the following e2e test code written using the Cypress test library. Ensure that:
- Follow best practices for Cypress code and e2e automation.
- Avoid using cy.wait in code.
- Avoid using cy.pause in code.
- Avoid using agHelper.sleep().
- Use locator variables for locators and do not use plain strings.
- Use data-* attributes for selectors.
- Avoid Xpaths, Attributes and CSS path.
- Avoid selectors like .btn.submit or button[type=submit].
- Perform logins via API with LoginFromAPI.
- Perform logout via API with LogOutviaAPI.
- Perform signup via API with SignupFromAPI.
- Avoid using it.only.
- Avoid using after and aftereach in test cases.
- Use multiple assertions for expect statements.
- Avoid using strings for assertions.
- Do not use duplicate filenames even with different paths.
- Avoid using agHelper.Sleep, this.Sleep in any file in code.
🪛 Biome (1.9.4)
app/client/cypress/e2e/Regression/ClientSide/Git/GitSync/MergeViaRemote_spec.ts
[error] 82-82: Unsafe usage of optional chaining.
If it short-circuits with 'undefined' the evaluation will throw TypeError here:
(lint/correctness/noUnsafeOptionalChaining)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-prettier / prettier-check
- GitHub Check: client-build / client-build
- GitHub Check: client-lint / client-lint
🔇 Additional comments (2)
app/client/cypress/e2e/Regression/ClientSide/Git/GitSync/MergeViaRemote_spec.ts (1)
8-13: LGTM! Clean variable declarations with proper typing.app/client/cypress/limited-tests.txt (1)
2-6: LGTM! Test configuration aligns with PR objectives.The changes correctly focus the test suite on Git sync functionality.
app/client/cypress/e2e/Regression/ClientSide/Git/GitSync/MergeViaRemote_spec.ts
Show resolved
Hide resolved
app/client/cypress/e2e/Regression/ClientSide/Git/GitSync/MergeViaRemote_spec.ts
Show resolved
Hide resolved
app/client/cypress/e2e/Regression/ClientSide/Git/GitSync/MergeViaRemote_spec.ts
Show resolved
Hide resolved
brayn003
added
ok-to-test
|
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/12750203169. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
app/client/cypress/limited-tests.txt (2)
7-7: Fix grammatical issue in the commentThe comment is missing an article.
-#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 the minimum of specs. Do not run the entire suite with this command.🧰 Tools
🪛 LanguageTool
[uncategorized] ~7-~7: You might be missing the article “the” here.
Context: ...ile to run minimum of specs. Do not run entire suite with this command.(AI_EN_LECTOR_MISSING_DETERMINER_THE)
Line range hint
1-7: Consider adding a comment explaining the purpose of Widget testsTo improve clarity, consider documenting why Widget tests are included in this limited test set.
# To run only limited tests - give the spec names in below format: #cypress/e2e/Regression/ClientSide/VisualTests/JSEditorIndent_spec.js # For running all specs - uncomment below: #cypress/e2e/**/**/* +# Running Widget tests to ensure core functionality cypress/e2e/Regression/ClientSide/Anvil/Widgets/*🧰 Tools
🪛 LanguageTool
[uncategorized] ~7-~7: You might be missing the article “the” here.
Context: ...ile to run minimum of specs. Do not run entire suite with this command.(AI_EN_LECTOR_MISSING_DETERMINER_THE)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/client/cypress/limited-tests.txt(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
app/client/cypress/limited-tests.txt (1)
Pattern app/client/cypress/**/**.*: Review the following e2e test code written using the Cypress test library. Ensure that:
- Follow best practices for Cypress code and e2e automation.
- Avoid using cy.wait in code.
- Avoid using cy.pause in code.
- Avoid using agHelper.sleep().
- Use locator variables for locators and do not use plain strings.
- Use data-* attributes for selectors.
- Avoid Xpaths, Attributes and CSS path.
- Avoid selectors like .btn.submit or button[type=submit].
- Perform logins via API with LoginFromAPI.
- Perform logout via API with LogOutviaAPI.
- Perform signup via API with SignupFromAPI.
- Avoid using it.only.
- Avoid using after and aftereach in test cases.
- Use multiple assertions for expect statements.
- Avoid using strings for assertions.
- Do not use duplicate filenames even with different paths.
- Avoid using agHelper.Sleep, this.Sleep in any file in code.
📓 Learnings (1)
📓 Common learnings
Learnt from: brayn003
PR: appsmithorg/appsmith#38635
File: app/client/cypress/e2e/Regression/ClientSide/Git/GitSync/MergeViaRemote_spec.ts:102-102
Timestamp: 2025-01-13T15:14:12.806Z
Learning: In Cypress Git sync tests, explicit assertions after _.gitSync.SwitchGitBranch() are not required as the helper method likely handles the verification internally.
Learnt from: brayn003
PR: appsmithorg/appsmith#38635
File: app/client/cypress/e2e/Regression/ClientSide/Git/GitSync/MergeViaRemote_spec.ts:52-58
Timestamp: 2025-01-13T15:15:05.763Z
Learning: The Git sync helper functions (_.gitSync.CommitAndPush, _.gitSync.MergeToMaster) in Cypress tests already include built-in assertions, so additional explicit assertions after these calls are not needed.
🪛 LanguageTool
app/client/cypress/limited-tests.txt
[uncategorized] ~7-~7: You might be missing the article “the” here.
Context: ...ile to run minimum of specs. Do not run entire suite with this command.
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
🔇 Additional comments (1)
app/client/cypress/limited-tests.txt (1)
Line range hint
1-5: Verify alignment with PR objectivesThe changes enable Widget tests while the PR's objective is to fix flakiness in
MergeViaRemote_spec. Consider adding the specific test file path:#cypress/e2e/Regression/ClientSide/VisualTests/JSEditorIndent_spec.js # For running all specs - uncomment below: #cypress/e2e/**/**/* +cypress/e2e/Regression/ClientSide/Git/GitSync/MergeViaRemote_spec.ts cypress/e2e/Regression/ClientSide/Anvil/Widgets/*✅ Verification successful
Focus on MergeViaRemote test instead of Widget tests
Since the PR aims to fix flakiness in MergeViaRemote_spec, replace the Widget tests path with the specific test file:
-cypress/e2e/Regression/ClientSide/Anvil/Widgets/* +cypress/e2e/Regression/ClientSide/Git/GitSync/MergeViaRemote_spec.ts🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify if MergeViaRemote_spec exists fd "MergeViaRemote_spec.ts" "app/client/cypress"Length of output: 126
Script:
#!/bin/bash # Check for any references to MergeViaRemote in Widget test files rg -l "MergeViaRemote" "app/client/cypress/e2e/Regression/ClientSide/Anvil/Widgets/" # Check for any shared test utilities or setup files rg -l "beforeEach|before\(" "app/client/cypress/e2e/Regression/ClientSide/Git/GitSync/MergeViaRemote_spec.ts"Length of output: 272
🧰 Tools
🪛 LanguageTool
[uncategorized] ~7-~7: You might be missing the article “the” here.
Context: ...ile to run minimum of specs. Do not run entire suite with this command.(AI_EN_LECTOR_MISSING_DETERMINER_THE)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/client/src/selectors/gitModSelectors.ts (1)
Line range hint
1-1: Plan removal of temporary file.The comment indicates this is a temporary file. Consider creating a tracking issue for its removal once Git modularization is fully rolled out.
Would you like me to create a GitHub issue to track the removal of this file after the Git modularization rollout?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/client/src/selectors/gitModSelectors.ts(1 hunks)
🧰 Additional context used
📓 Learnings (1)
📓 Common learnings
Learnt from: brayn003
PR: appsmithorg/appsmith#38635
File: app/client/cypress/e2e/Regression/ClientSide/Git/GitSync/MergeViaRemote_spec.ts:102-102
Timestamp: 2025-01-13T15:14:12.806Z
Learning: In Cypress Git sync tests, explicit assertions after _.gitSync.SwitchGitBranch() are not required as the helper method likely handles the verification internally.
Learnt from: brayn003
PR: appsmithorg/appsmith#38635
File: app/client/cypress/e2e/Regression/ClientSide/Git/GitSync/MergeViaRemote_spec.ts:52-58
Timestamp: 2025-01-13T15:15:05.763Z
Learning: The Git sync helper functions (_.gitSync.CommitAndPush, _.gitSync.MergeToMaster) in Cypress tests already include built-in assertions, so additional explicit assertions after these calls are not needed.
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: perform-test / client-build / client-build
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-lint / client-lint
- GitHub Check: client-build / client-build
🔇 Additional comments (1)
app/client/src/selectors/gitModSelectors.ts (1)
24-25: Verify the impact of removing feature flag.The change to always return
trueforces all Git operations to use the new modularized implementation. While this aligns with fixing test flakiness, ensure all consumers are ready for this change.✅ Verification successful
Feature flag removal is safe to proceed
The feature flag has already been effectively removed from active code, and no other conditional checks were found. The imported Git selectors are unrelated to the modularization feature.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining feature flag checks rg -A 2 "release_git_modularisation_enabled" # Search for any imports of the old selectors that might be affected rg "import.*gitSyncSelectors"Length of output: 2315
Description
Fixes flakiness for merge via remote spec
Automation
/ok-to-test tags="@tag.Git"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/12765495211
Commit: 7f83e90
Cypress dashboard.
Tags:
@tag.GitSpec:
Tue, 14 Jan 2025 11:18:09 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit