fix: Fix shareapp case due to load issue #36627
Conversation
WalkthroughThe pull request introduces changes primarily to the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
🪧 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=25 |
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/11121106106. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
app/client/cypress/limited-tests.txt (1)
Line range hint
1-7: Class, let's discuss the importance of this configuration file.Students, this file plays a crucial role in our testing strategy. It allows us to run a subset of tests, which is particularly useful for quick checks and our CI/CD pipeline. Remember, as good developers, we should:
- Use this file responsibly, running only the necessary tests for each scenario.
- Keep the comments up-to-date to guide future developers.
- Be cautious when modifying this file, as it directly impacts our continuous integration process.
As we progress in our development journey, we might want to consider automating the process of updating this file based on recent changes or test priorities. This could enhance our testing efficiency and ensure we're always testing the most critical parts of our application.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- app/client/cypress/e2e/Regression/ClientSide/Workspace/ShareAppTests_Spec.ts (2 hunks)
- app/client/cypress/limited-tests.txt (1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
app/client/cypress/e2e/Regression/ClientSide/Workspace/ShareAppTests_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/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.
🔇 Additional comments (3)
app/client/cypress/limited-tests.txt (1)
2-2: Class, let's examine this change closely.Now, students, we've updated our test specification from a JavaScript file to a TypeScript file. This is a positive step towards more robust type checking in our tests. However, we must ensure that all developers are aware of this transition.
Let's verify if this change is consistent across our testing suite:
Class, this script will help us understand if this change is part of a larger migration or an isolated update.
app/client/cypress/e2e/Regression/ClientSide/Workspace/ShareAppTests_Spec.ts (2)
12-12: Excellent work on importing thegitSyncLocatorsmodule!By importing
gitSyncLocators, you're promoting code reusability and maintainability. This approach centralizes your locators, making it easier to manage and update them across your tests.
117-117: Let's ensure we're using data- attributes for selectors*In your assertion
agHelper.AssertElementVisibility(gitSyncLocators.connectGitBottomBar);, it's important to verify that the locatorconnectGitBottomBarutilizes data-* attributes. Following this practice enhances the stability and reliability of your tests.To confirm that
connectGitBottomBaruses a data-* attribute, let's run the following script:
|
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/11121106106. |
|
Don't think it is a good idea to use a git element in a test that is not related to git. Probably use other indicators like canvas element appearing or consolidated-api returning 200 |
|
/ci-test-limit-count run_count=25 |
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/11162710118. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
app/client/cypress/e2e/Regression/ClientSide/Workspace/ShareAppTests_Spec.ts (2)
117-117: Excellent addition to our test suite!I'm pleased to see you've added an assertion to check the network status. This is a great way to ensure our application is behaving correctly. However, let's make it even better. Instead of using a hard-coded status code, we could use a constant. This would make our test more maintainable. Here's how we could improve it:
const HTTP_OK_STATUS = 200; assertHelper.AssertNetworkStatus("@getConsolidatedData", HTTP_OK_STATUS, true);This way, if we ever need to change the expected status code, we only need to update it in one place. What do you think about this suggestion?
Line range hint
1-150: Let's review our test practices, class.While our test suite is well-structured, there are a few areas where we can improve to align better with our coding guidelines:
I noticed we're using
cy.waitin several places. Remember, we want to avoid usingcy.waitas it can make our tests flaky. Instead, let's use Cypress's built-in retry-ability and wait for specific elements or network requests to resolve.We're using string selectors directly in some places. It's better to use locator variables for our selectors. This makes our tests more maintainable and easier to update if our UI changes.
Great job on using
assertHelper.AssertNetworkStatus! This is a good practice for checking network responses.I don't see any use of
cy.pause,agHelper.sleep(), orit.only, which is excellent!We're correctly using
LoginFromAPIandLogOutviaAPI, which aligns with our guidelines.Here's an example of how we could improve one of our selectors:
// Instead of this: agHelper.GetNClick(homePage._sharePublicToggle, 0, true); // We could define a locator variable: const sharePublicToggle = '[data-cy="share-public-toggle"]'; agHelper.GetNClick(sharePublicToggle, 0, true);Let's work on refactoring our selectors and removing
cy.waitcalls. This will make our tests more robust and easier to maintain. What do you think about these suggestions?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- app/client/cypress/e2e/Regression/ClientSide/Workspace/ShareAppTests_Spec.ts (2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
app/client/cypress/e2e/Regression/ClientSide/Workspace/ShareAppTests_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.
🔇 Additional comments (1)
app/client/cypress/e2e/Regression/ClientSide/Workspace/ShareAppTests_Spec.ts (1)
12-12: Class, let's discuss the new import statement.I see you've added a new import for
gitSyncLocators. While it's good to keep our imports organized, I don't see where these locators are being used in our test suite. Remember, it's important to only import what we need. Can you show me where you're using these locators, or if they're not being used, could you explain why we're importing them?Let's run a quick check to see if these locators are being used:
✅ Verification successful
Great observation!
I've checked the usage of
gitSyncLocatorsinShareAppTests_Spec.tsand found that it's imported but not utilized anywhere in the file. It's a good practice to remove any unused imports to keep the codebase clean and maintainable.
- File:
app/client/cypress/e2e/Regression/ClientSide/Workspace/ShareAppTests_Spec.ts- Line: 12
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usage of gitSyncLocators in the file rg 'gitSyncLocators' app/client/cypress/e2e/Regression/ClientSide/Workspace/ShareAppTests_Spec.tsLength of output: 165
|
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/11162710118. |
sagar-qa007
added
ok-to-test
|
@brayn003 Please check and approve now. Have updated the code. Seems like application page got blank in one instance. Kindly ignore that failure. 
|
|
/ci-test-limit-count run_count=25 runId=11162710118 |
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/11166070561. |
|
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/11166070561. |
Description
Fixes #
36626Issue: Script was clicking on share button without full page load.
Solution: Waited for page load with api.
EE PR: https://github.com/appsmithorg/appsmith-ee/pull/5285
Automation
/ok-to-test tags="@tag.Sanity"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/11174778224
Commit: 80be7e1
Cypress dashboard.
Tags:
@tag.SanitySpec:
Fri, 04 Oct 2024 07:01:59 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
Bug Fixes