test: fix datepicker tests #37042
test: fix datepicker tests #37042
Conversation
NandanAnantharamu
requested review from
ApekshaBhosale and
sagar-qa007
as code owners
WalkthroughThe changes in this pull request involve modifications to the Cypress end-to-end testing framework for the DatePicker widget. The main updates include reactivating a previously skipped test case, simplifying 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
|
github-actions
bot
added
skip-changelog
|
/ci-test-limit |
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/11481833194. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
app/client/cypress/e2e/Regression/ClientSide/Widgets/Datepicker/DatePicker2_spec.js (3)
Line range hint
22-46: Enhance test reliability with better assertions and selectors.The reactivated test case needs improvements:
- The selector
formWidgetsPage.datepickerWidget + " .bp3-input"should use data-* attributes instead- Add multiple assertions to validate both the date value and format
Consider refactoring like this:
- cy.get(formWidgetsPage.datepickerWidget + " .bp3-input").should( - "contain.value", - nextDay, - ); + cy.get('[data-testid="datepicker-input"]').should(($input) => { + expect($input.val()).to.equal(nextDay); + expect(dayjs($input.val(), "DD/MM/YYYY").isValid()).to.be.true; + });
Line range hint
169-171: Remove afterEach block as per coding guidelines.The coding guidelines specifically mention avoiding afterEach blocks in test cases.
Consider moving the navigation logic into individual test cases or use a different approach for test cleanup.
Line range hint
1-171: Consider implementing skipped test cases.There are several commented-out test cases that appear to test important functionality:
- Min/max date validation
- Default date validation
- Required field validation
These tests would provide better coverage for the DatePicker widget.
Would you like me to help implement these test cases following the current best practices?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
- app/client/cypress/e2e/Regression/ClientSide/Widgets/Datepicker/DatePicker2_spec.js (2 hunks)
- app/client/cypress/limited-tests.txt (1 hunks)
- app/client/cypress/support/commands.js (1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
app/client/cypress/e2e/Regression/ClientSide/Widgets/Datepicker/DatePicker2_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.
app/client/cypress/support/commands.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 (1)
app/client/cypress/limited-tests.txt (1)
2-4: LGTM! Verify test paths exist.The DatePicker test specifications have been correctly added to the limited tests configuration.
Let's verify these test files exist:
✅ Verification successful
Based on the script results, I can generate the final response since all test files were successfully found in the repository.
All DatePicker test paths verified ✓
All three test specifications added to the limited tests configuration exist in the correct locations within the repository.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the existence of the specified test files # Expected: All files should exist in the repository for test_path in \ "cypress/e2e/Regression/ClientSide/Widgets/Datepicker/DatePicker2_spec.js" \ "cypress/e2e/Regression/ClientSide/Binding/DatePicker_Text_spec.js" \ "cypress/e2e/Regression/ClientSide/Widgets/Datepicker/DatePicker_With_Switch_spec.js" do if ! fd -p "$test_path" app/client; then echo "Warning: Test file not found: $test_path" fi doneLength of output: 1279
| */ | ||
| cy.setDate(1, "ddd MMM DD YYYY"); | ||
| cy.setDate(1); |
There was a problem hiding this comment.
💡 Codebase verification
Update required: setDate command usage is inconsistent with implementation
The current implementation of setDate only accepts a single parameter for days offset, but there are multiple occurrences in the codebase where it's called with two parameters. This needs to be fixed:
DatePicker_With_Switch_spec.js: UsessetDate(1, "ddd MMM DD YYYY")DatePicker_Text_spec.js: Two instances usingsetDate(1, "ddd MMM DD YYYY")
These calls need to be updated to match the current implementation which only accepts the days offset parameter.
🔗 Analysis chain
Verify the impact of simplified setDate command.
The setDate command has been simplified to use a single parameter. Ensure this change is consistent with the command implementation.
Let me check the implementation of the setDate command to verify if the simplified usage is correct.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for all usages of setDate command to verify consistency
rg "cy\.setDate\(" --type js
Length of output: 889
Script:
#!/bin/bash
# Search for the implementation of setDate command
rg -A 10 "Cypress\.Commands\.add\(['\"]setDate" --type js
Length of output: 941
| Cypress.Commands.add("setDate", (date) => { | ||
| const expDate = dayjs().add(date, "days").format("dddd, MMMM DD"); | ||
| cy.get(`.react-datepicker__day[aria-label^="Choose ${expDate}"]`).click(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider making the date format configurable.
The hardcoded date format "dddd, MMMM DD" reduces flexibility. Consider keeping the format parameter to support different date formats across tests.
-Cypress.Commands.add("setDate", (date) => {
+Cypress.Commands.add("setDate", (date, dateFormat = "dddd, MMMM DD") => {
const expDate = dayjs()
.add(date, "days")
- .format("dddd, MMMM DD");
+ .format(dateFormat);
cy.get(`.react-datepicker__day[aria-label^="Choose ${expDate}"]`).click();
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Cypress.Commands.add("setDate", (date) => { | |
| const expDate = dayjs().add(date, "days").format("dddd, MMMM DD"); | |
| cy.get(`.react-datepicker__day[aria-label^="Choose ${expDate}"]`).click(); | |
| Cypress.Commands.add("setDate", (date, dateFormat = "dddd, MMMM DD") => { | |
| const expDate = dayjs().add(date, "days").format(dateFormat); | |
| cy.get(`.react-datepicker__day[aria-label^="Choose ${expDate}"]`).click(); |
|
/ci-test-limit-count run_count=25 |
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/11482044903. |
|
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/11481833194. |
|
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/11482044903. |
NandanAnantharamu
added
ok-to-test
EE PR:https://github.com/appsmithorg/appsmith-ee/pull/5438
Updated SetDate method as the older method was not working anymore
/ok-to-test tags="@tag.All"
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/11512762742
Commit: 1e0ec01
Cypress dashboard.
Tags:
@tag.AllSpec:
Fri, 25 Oct 2024 08:22:21 UTC