test: hard code date values in Date_column_types_validation_spec #36759
Conversation
WalkthroughThe changes in this pull request focus on enhancing the Cypress end-to-end test suite for validating date column types in a table widget. Key modifications include clarifying comments and updating assertions in the Changes
Assessment against linked 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
|
![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/TableV2/Date_column_types_validation_spec.ts (3)
46-47: Good job on improving the comment clarity, class!The updated comment provides a better explanation of why the date picker opens in September 2024. This aligns with our objective of addressing the issue with static date values.
However, let's make it even better! Can you add a note about how this relates to the problem described in the PR objectives? For example:
"This fixed date is part of the issue we're addressing, as it doesn't allow for testing relative dates like 'today' and 'tomorrow'."
Line range hint
1-180: Class, let's have a discussion about our overall test structure.You've done a commendable job organizing the tests for various date formats. The structure is logical and easy to follow. Gold star for that!
However, we need to address an important issue. Throughout our tests, we're still using hardcoded dates. This doesn't align with our goal of handling relative dates like 'today' and 'tomorrow'.
Here's your homework assignment:
- Refactor the
setEditableDateFormatsfunction to handle relative dates.- Update all test cases to use relative dates instead of fixed ones.
- Consider creating helper functions for getting 'today' and 'tomorrow' dates in various formats.
Remember, our goal is to make these tests robust and able to run correctly regardless of the current date. Let's work together to improve this!
Attention Needed: Remove Hardcoded Date in Line 55
Great effort in identifying and addressing most hardcoded dates. However, there's still a hardcoded date
"2024-09-26"present in line 55. Please replace this with a dynamic relative date to ensure the tests remain reliable and maintainable.🔗 Analysis chain
Line range hint
1-180: Class, let's summarize our lesson for today.We've made some progress in addressing the issue with fixed dates, but we still have work to do. Here are the key points to remember:
- The comment changes have improved clarity, but we need to relate them more directly to our objectives.
- We're still using hardcoded dates in our selectors and assertions, which doesn't solve our core problem.
- The overall test structure is good, but needs to be refactored to handle relative dates.
Your assignment is to revise this code to fully address the issue of handling relative dates. Remember, in software development, we often need to iterate on our solutions to get them just right. Keep up the good work, and don't hesitate to ask questions if you need help!
To ensure we've addressed all instances of hardcoded dates, let's run a quick check:
This will help us identify any remaining hardcoded dates that we need to replace with relative date logic.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for hardcoded date patterns in the file echo "Checking for hardcoded dates:" rg -n '\d{4}-\d{2}-\d{2}' app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/Date_column_types_validation_spec.tsLength of output: 257
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/Date_column_types_validation_spec.ts (1 hunks)
- app/client/cypress/support/Pages/Table.ts (0 hunks)
💤 Files with no reviewable changes (1)
- app/client/cypress/support/Pages/Table.ts
🧰 Additional context used
📓 Path-based instructions (1)
app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/Date_column_types_validation_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.
| agHelper.GetNClick( | ||
| `${table._dateInputPopover} [aria-label='${table.getFormattedTomorrowDates().verboseFormat}']`, | ||
| `${table._dateInputPopover} [aria-label='Thu Sep 26 2024']`, |
There was a problem hiding this comment.
Attention, students! We need to improve our selector usage.
While the change addresses the issue with the fixed date, it doesn't fully solve our problem. We're still using a hardcoded date, which goes against our goal of handling relative dates.
Let's think about how we can make this more dynamic. Can you come up with a way to select the date based on 'today' or 'tomorrow' instead of a fixed date? Remember, we want to avoid hardcoding specific dates in our tests.
| .then((val) => | ||
| expect(val).to.equal(table.getFormattedTomorrowDates().isoFormat), | ||
| ); | ||
| .then((val) => expect(val).to.equal("2024-09-26")); |
There was a problem hiding this comment.
Class, let's discuss this assertion change.
The assertion now uses a hardcoded date value "2024-09-26" instead of a dynamic method. While this matches the clicked date, it doesn't align with our goal of working with relative dates.
Let's think about how we can make this assertion more flexible. Could we use a variable that represents 'tomorrow' or 'today + 1 day' instead of a fixed date? This would make our test more robust and less likely to fail due to date changes.
| .then((val) => | ||
| expect(val).to.equal(table.getFormattedTomorrowDates().isoFormat), | ||
| ); | ||
| .then((val) => expect(val).to.equal("2024-09-26")); |
There was a problem hiding this comment.
Do we think this is also hard coded?
There was a problem hiding this comment.
Yes, this is a hardcoded date value, i.e when the fixed date from the Table data is used, this hardcoded date here is always visible in the date picker viewport and will be selected.
|
/ci-test-limit |
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/11250901833. |
|
/ci-test-limit-count run_count=15 runId=11250901833 |
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/11250926828. |
|
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/11250901833. |
rahulbarwal
changed the title
github-actions
bot
added
Task
jacquesikot
added
ok-to-test
github-actions
bot
added
the
Widgets & Accelerators Pod
jacquesikot
added
ok-to-test
rahulbarwal
added
ok-to-test
jacquesikot
added
ok-to-test
|
/ci-test-limit |
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/11270455557. |
|
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/11270455557. |
…o fix/date_column_types_validation_spec
Description
Problem
In the test spec, date data is fed directly from a file
(app/client/cypress/fixtures/tableDateColumnTypes.ts), which includes specific, static dates. However, the test is designed to handle relative dates (e.g., "today" and "tomorrow"). As a result, when the date picker opens, it displays a fixed date (e.g., September 2024), while the test is attempting to select a date like October 8 (tomorrow’s date), which is out of view.EE PR - https://github.com/appsmithorg/appsmith-ee/pull/5312
Fixes #36756
Automation
/ok-to-test tags="@tag.Table"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/11270917513
Commit: 9ac8789
Cypress dashboard.
Tags:
@tag.TableSpec:
Thu, 10 Oct 2024 09:53:10 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Bug Fixes
Refactor
getFormattedTomorrowDatesmethod from the Table class, streamlining date formatting functionality.