TESTING EXTERNAL SCRIPT: external merge request from Contributor #37185
Conversation
…smith into fix/Issue_25911_onTextChanged_phone_input_widget
…_25911_onTextChanged_phone_input_widget' into chore/external-contribution-34715
jacquesikot
requested review from
ApekshaBhosale and
sagar-qa007
as code owners
WalkthroughThe pull request introduces enhancements to the Phone Input widget tests within the Cypress framework, focusing on various functionalities such as property visibility, entity management, tooltip functionality, country code toggling, label adjustments, validation logic, widget state toggles, formatting, event handling, and styling properties. Additionally, it modifies 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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
app/client/cypress/e2e/Regression/ClientSide/Widgets/PhoneInput/PhoneInput_Part2_spec.ts (3)
Line range hint
73-73: Class, let's avoid usingagHelper.Sleep()!I notice you're using
agHelper.Sleep(2000)in your test. As per our coding guidelines, we should avoid using sleep commands. Instead, let's wait for specific conditions:Replace the sleep commands with explicit wait conditions:
-agHelper.Sleep(2000); +agHelper.WaitUntilAllToastsDisappear(); // or +agHelper.WaitUntilEleAppear(locators._dropdownText);Also applies to: 291-291
Line range hint
74-74: Let's improve our selector strategy, students!I see we're using XPath selectors. According to our coding guidelines, we should avoid XPath and use data-* attributes instead.
Replace the XPath selector with a data attribute:
- First, add a data attribute to the span element in the component:
<span data-testid="country-option-india">India (+91)</span>
- Then update the test:
-agHelper.GetNClick("//span[text()='India (+91)']"); +agHelper.GetNClick("[data-testid='country-option-india']");Also applies to: 291-291
295-297: Excellent work on the toast verification, class! ✨The addition of toast message disappearance verification is a good practice. However, let's maintain consistency in our approach.
Consider using the helper method consistently throughout the test:
-cy.get(locators._toastMsg).should("not.exist"); +agHelper.AssertElementAbsence(locators._toastMsg);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
app/client/cypress/e2e/Regression/ClientSide/Widgets/PhoneInput/PhoneInput_Part2_spec.ts(1 hunks)app/client/src/widgets/PhoneInputWidget/widget/index.tsx(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
app/client/cypress/e2e/Regression/ClientSide/Widgets/PhoneInput/PhoneInput_Part2_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.
| // This regular expression validates that the input: | ||
| // - Does not start with a whitespace character | ||
| // - Contains only digits, spaces, parentheses, plus, and minus symbols | ||
| if (/^(?!\s)[\d\s()+-]*$/.test(value)) { | ||
| this.props.updateWidgetMetaProperty("text", formattedValue, { | ||
| triggerPropertyName: "onTextChanged", | ||
| dynamicString: this.props.onTextChanged, | ||
| event: { | ||
| type: EventType.ON_TEXT_CHANGE, | ||
| }, | ||
| }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Class, let's improve our phone number validation! 📱
Good work on adding input validation! However, let's make it even better with these educational improvements:
- Move the regex pattern to a named constant with documentation:
+// Validates phone number input:
+// - No leading whitespace
+// - Only allows digits, spaces, parentheses, plus, and minus symbols
+const PHONE_NUMBER_PATTERN = /^(?!\s)[\d\s()+-]*$/;
+
onValueChange = (value: string) => {
// ... existing code ...
- if (/^(?!\s)[\d\s()+-]*$/.test(value)) {
+ if (PHONE_NUMBER_PATTERN.test(value)) {- Consider providing user feedback when invalid characters are entered:
if (PHONE_NUMBER_PATTERN.test(value)) {
this.props.updateWidgetMetaProperty("text", formattedValue, {
triggerPropertyName: "onTextChanged",
dynamicString: this.props.onTextChanged,
event: {
type: EventType.ON_TEXT_CHANGE,
},
});
+ } else {
+ // Notify user about invalid input
+ this.props.updateWidgetMetaProperty("errorMessage", "Please enter a valid phone number");
}- For extra credit 🌟, consider enhancing the validation:
- Add maximum length validation
- Consider country-specific phone number formats
- Add support for international number validation using
libphonenumber-js
Would you like me to help implement any of these improvements?
Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (10)
app/client/cypress/e2e/Regression/ClientSide/Widgets/PhoneInput/PhoneInput_Part2_spec.ts (10)
Line range hint
88-112: Clarify Tooltip Functionality ImplementationWhile your implementation of the tooltip functionality is commendable, consider the following to enhance clarity and efficiency:
- Consistency in Test Steps: Ensure that each action is clearly defined. For instance, after updating the tooltip property, you immediately proceed to hover over the tooltip icon. Explicitly stating any waits or validations in between can prevent flaky tests.
- Avoid Hardcoded Coordinates: Instead of using absolute positions like
550, 300, consider defining positions relative to the widget or page layout to enhance test reliability across different screen sizes.
Line range hint
114-124: Optimize Country Code Toggle TestYou've effectively tested the country code toggle functionality. However, to optimize the test:
- Reduce Sleep Time: Instead of using
agHelper.Sleep(2000);, wait for the specific element to be visible or interactable. This practice reduces test execution time and increases reliability.Consider replacing it with:
agHelper.GetNClick(propPane._countryCodeChangeDropDown); agHelper.TypeText(propPane._searchCountryPlaceHolder, "India"); - agHelper.Sleep(2000); + agHelper.WaitUntilElementVisible("//span[text()='India (+91)']"); agHelper.GetNClick("//span[text()='India (+91)']");
Line range hint
126-144: Ensure Accurate Attribute AssertionsWhen verifying the label position attribute, ensure that the attribute you are asserting matches the actual implementation. For instance, the
positionattribute may not reflect the label's visual position in the DOM.Consider using a CSS property or class that changes when the label position is updated. This approach provides a more accurate verification of the label's position.
Line range hint
173-187: Improve Toggle State Tests for ClarityWhile testing the 'visible', 'disabled', and 'autofocus' properties, ensure that:
- Attribute Checks Match Widget Behavior: For the 'visible' property, checking for the
data-hiddenattribute is correct. Additionally, you could verify the widget's presence or absence in the DOM.- Focus State Verification: After setting 'autofocus' to 'On' and refreshing the page, confirm that the input is indeed focused by checking the focus state explicitly.
Line range hint
189-199: Handle Country Selection Without DelaysAgain, to reduce reliance on arbitrary sleep times:
- Replace
agHelper.Sleep(2000);with a more robust waiting mechanism.- Ensure that the test waits for the dropdown options to be populated before attempting to select a country.
agHelper.TypeText(propPane._searchCountryPlaceHolder, "India"); - agHelper.Sleep(2000); + agHelper.WaitUntilElementVisible("//span[text()='India (+91)']"); agHelper.GetNClick("//span[text()='India (+91)']");
Line range hint
201-207: Validate Input Formatting ThoroughlyWhen testing the 'Enable formatting' toggle:
- Verify All Formatting Scenarios: Ensure that you're checking both enabling and disabling formatting and how it affects different types of input.
- Edge Cases: Test with edge cases such as maximum length numbers or special characters to confirm that the formatting applies correctly.
Line range hint
209-217: Auto Height Limits Best PracticesGreat job verifying the auto height with limits functionality. As a best practice:
- Dynamic Content Testing: Consider adding tests where the content inside the widget changes dynamically to see if the auto-height adjusts within the specified limits.
- Visual Confirmation: While asserting contains text like "Min-Height: 4 rows", you might also want to check the actual CSS properties to ensure they reflect the expected values.
Line range hint
219-237: Enhance Event Handling Tests with Negative ScenariosYour tests for event handlers are comprehensive. To further strengthen them:
- Negative Testing: Include scenarios where events should not trigger, ensuring that, for example, 'onTextChange' does not fire when the input value remains the same.
- Event Order Verification: If the order of event triggers is important, add assertions to verify that events fire in the correct sequence.
Line range hint
239-272: Improve Style Property AssertionsWhen verifying style changes:
- Use Specific CSS Assertions: Instead of asserting attributes like
font-styleas "ITALIC", check the actual CSS styles applied, such asfont-style: italic;orfont-weight: bold;.- Consistency in Emphasis: Ensure that toggling emphasis properties reflects consistently across editor, preview, and deploy modes.
Line range hint
274-294: Validate Border and Box Shadow ChangesFor the border and box shadow assertions:
- CSS Property Checks: Verify the exact CSS values applied after changing the border radius and box shadow settings.
- Cross-Browser Compatibility: Be aware of how different browsers might render these styles and consider adding tests or notes if discrepancies are expected.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
app/client/cypress/e2e/Regression/ClientSide/Widgets/PhoneInput/PhoneInput_Part2_spec.ts(1 hunks)app/client/src/widgets/PhoneInputWidget/widget/index.tsx(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
app/client/cypress/e2e/Regression/ClientSide/Widgets/PhoneInput/PhoneInput_Part2_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 (4)
app/client/src/widgets/PhoneInputWidget/widget/index.tsx (1)
414-425:
Class, let's examine this phone number validation implementation.
The added validation is a good step towards ensuring proper input format, but there are a few learning opportunities here:
-
The regex pattern could be more restrictive to match real phone number patterns:
- It currently allows multiple consecutive special characters like "((((" or "+++"
- It permits numbers to start with special characters other than "+"
-
The validation message isn't communicated to the user when invalid input is detected.
Here's your homework assignment to improve the implementation:
- if (/^(?!\s)[\d\s()+-]*$/.test(value)) {
+ const isValidPhoneChar = /^[+]?[\d\s()-]*$/.test(value);
+ if (isValidPhoneChar) {
this.props.updateWidgetMetaProperty("text", formattedValue, {
triggerPropertyName: "onTextChanged",
dynamicString: this.props.onTextChanged,
event: {
type: EventType.ON_TEXT_CHANGE,
},
});
+ } else if (value) {
+ // Notify user about invalid input
+ this.props.updateWidgetMetaProperty("errorMessage", "Please enter a valid phone number format");
}Extra credit: Consider using a more comprehensive phone number validation library like libphonenumber-js which is already imported in this file!
Let's check if the library is being used elsewhere in the codebase:
app/client/cypress/e2e/Regression/ClientSide/Widgets/PhoneInput/PhoneInput_Part2_spec.ts (3)
Line range hint 41-70: Excellent Work on Property Visibility Checks
You've done a splendid job verifying the visibility of the properties in different sections such as Data, Label, Validation, General, Events, Label Styles, and Border Shadows. Looping through the predefined arrays to assert each property's visibility is efficient and promotes maintainability.
Line range hint 72-86: Attention to Entity Management Functions
Great job implementing the renaming, duplication, and deletion functionalities. Ensuring that the widget can be manipulated correctly in both the entity explorer and the property pane is crucial for user experience.
Line range hint 146-171: Strengthen Validation Logic Tests
Your tests for regex validation and the 'Valid' property are well-structured. However, ensure that:
- Error Messages Are Displayed Appropriately: Verify that the custom error message "Not valid value" appears when invalid input is provided.
- Dependent Properties Are Handled Correctly: When the 'Valid' property depends on another widget's visibility, consider adding assertions to check the widget's state before and after changes.
| agHelper.WaitUntilToastDisappear("Value Changed"); | ||
| agHelper.ClearNType(locators._input, "a"); | ||
| cy.get(locators._toastMsg).should("not.exist"); |
There was a problem hiding this comment.
Address Potential Flaky Test
In lines 295-297:
agHelper.WaitUntilToastDisappear("Value Changed");
agHelper.ClearNType(locators._input, "a");
cy.get(locators._toastMsg).should("not.exist");To ensure the test is not flaky:
- Explicit Waits for Toast Messages: Replace
cy.get(locators._toastMsg).should("not.exist");with a more robust wait that accounts for potential delays in toast disappearance. - Error Handling: Consider adding error handling for cases where the toast might not disappear as expected.
|
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. |
Description
Fixes #
Warning
If no issue exists, please create an issue first, and check with the maintainers if the issue is valid.
Automation
/ok-to-test tags="@tag.All"
🔍 Cypress test results
Caution
If you modify the content in this section, you are likely to disrupt the CI result for your PR.
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
Bug Fixes
Tests