Fixed SortBy component test cases #2173 #2180
Conversation
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the Summary by CodeRabbit
WalkthroughUpdated SortBy unit test to use the new custom dropdown UI (button + hidden combobox) and bumped frontend dependencies (including heroui packages and tooling) in frontend/package.json. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Assessment against linked issues
Out-of-scope changes(no out-of-scope changes detected) Possibly related PRs
Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, 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
🧹 Nitpick comments (7)
frontend/package.json (1)
107-109: Align jest-environment-jsdom version with Jest
In jest.config.ts (line 33) thetestEnvironmentuses [email protected], but your Jest version is ^29.7.0—either downgrade jest-environment-jsdom to ^29.7.0 or upgrade Jest to ^30.x to avoid potential CI flakiness.frontend/__tests__/unit/components/SortBy.test.tsx (6)
25-28: Avoid relying on data-slot; assert within the button instead.Using
[data-slot="value"]couples the test to internal markup. Query within the visible control.Apply:
- const sortButton = screen.getByRole('button', { name: /Sort By/ }) - expect(sortButton).toBeInTheDocument() - const selectedOption = screen.getByText('Name', { selector: '[data-slot="value"]' }) - expect(selectedOption).toBeInTheDocument() + const sortButton = screen.getByRole('button', { name: /Sort By/ }) + expect(sortButton).toBeInTheDocument() + expect(sortButton).toHaveTextContent('Name')
35-41: This test duplicates the previous one; make it validate the list options.Open the menu and assert options by role rather than re-checking the selected value.
Suggested change:
- const sortButton = screen.getByRole('button', { name: /Sort By/ }) - expect(sortButton).toBeInTheDocument() - // The dropdown options aren't directly visible in the DOM - // We're checking for the selected value instead - const selectedOption = screen.getByText('Name', { selector: '[data-slot="value"]' }) - expect(selectedOption).toBeInTheDocument() + const sortButton = screen.getByRole('button', { name: /Sort By/ }) + await act(async () => { + sortButton.click() + }) + expect(await screen.findByRole('option', { name: 'Name' })).toBeInTheDocument() + expect(await screen.findByRole('option', { name: 'Date' })).toBeInTheDocument()
47-50: Prefer userEvent over manual change on hidden select.Drive the UI like a user: open the menu and click the option. This makes the test resilient to internal changes.
Apply:
- await act(async () => { - const hiddenSelect = screen.getByRole('combobox', { hidden: true }) - fireEvent.change(hiddenSelect, { target: { value: 'date' } }) - }) + const user = userEvent.setup() + await act(async () => { + await user.click(screen.getByRole('button', { name: /Sort By/ })) + const dateOption = await screen.findByRole('option', { name: 'Date' }) + await user.click(dateOption) + })And update imports:
-import { render, screen, fireEvent } from '@testing-library/react' +import { render, screen } from '@testing-library/react' +import userEvent from '@testing-library/user-event'
59-61: Don’t assert FontAwesome classes; assert accessible text/labels instead.Class names are implementation details. Prefer the tooltip text or an aria-label on the order button (e.g., “Ascending” / “Descending”).
If SortBy doesn’t expose an accessible label, consider adding one (aria-label or title) so these tests can assert it reliably.
Also applies to: 68-70
76-80: Avoid relying on button index; target the order toggle explicitly.Index-based selection is brittle. Use a named control or derive the button from a nearby landmark.
Suggested change:
- await act(async () => { - // Get the second button (the sort order button) - const buttons = screen.getAllByRole('button') - fireEvent.click(buttons[1]) - }) + const user = userEvent.setup() + const sortButton = screen.getByRole('button', { name: /Sort By/ }) + // Choose the other button in the same container + const container = sortButton.closest('[role="group"], div')! + const orderButton = Array.from(container.querySelectorAll('button')).find(b => b !== sortButton)! + await act(async () => { + await user.click(orderButton) + })If possible, expose a stable aria-label (e.g., “Toggle sort order”) on the order button and query it directly.
88-94: Drop tagName and container assertions; assert semantics instead.Checking
tagName === 'SELECT'andclosest('div')is brittle. Assert ARIA semantics:- const hiddenSelect = screen.getByRole('combobox', { hidden: true }) - expect(hiddenSelect.tagName).toBe('SELECT') + const hiddenSelect = screen.getByRole('combobox', { hidden: true }) + expect(hiddenSelect).toBeInTheDocument()Optionally, if the combobox is labelled by the Sort By control:
expect(hiddenSelect).toHaveAccessibleName(/Sort By/)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (2)
backend/poetry.lockis excluded by!**/*.lockfrontend/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (2)
frontend/__tests__/unit/components/SortBy.test.tsx(3 hunks)frontend/package.json(4 hunks)
🔇 Additional comments (2)
frontend/package.json (2)
26-26: Misc dependency bumps—LGTM.
- @fortawesome/react-fontawesome, @sentry/nextjs: patch updates should be safe.
- @typescript-eslint/* and typescript-eslint: minor bumps aligned.
Also applies to: 39-39, 93-94, 119-119
28-36: Approve HeroUI version bumps
Lockfile confirms all @heroui packages’ peerDependencies (system ≥2.4.18, theme ≥2.4.17, React ≥18/≥19.0.0-rc.0, Framer-Motion ≥11.5.6) are satisfied—no warnings expected.
|
* fixed SortBy component testcases * fixed bot suggestions * Update code --------- Co-authored-by: Arkadii Yakovets <[email protected]>
* fixed SortBy component testcases * fixed bot suggestions * Update code --------- Co-authored-by: Arkadii Yakovets <[email protected]>
2 participants
Resolves #2173
Description
The problem is in SortBy.test.py unit test file.
This is the specific line used in four different test cases which failed:
SortBy.tsxfile uses<Select>component imported fromherouilibrary whose internal implementation changed in the latest update which cased problem is selecting the target element in theSortBy.test.tsxfile.Proposed change
Used sortButton to get dropdown button for sort by:
Used hiddenSelect to get hidden accessible element for testing functionality.
Also Upgraded dependencies.
Checklist
make check-testlocally; all checks and tests passed.