fix: Focus Retention Inputs test #36612
Conversation
hetunandu
requested review from
ApekshaBhosale and
sagar-qa007
as code owners
WalkthroughThe pull request introduces modifications to the Cypress test files, specifically updating the way entities are selected in the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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
|
|
/ci-test-limit-count run_count=5 |
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/11103417731. |
![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 (4)
app/client/cypress/e2e/Regression/ClientSide/IDE/Focus_retentions_inputs_spec.js (4)
50-57: Excellent work on updating the entity selection method!I'm impressed to see you've replaced
EditorNavigation.SelectEntityByNamewithPageLeftPane.selectItem. This change aligns perfectly with our best practices for Cypress code.However, I noticed you're still using
cy.wait(1000)in several places. Remember, class, we should avoid using fixed waits as they can make our tests flaky. Let's try to replace these with more reliable waiting strategies, such as waiting for specific elements to be visible or enabled.Here's a little homework for you:
- Replace
cy.wait(1000)with appropriate Cypress commands that wait for specific elements or states.- Consider using
cy.intercept()to wait for network requests instead of arbitrary timeouts.Keep up the great work, and let me know if you need any help with these improvements!
Also applies to: 64-69, 79-81, 87-87
103-104: Well done on maintaining consistency in your test cases!I'm pleased to see you've continued to use
PageLeftPane.switchSegmentandPageLeftPane.selectItemthroughout this test case. This consistency will make our test suite easier to maintain and understand.One small suggestion for improvement:
Instead of usingxpathselectors likecy.xpath("//span[contains(text(), 'Params')]/parent::button"), consider using data attributes for more reliable and maintainable selectors. For example:cy.get('[data-cy="params-tab"]').should('have.attr', 'aria-selected', 'true');This change would align better with our coding guidelines and make our tests more robust. What do you think about implementing this change?
Also applies to: 106-106, 115-124, 134-134, 141-141, 148-151, 155-155
Line range hint
169-180: Good effort on updating this test case, but let's make it even better!I appreciate the minor updates you've made to this test case. However, I noticed we're still using xpath selectors, which isn't in line with our best practices. Let's work on improving this:
- Replace
cy.xpath(queryLocators.querySettingsTab)with a data attribute selector, likecy.get('[data-cy="query-settings-tab"]').- Instead of using
agHelper.GetElement(dataSources._usePreparedStatement), consider using a more specific Cypress command with a data attribute.Remember, using data attributes makes our tests more reliable and easier to maintain. Can you try implementing these changes?
Line range hint
1-203: Overall, a commendable effort in improving our test suite!Class, you've done a great job in updating our focus retention input tests. Here's a summary of your achievements and areas for further improvement:
Achievements:
- Consistent use of
PageLeftPane.selectItemfor entity selection.- Improved test coverage by combining related bug fix tests.
- General adherence to our coding guidelines.
Areas for improvement:
- Replace fixed waits (
cy.wait(1000)) with more reliable waiting strategies.- Use data attributes instead of xpath selectors for more robust element selection.
- Consider using
cy.intercept()for waiting on network requests.For your next assignment, I'd like you to focus on addressing these areas for improvement. This will further enhance the reliability and maintainability of our test suite.
Keep up the excellent work, and don't hesitate to ask if you need any clarification on these concepts!
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- app/client/cypress/e2e/Regression/ClientSide/IDE/Focus_retentions_inputs_spec.js (8 hunks)
- app/client/cypress/limited-tests.txt (1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
app/client/cypress/e2e/Regression/ClientSide/IDE/Focus_retentions_inputs_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/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 (5)
app/client/cypress/limited-tests.txt (2)
2-2: Well done, class! This change aligns perfectly with our lesson plan.I'm pleased to see that you've updated the test specification to focus on the 'Focus_retentions_inputs_spec.js'. This change is in line with our PR objectives to address the flaky test issue. It's like you've sharpened your pencil to write more precise answers!
Remember, children, in the world of testing, focus is key. By zeroing in on this specific test, we're ensuring that our application's focus retention inputs are working as smoothly as a well-oiled abacus.
Line range hint
1-8: A gold star for maintaining a tidy and informative file structure!Class, let's take a moment to appreciate the neat organization of this file. It's like a well-arranged classroom, where everything has its place:
- We have clear instructions at the top, just like the rules on our classroom wall.
- There's a commented-out option for running all specs - it's like having a spare eraser, always good to keep handy!
- The note about 'ci-test-limit' is our friendly reminder, much like the bell that signals the end of recess.
This structure helps everyone understand how to use the file, which is crucial for teamwork. Remember, in the world of coding, clear communication is as important as in our classroom discussions!
app/client/cypress/e2e/Regression/ClientSide/IDE/Focus_retentions_inputs_spec.js (3)
9-11: Good job on updating the imports, class!I see you've added new imports for PageLeftPane and PagePaneSegment. These additions are in line with the changes made in the test cases. Keep up the good work!
160-160: Good job on keeping this test case intact!I see you've renumbered this test case from 4 to 3, which helps maintain the logical flow of our test suite. The core functionality of checking the datasource edit mode remains unchanged, which is excellent. Keep up the good work in maintaining the integrity of our tests!
187-187: Excellent work on improving our test coverage!I'm very impressed with how you've combined the test cases for bug fixes 21999 and 22960. This is a great example of efficient testing, addressing multiple related issues in a single, comprehensive test case.
Your approach here demonstrates a good understanding of how to write effective and maintainable tests. Keep up this level of thoughtfulness in your test writing!
|
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/11103417731. |
|
/ci-test-limit-count run_count=5 |
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/11104510119. |
|
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/11104510119. |
|
/ci-test-limit-count run_count=5 runId=11104510119 |
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/11105339080. |
|
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/11105339080. |
…t' into fix/maintain-focus-retention-test
|
/ci-test-limit-count run_count=5 |
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/11119791735. |
|
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/11119791735. |
|
/ci-test-limit-count run_count=5 runId=11119791735 |
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/11121582280. |
|
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/11121582280. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
app/client/cypress/e2e/Regression/ClientSide/IDE/Focus_retentions_inputs_spec.js (2)
60-60: Good job updating our API interactions, students!I'm impressed with your use of new helper methods. It's like using better tools to solve a math problem - it makes everything clearer and easier to understand.
However, remember our class rule: we don't use cy.wait() or cy.pause() in our tests. It's like using a calculator when we're supposed to show our work. Can you double-check to make sure we're not using these anywhere?
Also applies to: 103-104, 106-110, 119-128, 130-132
Line range hint
179-195: Good effort on the new form control inputs test!Your new test case is like adding a practical experiment to our science class - it helps us understand how things really work. I'm pleased to see you using our new helper methods consistently.
However, I spotted something we need to fix. On line 192, you've used cy.wait(1000). Remember, using cy.wait() is like taking a shortcut in your work - it might seem easier, but it's not the best way to do things. Can you find a better way to handle this without using cy.wait()?
Please replace the cy.wait(1000) with a more appropriate waiting mechanism, such as waiting for a specific element to appear or an action to complete.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- app/client/cypress/e2e/Regression/ClientSide/IDE/Focus_retentions_inputs_spec.js (8 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
app/client/cypress/e2e/Regression/ClientSide/IDE/Focus_retentions_inputs_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.
🔇 Additional comments (6)
app/client/cypress/e2e/Regression/ClientSide/IDE/Focus_retentions_inputs_spec.js (6)
9-11: Very good, class! You've added new imports.I'm pleased to see you've imported PageLeftPane and PagePaneSegment. These new imports will help us organize our code better. Keep up the good work!
17-17: Excellent job updating the test suite description!Your new description, "Focus Retention of Inputs", is much more precise. It's like writing a clear title for your essay - it helps everyone understand what we're testing right away. Well done!
Line range hint
170-177: Excellent addition to our test suite!Adding this new test case for datasource edit mode is like adding a new chapter to our textbook - it covers an important topic we weren't testing before. Great job on improving our test coverage!
Line range hint
197-211: Fantastic work on the code editor focus test!This new test case is like solving a tricky word problem in math class - it tackles some specific, challenging scenarios. I'm impressed that you've addressed two different bug situations here. This kind of detailed testing will really help improve our code editor's usability.
Keep up the excellent work, and remember to always think about these edge cases in your future tests too!
142-144: Nice work on updating our query selection and assertions!Your use of PageLeftPane.selectItem() and PageLeftPane.switchSegment() is like using a good outline for your essay - it keeps everything organized and easy to follow.
But let's make sure these new methods are as reliable as our old ones. Can you run a few extra tests to make sure everything works just as well as before?
#!/bin/bash # Description: Verify the reliability of new query selection and assertion methods # Test: Check if PageLeftPane.selectItem consistently selects the correct query rg --type js 'PageLeftPane\.selectItem\(' app/client/cypress/e2e/Regression/ClientSide/IDE/Focus_retentions_inputs_spec.js -A 5 -B 5 # Test: Verify that assertions after PageLeftPane.switchSegment are consistent rg --type js 'PageLeftPane\.switchSegment\(' app/client/cypress/e2e/Regression/ClientSide/IDE/Focus_retentions_inputs_spec.js -A 10 -B 5Also applies to: 151-151, 158-158, 161-161, 165-165
50-50: A for effort on updating your entity selection methods!You've done a great job replacing EditorNavigation.SelectEntityByName() with PageLeftPane.selectItem(). This change makes our code more consistent, like using the same format for all your homework assignments.
However, let's make sure this change doesn't affect how well our tests work. Can you please run these tests a few times to make sure they're still reliable?
Also applies to: 57-57, 64-64, 69-69, 79-81, 87-87
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
app/client/cypress/e2e/Regression/ClientSide/IDE/Focus_retentions_inputs_spec.js (2)
50-50: Excellent use of the new PageLeftPane methods!I'm impressed with how you've updated the entity selection process, students. Using
PageLeftPane.selectItemandPageLeftPane.switchSegmentmakes our test more consistent and easier to read. However, let's make one small improvement:Consider adding a short comment before each
PageLeftPane.selectItemcall to explain what entity type we're selecting. For example:// Select REST API PageLeftPane.selectItem("Rest_Api_1");This will help your classmates understand the purpose of each selection at a glance.
Also applies to: 57-57, 64-64, 69-69, 71-71, 77-77
93-94: Well done on updating the assertions and selections!You've done a great job applying the new
PageLeftPanemethods consistently throughout this test case. I particularly like how you've updated the assertions to useagHelper. That's a gold star for you!To make our test even more robust, let's add a small wait after each
PageLeftPane.selectItemcall. This will ensure that the UI has fully updated before we make our assertions. Here's an example:PageLeftPane.selectItem("Rest_Api_1"); cy.wait(500); // Short wait for UI updateRemember, class, patience is a virtue in testing!
Also applies to: 96-100, 109-120, 132-134, 141-141, 144-144, 148-148
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- app/client/cypress/e2e/Regression/ClientSide/IDE/Focus_retentions_inputs_spec.js (7 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
app/client/cypress/e2e/Regression/ClientSide/IDE/Focus_retentions_inputs_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.
🔇 Additional comments (5)
app/client/cypress/e2e/Regression/ClientSide/IDE/Focus_retentions_inputs_spec.js (5)
9-11: Good job updating the imports and test suite description!Class, I'm pleased to see that you've added the necessary imports for
PageLeftPaneandPagePaneSegment. These new tools will help us navigate our test environment more effectively. Also, updating the describe block to "Focus Retention of Inputs" gives us a clearer picture of what we're testing. Keep up the good work!Also applies to: 17-17
Line range hint
153-160: Good job maintaining this test case!Class, this is an excellent example of "if it ain't broke, don't fix it." This test case for datasource edit mode retention is well-structured and doesn't need any changes. It's important to recognize when our existing code is serving its purpose effectively.
Line range hint
162-173: Nice work on refining this test case!I see you've made some subtle but important changes here. Using
agHelper.GetElementfor assertions is a step in the right direction. It's like sharpening your pencil before an exam - it makes everything that follows smoother and more precise.
Line range hint
180-192: Excellent attention to detail in updating this test case!Class, pay attention to how this test case has been updated. The new description clearly indicates that it's addressing two specific bugs. This is like writing a clear title for your essay - it helps anyone reading your code understand its purpose immediately.
The test logic itself remains unchanged, which is perfect as it was already well-structured to test the described behaviors. Remember, good code is like a good essay - clear, concise, and to the point!
Line range hint
1-193: Excellent work on updating this test file!Class, I want you all to take a good look at this test file. It's a shining example of how to improve your code while maintaining its core functionality.
Here's what makes this update stand out:
- Consistent use of new
PageLeftPanemethods for navigation and selection.- Clear and descriptive test case titles that explain the purpose of each test.
- Improved assertions using
agHelper, making our tests more robust.- Maintained the overall structure and purpose of the tests while enhancing readability.
These changes are like polishing a well-written essay - they make the good work you've already done shine even brighter. Keep up this level of attention to detail in all your coding assignments!
Description
Fix for Flaky MaintainFocus&Retention_spec.js
Automation
/ok-to-test tags="@tag.IDE"
🔍 Cypress test results
Caution
🔴 🔴 🔴 Some tests have failed.
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/11155677720
Commit: 375ada5
Cypress dashboard.
Tags: @tag.IDE
Spec:
The following are new failures, please fix them before merging the PR:
- cypress/e2e/Regression/ClientSide/IDE/Focus_retentions_inputs_spec.js
List of identified flaky tests.Thu, 03 Oct 2024 06:00:12 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Bug Fixes
Chores
Focus_retentions_inputs_spec.jsinstead of theFork_Template_spec.js.