test: updated git tests for regression#39616
test: updated git tests for regression#39616NandanAnantharamu wants to merge 2 commits intoreleasefrom
Conversation
NandanAnantharamu
requested review from
ApekshaBhosale and
sagar-qa007
as code owners
WalkthroughA new test suite for Git branch functionalities has been added to validate branch creation, deletion, and merging (including conflict, queries, JS objects, and theme changes). The limited tests configuration was updated to include the new suite while excluding certain others. Additionally, a new locator and a public method for preventing duplicate branch creation were introduced in the GitSync module, enhancing branch management through UI interactions. Changes
Sequence Diagram(s)sequenceDiagram
participant TS as Test Suite
participant GS as GitSync
participant UI as User Interface
TS->>GS: invoke verifyNoDuplicateBranch(branch)
GS->>UI: Click quickActionsBranchBtn
UI-->>GS: Display branchSearchInput
GS->>UI: Type branch name (base + GUID)
UI-->>GS: Update input field
GS->>UI: Check absence of createNewBranchButton
UI-->>GS: Return not visible
GS->>UI: Click branchCloseBtn
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🪧 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
|
|
/ci-test-limit-count run_count=10 |
github-actions
bot
added
skip-changelog
![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
🧹 Nitpick comments (3)
app/client/cypress/support/Pages/GitSync.ts (1)
91-91: Use data- attribute for selector instead of CSS class*The locator is using a CSS class selector instead of a data-* attribute as recommended in the coding guidelines.
- createNewBranchButton: ".t--create-new-branch-button", + createNewBranchButton: "[data-testid='t--create-new-branch-button']",app/client/cypress/e2e/Regression/ClientSide/Git/MergeBranch_spec.ts (2)
104-148: Avoid using force:true in click operationsUsing
force:true(lines 108, 111, etc.) indicates potential issues with element visibility or interactability. This can lead to flaky tests.Use proper waiting mechanisms or element visibility assertions before interacting with elements instead of forcing interactions.
24-30: Add after block for cleanupThe test has a before() hook but is missing an after() hook for cleanup. This could leave resources that affect other tests.
describe("Git Branch:", {}, function () { before(() => { gitSync.CreateNConnectToGit(); cy.get("@gitRepoName").then((repName) => { repoName = repName; }); }); + + after(() => { + // Clean up resources here, e.g., delete created branches + });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/client/cypress/e2e/Regression/ClientSide/Git/MergeBranch_spec.ts(1 hunks)app/client/cypress/limited-tests.txt(1 hunks)app/client/cypress/support/Pages/GitSync.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`app/client/cypress/**/**.*`: Review the following e2e test ...
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/limited-tests.txtapp/client/cypress/e2e/Regression/ClientSide/Git/MergeBranch_spec.tsapp/client/cypress/support/Pages/GitSync.ts
🧠 Learnings (2)
app/client/cypress/e2e/Regression/ClientSide/Git/MergeBranch_spec.ts (4)
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#36622
File: app/client/cypress/e2e/Regression/ClientSide/Git/GitPersistBranch_spec.ts:41-45
Timestamp: 2024-11-12T08:11:36.416Z
Learning: In `app/client/cypress/**/**.*` Cypress test files, the cleanup guideline is to use `after` hooks for cleanup tasks.
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.
Learnt from: brayn003
PR: appsmithorg/appsmith#38635
File: app/client/cypress/e2e/Regression/ClientSide/Git/GitSync/MergeViaRemote_spec.ts:81-82
Timestamp: 2025-01-13T15:14:42.085Z
Learning: In Cypress test files, unsafe optional chaining is acceptable when the intention is for the test to fail if the expected data structure is not present, as it helps validate the API response structure.
app/client/cypress/support/Pages/GitSync.ts (1)
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.
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-lint / client-lint
- GitHub Check: client-prettier / prettier-check
- GitHub Check: client-build / client-build
🔇 Additional comments (4)
app/client/cypress/limited-tests.txt (2)
2-2: LGTM - Added new MergeBranch spec to limited testsThe addition of the MergeBranch_spec.ts to the limited tests looks good.
5-5: LGTM - Commented out Widgets testsProperly commented out the Widgets test path from the limited tests.
app/client/cypress/support/Pages/GitSync.ts (1)
541-564: LGTM - New utility method for branch verificationThe method implementation looks good. It properly checks if a branch with the same name already exists by verifying the absence of the create button.
I notice you're using the retrieved learning to properly implement this method without unnecessary assertions after SwitchGitBranch().
app/client/cypress/e2e/Regression/ClientSide/Git/MergeBranch_spec.ts (1)
32-35: LGTM - Test validates duplicate branch creationGood test case that verifies a branch with the same name cannot be created twice.
| demoBranch = branchName; | ||
| gitSync.SwitchGitBranch("master"); | ||
| DeleteBranchFromUI(demoBranch); | ||
| cy.wait(5000); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace explicit wait with proper assertions
Using explicit waits like cy.wait(5000) is against the coding guidelines. Replace with appropriate assertions or waitUntil.
- cy.wait(5000);
+ agHelper.AssertElementVisibility(gitSync.locators.quickActionsBranchBtn);
+ // Or use a more specific assertion that confirms the UI is ready📝 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.
| cy.wait(5000); | |
| agHelper.AssertElementVisibility(gitSync.locators.quickActionsBranchBtn); | |
| // Or use a more specific assertion that confirms the UI is ready |
| gitSync.CreateGitBranch("Demo1", true); | ||
| PageList.AddNewPage(); | ||
| gitSync.CommitAndPush(); | ||
| cy.wait(3000); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace explicit wait with proper assertions
Using explicit waits like cy.wait(3000) is against the coding guidelines. Replace with appropriate assertions or waitUntil.
- cy.wait(3000);
+ agHelper.WaitUntilElementVisible(gitSync.locators.quickActionsCommitBtn);
+ // Or use a more specific assertion that confirms the UI is ready📝 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.
| cy.wait(3000); | |
| agHelper.WaitUntilElementVisible(gitSync.locators.quickActionsCommitBtn); | |
| // Or use a more specific assertion that confirms the UI is ready |
| function DeleteBranchFromUI(branch: any) { | ||
| cy.get(gitSync.locators.quickActionsBranchBtn).click(); | ||
| agHelper.HoverElement( | ||
| `${gitSync.locators.branchItem}:contains('${branch}')`, | ||
| ); | ||
| cy.get(gitSync.locators.branchItemMenuBtn).click({ force: true }); | ||
| cy.get(gitSync.locators.branchItemMenuDeleteBtn) | ||
| .should("be.visible") | ||
| .click({ force: true }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use data- attributes in DeleteBranchFromUI function*
The function uses CSS contains selector rather than data attributes, which goes against guidelines.
function DeleteBranchFromUI(branch: any) {
cy.get(gitSync.locators.quickActionsBranchBtn).click();
- agHelper.HoverElement(
- `${gitSync.locators.branchItem}:contains('${branch}')`,
- );
+ // Use a more appropriate selector without :contains
+ agHelper.HoverElement(gitSync.locators.branchItem);
+ // Use agHelper methods that target elements by text content instead
cy.get(gitSync.locators.branchItemMenuBtn).click({ force: true });
cy.get(gitSync.locators.branchItemMenuDeleteBtn)
.should("be.visible")
.click({ force: true });
}Committable suggestion skipped: line range outside the PR's diff.
|
/ci-test-limit-count run_count=10 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
app/client/cypress/e2e/Regression/ClientSide/Git/MergeBranch_spec.ts (2)
23-23: Add a specific type definition fordemoBranchReplace
anywith a more specific type definition to improve type safety and code readability.-let demoBranch: any; +let demoBranch: string;
47-50: Implement or remove commented codeThe commented code related to verifying that a checked-out branch cannot be deleted should either be implemented or removed to maintain clean code.
- // Verify cannot delete checked out branch - // DeleteBranchFromUI(demoBranch); - // agHelper.ValidateToastMessage("Cannot delete checked out branch."); - // cy.wait(5000) + // TODO: Implement test for verifying that a checked-out branch cannot be deleted
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (1)
app/client/cypress/e2e/Regression/ClientSide/Git/MergeBranch_spec.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`app/client/cypress/**/**.*`: Review the following e2e test ...
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/MergeBranch_spec.ts
🧠 Learnings (1)
app/client/cypress/e2e/Regression/ClientSide/Git/MergeBranch_spec.ts (4)
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#36622
File: app/client/cypress/e2e/Regression/ClientSide/Git/GitPersistBranch_spec.ts:41-45
Timestamp: 2024-11-12T08:11:36.416Z
Learning: In `app/client/cypress/**/**.*` Cypress test files, the cleanup guideline is to use `after` hooks for cleanup tasks.
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.
Learnt from: brayn003
PR: appsmithorg/appsmith#38635
File: app/client/cypress/e2e/Regression/ClientSide/Git/GitSync/MergeViaRemote_spec.ts:81-82
Timestamp: 2025-01-13T15:14:42.085Z
Learning: In Cypress test files, unsafe optional chaining is acceptable when the intention is for the test to fail if the expected data structure is not present, as it helps validate the API response structure.
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: client-build / client-build
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-prettier / prettier-check
- GitHub Check: client-lint / client-lint
🔇 Additional comments (3)
app/client/cypress/e2e/Regression/ClientSide/Git/MergeBranch_spec.ts (3)
44-44: Replace explicit wait with proper assertionsUsing explicit waits like
cy.wait(5000)is against the coding guidelines. Replace with appropriate assertions or waitUntil.- cy.wait(5000); + agHelper.AssertElementVisibility(gitSync.locators.quickActionsBranchBtn); + // Or use a more specific assertion that confirms the UI is ready
58-58: Replace explicit wait with proper assertionsUsing explicit waits like
cy.wait(3000)is against the coding guidelines. Replace with appropriate assertions or waitUntil.- cy.wait(3000); + agHelper.WaitUntilElementVisible(gitSync.locators.quickActionsCommitBtn); + // Or use a more specific assertion that confirms the UI is ready
150-159: Use data- attributes in DeleteBranchFromUI function*The function uses CSS contains selector rather than data attributes, which goes against guidelines.
function DeleteBranchFromUI(branch: any) { cy.get(gitSync.locators.quickActionsBranchBtn).click(); - agHelper.HoverElement( - `${gitSync.locators.branchItem}:contains('${branch}')`, - ); + // Use a more appropriate selector without :contains + agHelper.HoverElement(gitSync.locators.branchItem); + // Use agHelper methods that target elements by text content instead cy.get(gitSync.locators.branchItemMenuBtn).click({ force: true }); cy.get(gitSync.locators.branchItemMenuDeleteBtn) .should("be.visible") .click({ force: true }); }
| cy.wrap(demoBranch).as("demoBranch"); | ||
| demoBranch = branchName; | ||
| gitSync.SwitchGitBranch("master"); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix the variable wrapping and assignment logic
The current implementation appears to be wrapping a variable before it's assigned, which is incorrect.
- cy.wrap(demoBranch).as("demoBranch");
- demoBranch = branchName;
+ demoBranch = branchName;
+ cy.wrap(demoBranch).as("demoBranch");📝 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.
| cy.wrap(demoBranch).as("demoBranch"); | |
| demoBranch = branchName; | |
| gitSync.SwitchGitBranch("master"); | |
| demoBranch = branchName; | |
| cy.wrap(demoBranch).as("demoBranch"); | |
| gitSync.SwitchGitBranch("master"); |
|
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/13727675969. |
|
/ci-test-limit-count run_count=10 |
|
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/13780788299. |
|
This PR has not seen activitiy for a while. It will be closed in 7 days unless further activity is detected. |
|
This PR has been closed because of inactivity. |
1 participant
Summary by CodeRabbit
New Features
Tests
Chores
Warning
Tests have not run on the HEAD eca67f0 yet
Fri, 07 Mar 2025 19:15:15 UTC