Refactor Facility Export & Add New Sidebar collapse & expand Test #9066
Conversation
WalkthroughThe pull request introduces modifications to the Cypress end-to-end test suite for the facility homepage, including the addition of a sidebar collapse and expand functionality test. Enhancements to the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
cypress/e2e/facility_spec/FacilityHomepage.cy.tsOops! Something went wrong! :( ESLint: 8.57.1 Error: Failed to load parser '@typescript-eslint/parser' declared in '.eslintrc.json': Cannot find module '@typescript-eslint/parser'
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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
CodeRabbit Configuration File (
|
✅ Deploy Preview for care-ohc ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
![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 (2)
cypress/pageobject/Facility/FacilityHome.ts (1)
7-9: Consider maintaining consistent selector patterns across the class.While the new selectors correctly use data-testid attributes (which is great for test stability), they differ in style from existing selectors in the class. Consider either:
- Converting all selectors to arrow functions (preferred for preventing stale elements), or
- Maintaining consistency with existing string property pattern
- sidebarToggle = () => cy.get('[data-testid="sidebar-toggle"]'); - sidebarIcon = () => cy.get('[data-testid="sidebar-icon"]'); - sidebarText = () => cy.get('[data-testid="sidebar-text"]'); + // Option 1 (Preferred): Convert all selectors to arrow functions + exportButton = () => cy.get("#export-button"); + sidebarToggle = () => cy.get('[data-testid="sidebar-toggle"]'); + sidebarIcon = () => cy.get('[data-testid="sidebar-icon"]'); + sidebarText = () => cy.get('[data-testid="sidebar-text"]'); + + // OR Option 2: Keep consistent with current pattern + sidebarToggle = '[data-testid="sidebar-toggle"]'; + sidebarIcon = '[data-testid="sidebar-icon"]'; + sidebarText = '[data-testid="sidebar-text"]';cypress/e2e/facility_spec/FacilityHomepage.cy.ts (1)
139-152: Enhance test readability with descriptive steps.The test implementation looks good and covers all required scenarios. Consider adding
cy.log()statements to make the test steps more descriptive and easier to debug.it("Verify sidebar collapse and expand functionality", () => { + cy.log('Verify initial sidebar state'); facilityHome.toggleSidebar(); facilityHome.verifyIconsVisible(); facilityHome.verifyTextVisible(); - // Click toggle button to collapse sidebar, verify icons visible and text hidden + cy.log('Verify collapsed sidebar state'); facilityHome.toggleSidebar(); facilityHome.verifyIconsVisible(); facilityHome.verifyTextHidden(); - // Click toggle button again to expand sidebar, verify icons and text are visible again + cy.log('Verify expanded sidebar state'); facilityHome.toggleSidebar(); facilityHome.verifyIconsVisible(); facilityHome.verifyTextVisible(); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
cypress/e2e/facility_spec/FacilityHomepage.cy.ts(4 hunks)cypress/pageobject/Facility/FacilityHome.ts(2 hunks)src/components/Common/Sidebar/Sidebar.tsx(1 hunks)src/components/Common/Sidebar/SidebarItem.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/components/Common/Sidebar/Sidebar.tsx
🔇 Additional comments (8)
cypress/pageobject/Facility/FacilityHome.ts (2)
111-113: LGTM! Clean and focused implementation.
The toggleSidebar method is simple and follows the single responsibility principle.
115-129: Consider enhancing visibility verification methods.
While the implementation is functional, consider these improvements:
- Add timeout configuration for visibility checks
- Add error handling for missing elements
- Consider combining similar verification methods using a parameter
- verifyIconsVisible() {
- this.sidebarIcon().each(($el) => {
- cy.wrap($el).should("be.visible");
- });
- }
-
- verifyTextVisible() {
- this.sidebarText().each(($el) => {
- cy.wrap($el).should("be.visible");
- });
- }
+ verifySidebarElementsVisibility(elementType: 'icon' | 'text', shouldBeVisible: boolean = true) {
+ const selector = elementType === 'icon' ? this.sidebarIcon : this.sidebarText;
+ const assertion = shouldBeVisible ? 'be.visible' : 'not.be.visible';
+
+ selector()
+ .should('exist')
+ .should(assertion, { timeout: 10000 })
+ .then($elements => {
+ if ($elements.length === 0) {
+ throw new Error(`No ${elementType} elements found in sidebar`);
+ }
+ });
+ }This refactored version:
- Combines similar methods into one configurable method
- Adds timeout for flaky test prevention
- Verifies elements exist before checking visibility
- Provides better error messages
src/components/Common/Sidebar/SidebarItem.tsx (3)
63-65: LGTM! Well-placed test identifier for the sidebar icon.
The data-testid="sidebar-icon" attribute is appropriately placed on the icon container, making it easily targetable in the new sidebar collapse/expand tests.
67-67: LGTM! Well-placed test identifier for the sidebar text.
The data-testid="sidebar-text" attribute is correctly added to the text container span, enabling proper visibility testing when the sidebar is collapsed/expanded.
Line range hint 52-57: Verify resetHistory functionality as noted in the comment.
There's a comment suggesting to check if resetHistory is working as intended. Let's verify its usage across the codebase.
✅ Verification successful
Let me gather more context about the useAppHistory hook and its implementation to better understand the resetHistory functionality.
Let me search for the context and implementation of the history-related contexts to understand the complete flow.
resetHistory functionality is correctly implemented and used
The verification shows that resetHistory is properly implemented and used in the SidebarItem component:
- It's correctly imported from
useAppHistoryhook - The hook properly connects to the
ResetHistoryContextfromHistoryAPIProvider - It's called in the
onClickhandler along with other navigation-related callbacks - The implementation follows React's context pattern for history management
The comment "On Review: Check if resetHistory is working as intended" can be safely removed as the functionality is verified.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for resetHistory usage and implementation
echo "Searching for resetHistory implementation..."
ast-grep --pattern 'const { resetHistory } = useAppHistory()' -A 10
echo "\nSearching for resetHistory usage..."
rg 'resetHistory\(' -A 5
Length of output: 1677
Script:
#!/bin/bash
# Search for useAppHistory implementation
echo "Searching for useAppHistory implementation..."
ast-grep --pattern 'function useAppHistory()' -A 15
echo "\nSearching for useAppHistory hook definition..."
rg 'export.*useAppHistory' -A 10
echo "\nSearching for AppHistory type or interface..."
rg 'type.*AppHistory|interface.*AppHistory' -A 5
Length of output: 1157
Script:
#!/bin/bash
# Search for HistoryContext and ResetHistoryContext
echo "Searching for History contexts..."
rg "HistoryContext|ResetHistoryContext" -A 5
echo "\nSearching for history provider implementation..."
ast-grep --pattern 'function HistoryProvider' -A 15
echo "\nSearching for resetHistory usage in the component..."
rg "resetHistory" src/components/Common/Sidebar/SidebarItem.tsx -A 5 -B 5
Length of output: 3149
cypress/e2e/facility_spec/FacilityHomepage.cy.ts (3)
2-3: LGTM: Import changes and new capacity alias.
The addition of the capacitiesAlias constant aligns with the PR objective of integrating capacity export functionality into the main facility export test.
Also applies to: 19-19
156-158: LGTM: afterEach hook relocation.
The relocation of the afterEach hook to the end of the file is a good organizational change and maintains the correct functionality.
111-115: 🛠️ Refactor suggestion
Add API verification for capacity export.
While the integration of capacity export test looks good, the PR objectives mention adding API verification to enhance test reliability. Consider adding API checks to verify the exported data.
Example implementation:
// Verify Capacity Export
facilityHome.csvDownloadIntercept(capacitiesAlias, "&capacity");
// Add API verification before download
cy.request('GET', '/api/v1/facility/capacity').then((response) => {
expect(response.status).to.eq(200);
const apiData = response.body;
facilityHome.clickExportButton();
facilityHome.clickMenuItem("Capacities");
// Verify downloaded CSV matches API data
facilityHome.verifyDownload(capacitiesAlias).then((csvData) => {
expect(csvData).to.deep.equal(apiData);
});
});
|
👋 Hi, @Alokih, This message is automatically generated by prince-chrismc/label-merge-conflicts-action so don't hesitate to report issues/improvements there. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
cypress/e2e/facility_spec/FacilityHomepage.cy.ts (1)
141-156: Consider verifying initial sidebar state.While the test effectively verifies collapse and expand functionality, consider adding assertions to verify the initial state of the sidebar before any interactions.
it("Verify sidebar collapse and expand functionality", () => { + // Verify initial state + facilityHome.verifyIconsVisible(); + facilityHome.verifyTextVisible(); + facilityHome.toggleSidebar(); facilityHome.verifyIconsVisible(); facilityHome.verifyTextVisible();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
cypress/e2e/facility_spec/FacilityHomepage.cy.ts(4 hunks)cypress/pageobject/Facility/FacilityHome.ts(2 hunks)src/components/Common/Sidebar/SidebarItem.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- cypress/pageobject/Facility/FacilityHome.ts
- src/components/Common/Sidebar/SidebarItem.tsx
🧰 Additional context used
🪛 Biome
cypress/e2e/facility_spec/FacilityHomepage.cy.ts
[error] 209-209: expected } but instead the file ends
the file ends here
(parse)
[error] 206-209: Disallow duplicate setup and teardown hooks.
Disallow afterEach duplicacy inside the describe function.
(lint/suspicious/noDuplicateTestHooks)
🔇 Additional comments (2)
cypress/e2e/facility_spec/FacilityHomepage.cy.ts (2)
21-21: LGTM: Consistent constant naming.
The new capacitiesAlias follows the established naming pattern for CSV download aliases.
113-117: LGTM: Clean integration of capacity export test.
The capacity export verification follows the established pattern and successfully integrates with the facility export test as per PR objectives.
✅ Verification successful
The previous search was too specific. Let's broaden our search patterns to find any capacity export related tests that might still exist separately.
Integration of capacity export test verified - no duplicate tests found
The search results confirm that the capacity export test exists only in FacilityHomepage.cy.ts and there are no separate or duplicate capacity export tests remaining in the codebase. The integration is clean and follows the consolidated approach as intended.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify no remaining separate capacity export tests
rg -l "Verify Capacity Export Functionality" --type ts
Length of output: 56
Script:
#!/bin/bash
# Search for any capacity export related tests with broader patterns
rg -i "capacity.*export" --type ts
# Also check for any test descriptions mentioning capacity exports
rg -i "describe.*capacity.*export" --type ts
rg -i "it.*capacity.*export" --type ts
# Look for any cy.intercept calls related to capacity
ast-grep --pattern 'cy.intercept($$$capacity$$$)'
Length of output: 255
| afterEach(() => { | ||
| cy.saveLocalStorage(); | ||
| }); |
There was a problem hiding this comment.
Remove duplicate afterEach hook.
There's a duplicate afterEach hook outside the describe block. This is redundant as the same hook already exists within the describe block and could lead to unexpected behavior.
Remove the duplicate hook:
-afterEach(() => {
- cy.saveLocalStorage();
-});📝 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.
| afterEach(() => { | |
| cy.saveLocalStorage(); | |
| }); |
🧰 Tools
🪛 Biome
[error] 209-209: expected } but instead the file ends
the file ends here
(parse)
[error] 206-209: Disallow duplicate setup and teardown hooks.
Disallow afterEach duplicacy inside the describe function.
(lint/suspicious/noDuplicateTestHooks)
| it("Verify sidebar collapse and expand functionality", () => { | ||
| facilityHome.toggleSidebar(); | ||
| facilityHome.verifyIconsVisible(); | ||
| facilityHome.verifyTextVisible(); |
There was a problem hiding this comment.
- it does not verify that each icon has a respective text next to it.
There was a problem hiding this comment.
@nihal467 sir I have made changes for this & sorry for those syntax & duplicate errors I tried to fix merge conflicts directly & made those mistakes
| facilityHome.toggleSidebar(); | ||
| facilityHome.verifyIconsVisible(); | ||
| facilityHome.verifyTextVisible(); | ||
|
|
There was a problem hiding this comment.
- once the test is complete, the brackets are missing, syntax error
| @@ -185,3 +203,7 @@ describe("Facility Homepage Function", () => { | |||
| cy.saveLocalStorage(); | |||
| }); | |||
| }); | |||
|
|
|||
| afterEach(() => { | |||
There was a problem hiding this comment.
- delete the duplicate aftereach function
nihal467
added
test failed
changes required
and removed
needs testing
needs review
labels
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
cypress/pageobject/Facility/FacilityHome.ts (1)
7-8: Maintain consistent selector definition styleThe new selectors use arrow functions while existing selectors use direct string assignments. Consider maintaining consistency with the existing pattern:
- sidebarToggle = () => cy.get('[data-testid="sidebar-toggle"]'); - sidebarItems = () => cy.get('[data-testid="sidebar-item"]'); + sidebarToggle = '[data-testid="sidebar-toggle"]'; + sidebarItems = '[data-testid="sidebar-item"]';Then update the methods to use
cy.get(this.sidebarToggle)andcy.get(this.sidebarItems).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
cypress/e2e/facility_spec/FacilityHomepage.cy.ts(3 hunks)cypress/pageobject/Facility/FacilityHome.ts(2 hunks)src/components/Common/Sidebar/SidebarItem.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- cypress/e2e/facility_spec/FacilityHomepage.cy.ts
- src/components/Common/Sidebar/SidebarItem.tsx
🔇 Additional comments (1)
cypress/pageobject/Facility/FacilityHome.ts (1)
7-8: Implementation aligns well with PR objectives
The added methods provide a solid foundation for testing sidebar collapse functionality:
- Clear separation of concerns between interaction and verification
- Comprehensive checks for both expanded and collapsed states
- Use of reliable data-testid selectors
Also applies to: 111-129
| verifyIconsAndTextVisible() { | ||
| this.sidebarItems().each(($item) => { | ||
| cy.wrap($item).find('[data-testid="sidebar-icon"]').should("be.visible"); | ||
| cy.wrap($item).find('[data-testid="sidebar-text"]').should("be.visible"); | ||
| }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance verification methods reliability and reduce duplication
Consider these improvements:
- Add timeout and empty check
- Extract common verification logic
+ private verifySidebarElements(textVisibility: 'be.visible' | 'not.be.visible') {
+ this.sidebarItems().should('have.length.at.least', 1)
+ .each(($item) => {
+ cy.wrap($item, { timeout: 10000 })
+ .find('[data-testid="sidebar-icon"]')
+ .should('be.visible');
+ cy.wrap($item, { timeout: 10000 })
+ .find('[data-testid="sidebar-text"]')
+ .should(textVisibility);
+ });
+ }
verifyIconsAndTextVisible() {
- this.sidebarItems().each(($item) => {
- cy.wrap($item).find('[data-testid="sidebar-icon"]').should("be.visible");
- cy.wrap($item).find('[data-testid="sidebar-text"]').should("be.visible");
- });
+ this.verifySidebarElements('be.visible');
}
verifyIconsVisibleAndTextHidden() {
- this.sidebarItems().each(($item) => {
- cy.wrap($item).find('[data-testid="sidebar-icon"]').should("be.visible");
- cy.wrap($item)
- .find('[data-testid="sidebar-text"]')
- .should("not.be.visible");
- });
+ this.verifySidebarElements('not.be.visible');
}Also applies to: 122-129
| toggleSidebar() { | ||
| this.sidebarToggle().click(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add visibility check before clicking
To prevent flaky tests, consider adding a visibility check before clicking:
toggleSidebar() {
- this.sidebarToggle().click();
+ this.sidebarToggle().should('be.visible').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.
| toggleSidebar() { | |
| this.sidebarToggle().click(); | |
| } | |
| toggleSidebar() { | |
| this.sidebarToggle().should('be.visible').click(); | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
cypress/pageobject/Facility/FacilityHome.ts (2)
111-113: Consider adding error handling for better test stabilityWhile the implementation correctly checks for visibility before clicking, consider adding error handling for cases where the toggle action fails.
toggleSidebar() { - this.sidebarToggle().should("be.visible").click(); + this.sidebarToggle() + .should("be.visible") + .click() + .should("exist") + .then(($el) => { + // Verify the click was successful + if (!$el) { + throw new Error("Sidebar toggle action failed"); + } + }); }
115-128: Consider adding retry mechanism for better test stabilityWhile the implementation is solid, consider adding retry logic for better handling of flaky scenarios where elements might take longer to update their visibility state.
private verifySidebarElements( textVisibility: "be.visible" | "not.be.visible", ) { this.sidebarItems() .should("have.length.at.least", 1) - .each(($item) => { - cy.wrap($item, { timeout: 10000 }) - .find('[data-testid="sidebar-icon"]') - .should("be.visible"); - cy.wrap($item, { timeout: 10000 }) - .find('[data-testid="sidebar-text"]') - .should(textVisibility); - }); + .each(($item) => { + const retryOptions = { + timeout: 10000, + interval: 500, + }; + + cy.wrap($item) + .find('[data-testid="sidebar-icon"]') + .should("be.visible", retryOptions); + + cy.wrap($item) + .find('[data-testid="sidebar-text"]') + .should(textVisibility, retryOptions); + }); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
cypress/pageobject/Facility/FacilityHome.ts(2 hunks)
🔇 Additional comments (2)
cypress/pageobject/Facility/FacilityHome.ts (2)
7-8: LGTM! Well-structured selectors using data-testid
The selectors follow Cypress best practices by using data-testid attributes and are implemented as functions for better dynamic element selection.
130-136: LGTM! Well-structured verification methods
The public verification methods are well-named and effectively utilize the private helper method, following the DRY principle.
| .should("be.visible"); | ||
| cy.wrap($item, { timeout: 10000 }) | ||
| .find('[data-testid="sidebar-text"]') | ||
| .should(textVisibility); |
There was a problem hiding this comment.
As mentioned earlier, ensure that the exact text is verified. For example, confirm that the text corresponding to the facility icon is precisely "Facilities" and not just any visible text. Similarly, verify the exact text for other icons as well.
|
@Alokih make the requested changes |
|
@nihal467 I have my end sem exams going on currently. Can I do it after few days ? |
Proposed Changes
@ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit
New Features
Improvements
data-testidattributes to components for better testability.Bug Fixes