Skip to content

Create test to select #78

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
rotemergty5 wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Create test to select #78

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5959861
Create test to select
rotemergty5 May 25, 2026
50f9bf2
Fix SGLSelect tests
rotemergty5 Jun 1, 2026
ff73993
Extract shared test helper
rotemergty5 Jun 6, 2026
fb7fc70
Use wrapper component with children
rotemergty5 Jun 9, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 57 additions & 0 deletions src/components/UI/Select/SGLSelect.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
import { describe, it, expect, afterEach } from 'vitest'
import '@testing-library/jest-dom/vitest'
import { screen, fireEvent, cleanup } from '@testing-library/react'
import { renderWithTheme } from '../../../tests/renderWithTheme'
import { SGLSelect } from './SGLSelect'

afterEach(() => {
cleanup()
})

describe('SGLSelect', () => {
const options = ['Apple', 'Banana', 'Orange']

it('should select the relevant item', () => {
renderWithTheme(<SGLSelect options={options} />)

const select = screen.getByRole('combobox')

fireEvent.mouseDown(select)

const option = screen.getByText('Banana')
fireEvent.click(option)

expect(select).toHaveTextContent('Banana')
})

it('should not select any item when dropdown is opened and closed', () => {
renderWithTheme(<SGLSelect options={options} value="" />)

const select = screen.getByRole('combobox')

fireEvent.mouseDown(select)
fireEvent.keyDown(select, { key: 'Escape' })

expect(select).not.toHaveTextContent('Banana')
})
Comment on lines +27 to +36

@coderabbitai coderabbitai Bot May 25, 2026 •
edited
Loading

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Strengthen the Escape key test assertion.

The current assertion doesn't properly verify Escape behavior. Since value="" is set initially, the combobox would not contain "Banana" regardless of whether Escape works correctly. The test should verify that the original value is preserved after pressing Escape.

🧪 Improved test for Escape key behavior 
-  it('should not select any item when dropdown is opened and closed', () => {
-    render(<SGLSelect options={options} value="" />)
+  it('should preserve the selected value when Escape is pressed', () => {
+    render(<SGLSelect options={options} />)
 
     const select = screen.getByRole('combobox')
+    
+    // Initially should show first option (Apple) due to defaultValue
+    expect(select).toHaveTextContent('Apple')
 
     fireEvent.mouseDown(select)
     fireEvent.keyDown(select, { key: 'Escape' })
 
-    expect(select).not.toHaveTextContent('Banana')
+    // After Escape, should still show Apple
+    expect(select).toHaveTextContent('Apple')
   })
🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/components/UI/Select/SGLSelect.test.tsx` around lines 28 - 37, The test
currently renders <SGLSelect options={options} value="" /> so asserting the
combobox does not contain "Banana" cannot verify Escape behavior; update the
test to render with a non-empty initial value (e.g., value="Banana") or with a
controlled value variable set to "Banana", open the dropdown via
fireEvent.mouseDown(select), send Escape with fireEvent.keyDown(select, { key:
'Escape' }), and then assert the combobox still contains "Banana" (or that the
controlled value didn't change) to confirm the original value is preserved after
pressing Escape; use the existing SGLSelect component and the select element
obtained by screen.getByRole('combobox') to locate the element.

@Tamir198 Tamir198 Jun 1, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rotemergty5 I agree with this comment

@coderabbitai coderabbitai Bot Jun 1, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!

@Tamir198 Tamir198 Jun 6, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rotemergty5 Reminder

Comment on lines +27 to +36

@GilHeller GilHeller Jun 9, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As @Tamir198 and the coderabbit said, you have a logic flaw. If you wont click anything the value obviously wont be "Banana". Consider to change the test for trying to click "Banana" option while not appears.

@rotemergty5 rotemergty5 Jun 14, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Updated the wrapper name and improved the test.


it('should change the selected value when selecting a different option', () => {
renderWithTheme(<SGLSelect options={options} />)

const select = screen.getByRole('combobox')

fireEvent.mouseDown(select)

const bananaOption = screen.getByText('Banana')
fireEvent.click(bananaOption)

expect(select).toHaveTextContent('Banana')

fireEvent.mouseDown(select)

const orangeOption = screen.getByText('Orange')
fireEvent.click(orangeOption)

expect(select).toHaveTextContent('Orange')
})
})
Comment thread
coderabbitai[bot] marked this conversation as resolved.
11 changes: 11 additions & 0 deletions src/tests/renderWithTheme.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
import { render } from '@testing-library/react'
import { ThemeProvider } from '@mui/material/styles'
import type { PropsWithChildren, ReactElement } from 'react'

import { theme } from '../theme'

const Wrapper = ({ children }: PropsWithChildren) => (

Check warning on line 7 in src/tests/renderWithTheme.tsx

View workflow job for this annotation

GitHub Actions / Build, lint, prettier and tests 

Fast refresh only works when a file only exports components. Move your component(s) to a separate file
<ThemeProvider theme={theme}>{children}</ThemeProvider>
)

export const renderWithTheme = (ui: ReactElement) => render(ui, { wrapper: Wrapper })

@GilHeller GilHeller Jun 9, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can just export the Wrapper component. Also rename it to more indicative name

Loading