-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
chore: git mod - test fixes #38357
chore: git mod - test fixes #38357
Conversation
…o chore/git-mod-6
…o chore/git-mod-6
This PR has increased the number of cyclic dependencies by 3, when compared with the release branch. Refer this document to identify the cyclic dependencies introduced by this PR. |
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
🧹 Nitpick comments (2)
app/client/src/pages/Editor/IDE/Layout/AnimatedLayout.tsx (1)
16-16
: Naming clarity recommendation.Renaming the import to
GitProtectedBranchCalloutNew
could cause confusion. If needed, rename the identifier to something more descriptive (e.g.,NewProtectedBranchCallout
) to reflect its purpose clearly.app/client/src/pages/Editor/IDE/Layout/StaticLayout.tsx (1)
17-17
: Clarify the alias name for readabilityRenaming the component alias from
GitProtectedBranchCalloutNew
to something more descriptive—such asGitProtectedBranchBanner
—can make its purpose clearer in the code.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/client/src/pages/Editor/IDE/Layout/AnimatedLayout.tsx
(1 hunks)app/client/src/pages/Editor/IDE/Layout/StaticLayout.tsx
(1 hunks)
🔇 Additional comments (2)
app/client/src/pages/Editor/IDE/Layout/AnimatedLayout.tsx (1)
13-15
: Imports appear consistent.
The import of useGitModEnabled
and useGitProtectedMode
from "pages/Editor/gitSync/hooks/modHooks"
aligns well with the file’s logic. No issues spotted.
app/client/src/pages/Editor/IDE/Layout/StaticLayout.tsx (1)
14-16
: Maintain consistent naming or usage across imports
Currently, useGitModEnabled
and useGitProtectedMode
are imported from modHooks
, while the related GitProtectedBranchCallout
component is alias-imported from git
. Verify that these imports correctly reflect your intended sources.
|
||
if (assertConnect) { | ||
this.assertHelper.AssertNetworkStatus("@importFromGit", 201); | ||
} | ||
} | ||
|
||
public clearBranchProtection() { |
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.
Are these not used anymore?
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.
nope
this.agHelper.GetNClick(this.locators.opsMergeBranchSelectMenu, 0, true); | ||
this.agHelper.AssertContains(destinationBranch); | ||
this.agHelper.GetNClickByContains( | ||
".rc-select-item-option-content", |
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.
Can we use dropdownmenu
from app/client/cypress/locators/commonlocators.json
?
return ( | ||
<GitContextProvider | ||
artifact={application ?? null} | ||
artifactType={artifactType} | ||
artifacts={applications ?? null} |
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.
Why does the git context provider need all the applications?
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.
This is used to show a list of git connected applications in RepoLimitErrorModal
yield put(gitArtifactActions.unmount({ artifactDef })); | ||
yield put( | ||
gitArtifactActions.initGitForEditor({ | ||
artifactDef, | ||
artifact: response.data, | ||
}), | ||
); | ||
yield put(gitArtifactActions.closeDisconnectModal({ artifactDef })); | ||
yield put( | ||
gitArtifactActions.toggleOpsModal({ | ||
artifactDef, | ||
open: false, | ||
tab: GitOpsTab.Deploy, | ||
}), | ||
); | ||
yield put(fetchAllApplicationsOfWorkspace()); |
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.
Does this pose any race condition since we have a bunch of sequential put?
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.
Not really, all of them get realised at the store one after the other. So, this should be fine
This PR has increased the number of cyclic dependencies by 3, when compared with the release branch. Refer this document to identify the cyclic dependencies introduced by this PR. |
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: 1
🧹 Nitpick comments (1)
app/client/cypress/support/Pages/GitSync.ts (1)
Line range hint
285-381
: Consider making timeouts configurableThe branch operations have several hardcoded timeouts that could make tests flaky:
- Line 368: timeout: 500
- Line 376: timeout: 45000
- Line 379: timeout: 45000
Consider moving these values to configuration for better maintainability.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
app/client/cypress/e2e/Regression/Apps/EchoApiCMS_spec.js
(3 hunks)app/client/cypress/e2e/Regression/Apps/MongoDBShoppingCart_spec.ts
(3 hunks)app/client/cypress/e2e/Regression/ClientSide/Git/GitSync/GitConnectV2_spec.ts
(1 hunks)app/client/cypress/support/Objects/CommonLocators.ts
(2 hunks)app/client/cypress/support/Pages/GitSync.ts
(10 hunks)app/client/cypress/tags.js
(1 hunks)app/client/src/selectors/gitModSelectors.ts
(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/client/cypress/support/Objects/CommonLocators.ts
🧰 Additional context used
📓 Path-based instructions (5)
app/client/cypress/tags.js (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/GitConnectV2_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.
app/client/cypress/e2e/Regression/Apps/MongoDBShoppingCart_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.
app/client/cypress/e2e/Regression/Apps/EchoApiCMS_spec.js (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/support/Pages/GitSync.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.
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: perform-test / client-build / client-build
- GitHub Check: perform-test / server-build / server-unit-tests
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-lint / client-lint
- GitHub Check: client-build / client-build
🔇 Additional comments (12)
app/client/src/selectors/gitModSelectors.ts (2)
5-9
: LGTM! Clean import organization.The new imports are well-structured and properly aligned with the new selector implementations.
Also applies to: 13-14
58-76
: Consider consolidating duplicate selector logic.While the implementation is correct, there's significant code duplication between
selectGitOpsModalOpen
andselectGitConnectModalOpen
. However, given this is a temporary file (as noted in the top comment), the current implementation is acceptable.If you decide to refactor, here's a suggested approach:
const createModalSelector = (getNewModalState: any) => createSelector( selectGitModEnabled, getIsGitSyncModalOpen, (state) => getNewModalState(state, selectGitApplicationArtifactDef(state)), (isGitModEnabled, isOldModalOpen, isNewModalOpen) => isGitModEnabled ? isNewModalOpen : isOldModalOpen, ); export const selectGitOpsModalOpen = createModalSelector(selectGitOpsModalOpenNew); export const selectGitConnectModalOpen = createModalSelector(selectGitConnectModalOpenNew);app/client/cypress/support/Pages/GitSync.ts (3)
11-91
: Well-structured locators with consistent naming!The locators object is well-organized with clear, descriptive names and consistent use of data-testid attributes.
93-130
: Clean modal operations with proper assertions!Modal operations follow best practices by:
- Using element visibility assertions instead of cy.wait()
- Following a consistent pattern for opening/closing operations
Line range hint
387-532
: LGTM! Well-structured remote operations.The implementation follows best practices with proper error handling and assertions.
app/client/cypress/tags.js (1)
27-27
: New Git-related tag addition
Your addition of@tag.GitTest
looks good and aligns with the pattern of existing tags.app/client/cypress/e2e/Regression/ClientSide/Git/GitSync/GitConnectV2_spec.ts (1)
17-17
: Consistent tagging approach
Adding@tag.GitTest
is consistent with the broader effort to label Git-related tests.app/client/cypress/e2e/Regression/Apps/EchoApiCMS_spec.js (2)
20-20
: Extended test coverage for Git
Including@tag.GitTest
ensures better categorization of Git-related tests.
117-117
: Effective feature flag usage
ThefeatureFlagIntercept
call helps toggle the modular Git functionality. No issues here.app/client/cypress/e2e/Regression/Apps/MongoDBShoppingCart_spec.ts (3)
2-2
: Feature flag import
ImportingfeatureFlagIntercept
is a good addition to enable feature-specific test behavior.
17-17
: Git tag inclusion
Adding@tag.GitTest
improves traceability of Git-related tests.
203-203
: Feature flag usage for Git
Toggling the Git modularization flag before connecting to Git is well-crafted.
This PR has increased the number of cyclic dependencies by 3, when compared with the release branch. Refer this document to identify the cyclic dependencies introduced by this PR. |
This PR has increased the number of cyclic dependencies by 3, when compared with the release branch. Refer this document to identify the cyclic dependencies introduced by this PR. |
1 similar comment
This PR has increased the number of cyclic dependencies by 3, when compared with the release branch. Refer this document to identify the cyclic dependencies introduced by this PR. |
This comment was marked as duplicate.
This comment was marked as duplicate.
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: 1
🔭 Outside diff range comments (1)
app/client/cypress/support/Pages/GitSync.ts (1)
Line range hint
346-346
: Replace arbitrary wait with assertionUsing
cy.wait(1000)
can make tests flaky. Use Cypress's built-in retry-ability instead.- cy.wait(1000); + this.agHelper.WaitUntilEleAppear(this.locators.branchItem);
🧹 Nitpick comments (2)
app/client/cypress/support/Pages/GitSync.ts (2)
221-225
: Extract timeout to configurationThe hard-coded timeout value should be moved to a configuration constant for better maintainability.
+ private readonly DEFAULT_TIMEOUT = 30000; + public CreateNConnectToGit(...) { ... this.agHelper.AssertElementExist( this.locators.quickActionsCommitBtn, 0, - 30000, + this.DEFAULT_TIMEOUT, );
361-361
: Remove commented codeRemove the commented line to maintain code cleanliness.
- //cy.get(gitSync.locators.branchItem).contains(branch).click();
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app/client/cypress/e2e/Regression/Apps/EchoApiCMS_spec.js
(1 hunks)app/client/cypress/e2e/Regression/Apps/MongoDBShoppingCart_spec.ts
(1 hunks)app/client/cypress/e2e/Regression/ClientSide/Git/GitSync/SwitchBranches_spec.js
(6 hunks)app/client/cypress/support/Pages/GitSync.ts
(10 hunks)app/client/src/sagas/ActionExecution/PluginActionSaga.ts
(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- app/client/cypress/e2e/Regression/Apps/MongoDBShoppingCart_spec.ts
- app/client/cypress/e2e/Regression/ClientSide/Git/GitSync/SwitchBranches_spec.js
- app/client/cypress/e2e/Regression/Apps/EchoApiCMS_spec.js
- app/client/src/sagas/ActionExecution/PluginActionSaga.ts
🧰 Additional context used
📓 Path-based instructions (1)
app/client/cypress/support/Pages/GitSync.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.
📓 Learnings (1)
app/client/cypress/support/Pages/GitSync.ts (2)
Learnt from: sagar-qa007
PR: appsmithorg/appsmith#34955
File: app/client/cypress/e2e/Regression/ClientSide/ActionExecution/General_settings_Spec.ts:14-56
Timestamp: 2024-11-12T08:11:36.416Z
Learning: Avoid using sleep functions like `agHelper.Sleep`, `this.Sleep` in Cypress tests within the `app/client/cypress` directory to prevent non-deterministic behaviors and ensure tests are more reliable and maintainable.
Learnt from: sagar-qa007
PR: appsmithorg/appsmith#34955
File: app/client/cypress/e2e/Regression/ClientSide/ActionExecution/General_settingsTe_Spec.ts:33-33
Timestamp: 2024-11-12T08:11:25.416Z
Learning: Avoid using sleep functions like `agHelper.Sleep` and `this.Sleep` in Cypress tests within the `app/client/cypress` directory to prevent non-deterministic behaviors and ensure tests are more reliable and maintainable.
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: perform-test / client-build / client-build
- GitHub Check: perform-test / server-build / server-unit-tests
- 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/cypress/support/Pages/GitSync.ts (1)
384-384
:⚠️ Potential issueRemove Sleep call
According to the guidelines and past learnings, we should avoid using
agHelper.Sleep()
. Use Cypress's built-in retry-ability or wait until assertions instead.- this.agHelper.Sleep(2000); + this.agHelper.WaitUntilEleAppear(this.locators.quickActionsBranchBtn);⛔ Skipped due to learnings
Learnt from: sagar-qa007 PR: appsmithorg/appsmith#34955 File: app/client/cypress/e2e/Regression/ClientSide/ActionExecution/General_settings_Spec.ts:14-56 Timestamp: 2024-11-12T08:11:36.416Z Learning: Avoid using sleep functions like `agHelper.Sleep`, `this.Sleep` in Cypress tests within the `app/client/cypress` directory to prevent non-deterministic behaviors and ensure tests are more reliable and maintainable.
Learnt from: sagar-qa007 PR: appsmithorg/appsmith#34955 File: app/client/cypress/e2e/Regression/ClientSide/ActionExecution/General_settingsTe_Spec.ts:33-33 Timestamp: 2024-11-12T08:11:25.416Z Learning: Avoid using sleep functions like `agHelper.Sleep` and `this.Sleep` in Cypress tests within the `app/client/cypress` directory to prevent non-deterministic behaviors and ensure tests are more reliable and maintainable.
This comment was marked as duplicate.
This comment was marked as duplicate.
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
🧹 Nitpick comments (1)
app/client/src/git/sagas/deleteBranchSaga.ts (1)
37-42
: Consider adding error toast notifications for consistency.While the success notification is well implemented, consider adding toast notifications for error cases in the catch block for a consistent user experience.
} catch (e) { if (response && response.responseMeta.error) { const { error } = response.responseMeta; + toast.show(error.message, { + kind: "error", + }); yield put(gitArtifactActions.deleteBranchError({ artifactDef, error })); } else { log.error(e); captureException(e); + toast.show("Failed to delete branch", { + kind: "error", + }); } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/client/cypress/e2e/Regression/ClientSide/Git/GitSync/DeleteBranch_spec.js
(5 hunks)app/client/cypress/e2e/Regression/ClientSide/Git/GitSync/GitSyncedApps_spec.js
(5 hunks)app/client/src/git/sagas/deleteBranchSaga.ts
(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- app/client/cypress/e2e/Regression/ClientSide/Git/GitSync/DeleteBranch_spec.js
- app/client/cypress/e2e/Regression/ClientSide/Git/GitSync/GitSyncedApps_spec.js
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: perform-test / server-build / server-unit-tests
- 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 (2)
app/client/src/git/sagas/deleteBranchSaga.ts (2)
15-16
: LGTM! Clean import additions.The new imports are correctly placed with other external dependencies.
Line range hint
1-60
: Verify toast message constant definition.The implementation looks good, but let's verify the
DELETE_BRANCH_SUCCESS
message constant is properly defined.✅ Verification successful
Import path and constant definition verified
The constant is properly defined and the import path is correct. The enterprise edition (ee) re-exports the constant from the community edition (ce), which is the intended pattern.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for DELETE_BRANCH_SUCCESS constant definition rg "DELETE_BRANCH_SUCCESS.*=" -A 2Length of output: 266
Script:
#!/bin/bash # Search for path alias configurations fd -e json tsconfig fd -e js webpack.config fd -e js vite.configLength of output: 826
Script:
#!/bin/bash # Check tsconfig files for path aliases cat app/client/tsconfig.json cat app/client/tsconfig.path.jsonLength of output: 1183
Script:
#!/bin/bash # Check for duplicate definitions in ee directory fd constants/messages -t fLength of output: 28
Script:
#!/bin/bash # Find all potential message files in ce and ee directories rg -l "messages" --type ts app/client/src/ce/ rg -l "messages" --type ts app/client/src/ee/Length of output: 2373
Script:
#!/bin/bash # Check the content of ee messages file cat app/client/src/ee/constants/messages.tsLength of output: 84
Description
Fixes #37821
Fixes #37822
Fixes #37824
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/12649057943
Commit: 94c57d0
Cypress dashboard.
Tags:
@tag.Git
Spec:
Tue, 07 Jan 2025 10:42:55 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Release Notes
New Features
Improvements
Bug Fixes
Testing