fix: Focus retention on inputs #36770
Conversation
…t' into fix/maintain-focus-retention-test
WalkthroughThe pull request modifies the test suite for focus retention in the application. It updates the test suite name and alters the methods used for selecting entities, emphasizing the left pane interaction. The Changes
Assessment against linked issues
Possibly related issues
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
|
github-actions
bot
added
Bug
![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 (3)
app/client/cypress/e2e/Regression/ClientSide/IDE/Focus_retentions_inputs_spec.js (3)
50-50: Good job updating the test case with new methods!Class, let's look at how your classmate has improved this test case. They've replaced
EditorNavigation.SelectEntityByName()withPageLeftPane.selectItem(). This is like using a more precise tool in our science experiments. It's clearer and easier to understand.However, I have a small suggestion to make it even better:
Consider adding a short comment before each
PageLeftPane.selectItem()call to explain what entity is being selected. This will help your classmates understand the test flow more easily.For example:
// Select REST API 1 PageLeftPane.selectItem("Rest_Api_1");Keep up the good work!
Also applies to: 57-57, 64-64, 69-69, 79-81, 87-87
103-104: Excellent work on improving the second test case!Class, pay attention to how your classmate has enhanced this test case. They've used
PageLeftPane.switchSegment()andPageLeftPane.selectItem()to navigate through different parts of the application. This is like following a clear map in our classroom experiments.The new assertions using
agHelper.GetElement()are like double-checking our results, which is very important in scientific testing.However, I have a small suggestion to make it even better:
Consider adding a short wait time after each
PageLeftPane.selectItem()call to ensure the UI has fully updated before making assertions. You can usecy.wait(1000)for this. For example:PageLeftPane.selectItem("Rest_Api_1"); cy.wait(1000); // Wait for UI to update agHelper.GetElement(locators._queryName).should("have.text", "Rest_Api_1");This will make our tests more reliable, just like how we wait for our chemical reactions to complete before observing the results. Keep up the excellent work!
Also applies to: 106-110, 119-132, 142-144, 151-151, 158-158, 161-161, 165-165
Line range hint
179-195: Good effort on the fourth test case, but let's make it even better!Class, your classmate has made some good improvements to this test case. They've used
EditorNavigation.SelectEntityByName()for selecting queries, which is consistent with our previous examples. The new assertions usingagHelper.GetElement()are like double-checking our experiment results, which is excellent scientific practice.However, I've noticed something we need to improve. Can anyone spot the issue? That's right - we're using xpath selectors in a few places. Remember our class rule: we should avoid using xpath selectors. Let's fix this together:
- Replace
cy.xpath(queryLocators.querySettingsTab)with a more appropriate selector, preferably using data-* attributes.- Instead of
cy.xpath(queryLocators.queryTimeout), let's use a more reliable selector.For example:
cy.get('[data-cy="query-settings-tab"]').click(); cy.get('[data-cy="query-timeout-input"]').should("be.focused");By making these changes, our test will be more robust and easier to maintain, just like keeping our lab equipment in good condition. Keep up the good work, and remember to always follow our class rules!
📜 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 (5)
app/client/cypress/e2e/Regression/ClientSide/IDE/Focus_retentions_inputs_spec.js (5)
9-11: Well done on updating the imports and test suite description!Class, I'm pleased to see that you've made some thoughtful changes to the imports and the test suite description. The new imports for
PageLeftPaneandPagePaneSegmentwill be useful for our upcoming lessons on page navigation. The updated test suite name, "Focus Retention of Inputs," is much clearer and tells us exactly what we're testing. Good job on adding the@tag.IDEtag too - it's like putting the right label on your notebook!Also applies to: 17-17
19-20: Excellent simplification of the test setup!Children, take note of how your classmate has simplified the test setup. Instead of creating a new workspace, we're now using
homePage.NavigateToHome(). This is like starting with a clean slate before each test, which is very important in our experiments. It's a great example of keeping things simple and following our class rules for Cypress tests. Well done!
Line range hint
170-177: Good job on updating the third test case!Class, let's appreciate how your classmate has updated this test case. They've used
EditorNavigation.SelectEntityByName()consistently for selecting datasources. This is like using the same measuring tool throughout an experiment, which helps us avoid confusion.The test flow is clear and easy to follow, just like a well-written lab report. It checks if the datasource edit mode is maintained correctly, which is an important detail in our application.
Keep up the good work! Your attention to consistency is commendable.
Line range hint
197-211: Excellent work on the fifth test case!Class, let's give a round of applause to your classmate for this well-crafted test case. They've done a fantastic job addressing two specific bugs: maintaining focus when the Escape key is pressed with autocomplete open, and when it's pressed in general.
The use of
EditorNavigation.SelectEntityByName()for JSObject selection is consistent with our previous examples, which is great. It's like using the same scientific method across different experiments.I'm particularly impressed with the use of
cy.assertCursorOnCodeInput(). This is a very precise way of checking where our cursor is, just like using a microscope to observe tiny details in our lab work.The step-by-step approach in this test case is clear and logical:
- Select the JSObject
- Assert the initial cursor position
- Type some text
- Check for autocomplete
- Press Escape and verify cursor position
- Press Escape again and verify cursor position
This methodical approach is exactly what we want to see in our scientific tests. Well done!
Line range hint
1-211: Overall, excellent improvements to our focus retention tests!Class, let's take a moment to appreciate the hard work your classmate has put into improving these tests. They've made significant enhancements that will make our test suite more reliable and easier to maintain.
Here's a summary of the key improvements:
- Updated imports and test descriptions for better clarity
- Simplified test setup using
homePage.NavigateToHome()- Consistent use of
PageLeftPane.selectItem()andPageLeftPane.switchSegment()for navigation- Added more precise assertions using
agHelper.GetElement()- Addressed specific bug fixes related to focus retention
These changes are like upgrading our lab equipment - they'll help us conduct more accurate and efficient experiments in the future.
However, remember that there's always room for improvement:
- Consider adding short comments before entity selections for better readability
- Add small wait times after selections to ensure UI updates
- Replace xpath selectors with more reliable data-* attributes
Keep up the excellent work, and remember: in the world of testing, attention to detail is our best friend!
Description
Fix focus retention tests
Fixes #36673
Automation
/ok-to-test tags="@tag.IDE"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/11248761306
Commit: 0638436
Cypress dashboard.
Tags:
@tag.IDESpec:
Wed, 09 Oct 2024 06:49:03 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Bug Fixes
Tests