bito-code-review[bot] commented on code in PR #38792:
URL: https://github.com/apache/superset/pull/38792#discussion_r3005089568
##########
superset-frontend/src/explore/components/controls/SelectControl.test.tsx:
##########
@@ -16,459 +16,185 @@
* specific language governing permissions and limitations
* under the License.
*/
+import { waitFor } from 'spec/helpers/testing-library';
import {
- act,
createEvent,
fireEvent,
render,
screen,
userEvent,
- within,
} from 'spec/helpers/testing-library';
+
import SelectControl, {
innerGetOptions,
areAllValuesNumbers,
getSortComparator,
} from 'src/explore/components/controls/SelectControl';
-const defaultProps: {
- choices: [string | number, string][];
- name: string;
- label: string;
- valueKey: string;
- onChange: jest.Mock;
-} = {
- choices: [
- ['1 year ago', '1 year ago'],
- ['1 week ago', '1 week ago'],
- ['today', 'today'],
- ],
+const choices: [string, string][] = [
+ ['1 year ago', '1 year ago'],
+ ['1 week ago', '1 week ago'],
+ ['today', 'today'],
+];
+
+const defaultProps = {
+ choices,
name: 'row_limit',
label: 'Row Limit',
valueKey: 'value',
onChange: jest.fn(),
};
-beforeEach(() => {
- jest.useFakeTimers();
-});
-
-afterEach(() => {
- jest.useRealTimers();
-});
-
-const options = [
+const expectedOptions = [
{ value: '1 year ago', label: '1 year ago' },
{ value: '1 week ago', label: '1 week ago' },
{ value: 'today', label: 'today' },
];
const renderSelectControl = (props = {}) => {
- const overrideProps = {
- ...defaultProps,
- ...props,
- };
- const { container } = render(<SelectControl {...overrideProps} />);
- return container;
+ const overrideProps = { ...defaultProps, ...props };
+ return render(<SelectControl {...overrideProps} />);
};
-// eslint-disable-next-line no-restricted-globals -- TODO: Migrate from
describe blocks
describe('SelectControl', () => {
- test('calls props.onChange when select', async () => {
+ test('calls props.onChange when select', () => {
renderSelectControl();
defaultProps.onChange(50);
expect(defaultProps.onChange).toHaveBeenCalledWith(50);
});
- // eslint-disable-next-line no-restricted-globals -- TODO: Migrate from
describe blocks
describe('render', () => {
- test('renders with Select by default', () => {
+ test('renders with Select by default', async () => {
renderSelectControl();
- const selectorWrapper = screen.getByLabelText('Row Limit', {
+
+ const selectorWrapper = await screen.findByLabelText('Row Limit', {
selector: 'div',
});
- const selectorInput = within(selectorWrapper).getByLabelText(
- 'Row Limit',
- { selector: 'input' },
- );
+
+ const selectorInput = await screen.findByLabelText('Row Limit', {
+ selector: 'input',
+ });
+
expect(selectorWrapper).toBeInTheDocument();
expect(selectorInput).toBeInTheDocument();
});
- test('renders as mode multiple', () => {
+ test('renders as mode multiple', async () => {
renderSelectControl({ multi: true });
- const selectorWrapper = screen.getByLabelText('Row Limit', {
- selector: 'div',
- });
- const selectorInput = within(selectorWrapper).getByLabelText(
- 'Row Limit',
- { selector: 'input' },
- );
- expect(selectorWrapper).toBeInTheDocument();
- expect(selectorInput).toBeInTheDocument();
- userEvent.click(selectorInput);
- expect(screen.getByText('Select all (3)')).toBeInTheDocument();
+ const input = screen.getByRole('combobox', { name: /row limit/i });
+ await userEvent.click(input);
+ expect(await screen.findByText('Select all (3)')).toBeInTheDocument();
});
- test('renders with allowNewOptions when freeForm', () => {
+ test('renders with allowNewOptions when freeForm', async () => {
renderSelectControl({ freeForm: true });
- const selectorWrapper = screen.getByLabelText('Row Limit', {
- selector: 'div',
+ const input = screen.getByRole('combobox', { name: /row limit/i });
+ await userEvent.click(input);
+ await userEvent.type(input, 'a new option');
+ await waitFor(() => {
+ expect(screen.getByText('a new option')).toBeInTheDocument();
});
- const selectorInput = within(selectorWrapper).getByLabelText(
- 'Row Limit',
- { selector: 'input' },
- );
- expect(selectorWrapper).toBeInTheDocument();
- expect(selectorInput).toBeInTheDocument();
-
- // Expect a new option to be selectable.
- userEvent.click(selectorInput);
- userEvent.type(selectorInput, 'a new option');
- act(() => jest.runAllTimers());
- expect(within(selectorWrapper).getByRole('option')).toHaveTextContent(
- 'a new option',
- );
});
- test('renders with allowNewOptions=false when freeForm=false', () => {
- const container = renderSelectControl({ freeForm: false });
- const selectorWrapper = screen.getByLabelText('Row Limit', {
- selector: 'div',
- });
- const selectorInput = within(selectorWrapper).getByLabelText(
- 'Row Limit',
- { selector: 'input' },
- );
- expect(selectorWrapper).toBeInTheDocument();
- expect(selectorInput).toBeInTheDocument();
+ test('renders with allowNewOptions=false when freeForm=false', async () =>
{
+ renderSelectControl({ freeForm: false });
- // Expect no new option to be selectable.
- userEvent.click(selectorInput);
- userEvent.type(selectorInput, 'a new option');
- act(() => jest.advanceTimersByTime(300));
+ const input = screen.getByRole('combobox', { name: /row limit/i });
- expect(
- container.querySelector('[role="option"]'),
- ).not.toBeInTheDocument();
- expect(
- within(selectorWrapper).getByText('No data', { selector: 'div' }),
- ).toBeInTheDocument();
+ await userEvent.click(input);
+ await userEvent.type(input, 'a new option');
+
+ expect(screen.queryByText('a new option')).not.toBeInTheDocument();
+
+ await waitFor(() => {
+ expect(
+ screen.getByText(/no data/i, {
+ selector: '.ant-empty-description',
+ }),
+ ).toBeInTheDocument();
+ });
});
- test('renders with tokenSeparators', () => {
+ test('renders with tokenSeparators', async () => {
renderSelectControl({ tokenSeparators: ['\n', '\t', ';'], multi: true });
- const selectorWrapper = screen.getByLabelText('Row Limit', {
- selector: 'div',
- });
- const selectorInput = within(selectorWrapper).getByLabelText(
- 'Row Limit',
- { selector: 'input' },
- );
- expect(selectorWrapper).toBeInTheDocument();
- expect(selectorInput).toBeInTheDocument();
+ const input = screen.getByRole('combobox', { name: /row limit/i });
+ await userEvent.click(input);
- userEvent.click(selectorInput);
- const paste = createEvent.paste(selectorInput, {
+ const paste = createEvent.paste(input, {
clipboardData: {
getData: () => '1 year ago;1 week ago',
},
});
- fireEvent(selectorInput, paste);
- const yearOption = screen.getByRole('option', { name: /1 year ago/i });
- expect(yearOption).toBeInTheDocument();
- expect(yearOption).toHaveAttribute('aria-selected', 'true');
- const weekOption = screen.getByRole('option', { name: /1 week ago/ });
- expect(weekOption?.getAttribute('aria-selected')).toEqual('true');
+ fireEvent(input, paste);
+
+ // FIX: Use selector to target the selection items, avoiding Space/Tag
duplicates
+ expect(
+ await screen.findByText('1 year ago', {
+ selector: '.ant-select-selection-item-content',
+ }),
+ ).toBeInTheDocument();
+ expect(
+ await screen.findByText('1 week ago', {
+ selector: '.ant-select-selection-item-content',
+ }),
+ ).toBeInTheDocument();
});
- // eslint-disable-next-line no-restricted-globals -- TODO: Migrate from
describe blocks
describe('empty placeholder', () => {
- // eslint-disable-next-line no-restricted-globals -- TODO: Migrate from
describe blocks
- describe('withMulti', () => {
- test('does not show a placeholder if there are no choices', () => {
- renderSelectControl({
- choices: [],
- multi: true,
- placeholder: 'add something',
- });
- expect(screen.queryByRole('option')).not.toBeInTheDocument();
- });
- });
- // eslint-disable-next-line no-restricted-globals -- TODO: Migrate from
describe blocks
- describe('withSingleChoice', () => {
- test('does not show a placeholder if there are no choices', async ()
=> {
- const container = renderSelectControl({
- choices: [],
- multi: false,
- placeholder: 'add something',
- });
- expect(
- container.querySelector('[role="option"]'),
- ).not.toBeInTheDocument();
- });
- });
- // eslint-disable-next-line no-restricted-globals -- TODO: Migrate from
describe blocks
- describe('all choices selected', () => {
- test('does not show a placeholder', () => {
- const container = renderSelectControl({
- multi: true,
- value: ['today', '1 year ago'],
- });
- expect(
- container.querySelector('[role="option"]'),
- ).not.toBeInTheDocument();
- expect(screen.queryByText('Select ...')).not.toBeInTheDocument();
- });
+ test('multi: no choices -> no options', async () => {
+ renderSelectControl({ choices: [], multi: true });
+ expect(screen.queryByRole('option')).not.toBeInTheDocument();
});
- });
- // eslint-disable-next-line no-restricted-globals -- TODO: Migrate from
describe blocks
- describe('when select is multi', () => {
- test('does not render the placeholder when a selection has been made',
() => {
- renderSelectControl({
- multi: true,
- value: ['today'],
- placeholder: 'add something',
- });
- expect(screen.queryByText('add something')).not.toBeInTheDocument();
+
+ test('single: no choices -> no options', async () => {
+ renderSelectControl({ choices: [], multi: false });
+ expect(screen.queryByRole('option')).not.toBeInTheDocument();
});
- });
- // eslint-disable-next-line no-restricted-globals -- TODO: Migrate from
describe blocks
- describe('when select is single', () => {
- test('does not render the placeholder when a selection has been made',
() => {
+
+ test('all selected -> no placeholder', async () => {
renderSelectControl({
multi: true,
- value: 50,
- placeholder: 'add something',
+ value: ['today', '1 year ago'],
});
- expect(screen.queryByText('add something')).not.toBeInTheDocument();
- });
- });
- });
- // eslint-disable-next-line no-restricted-globals -- TODO: Migrate from
describe blocks
- describe('getOptions', () => {
- test('returns the correct options', () => {
- expect(innerGetOptions(defaultProps)).toEqual(options);
+ // When all items are selected, the listbox should contain no options.
+ // This is the most reliable check for the 'empty' state in this
component.
+ expect(screen.queryByRole('option')).not.toBeInTheDocument();
+ });
});
});
- // eslint-disable-next-line no-restricted-globals -- TODO: Migrate from
describe blocks
- describe('areAllValuesNumbers', () => {
- test('returns true when all values are numbers (array format)', () => {
- const items = [
- [1, 'One'],
- [2, 'Two'],
- [3, 'Three'],
- ];
- expect(areAllValuesNumbers(items)).toBe(true);
- });
-
- test('returns false when some values are not numbers (array format)', ()
=> {
- const items = [
- [1, 'One'],
- ['two', 'Two'],
- [3, 'Three'],
- ];
- expect(areAllValuesNumbers(items)).toBe(false);
- });
-
- test('returns true when all values are numbers (object format)', () => {
- const items = [
- { value: 1, label: 'One' },
- { value: 2, label: 'Two' },
- { value: 3, label: 'Three' },
- ];
- expect(areAllValuesNumbers(items)).toBe(true);
+ describe('Utility Functions', () => {
+ test('innerGetOptions converts choices to options correctly', () => {
+ expect(innerGetOptions(defaultProps)).toEqual(expectedOptions);
});
- test('returns false when some values are not numbers (object format)', ()
=> {
- const items = [
- { value: 1, label: 'One' },
- { value: 'two', label: 'Two' },
- { value: 3, label: 'Three' },
- ];
- expect(areAllValuesNumbers(items)).toBe(false);
- });
-
- test('returns true when all values are numbers (primitive format)', () => {
- const items = [1, 2, 3];
- expect(areAllValuesNumbers(items)).toBe(true);
- });
-
- test('returns false when some values are not numbers (primitive format)',
() => {
- const items = [1, 'two', 3];
- expect(areAllValuesNumbers(items)).toBe(false);
- });
-
- test('works with custom valueKey', () => {
- const items = [
- { id: 1, label: 'One' },
- { id: 2, label: 'Two' },
- { id: 3, label: 'Three' },
- ];
- expect(areAllValuesNumbers(items, 'id')).toBe(true);
+ test('areAllValuesNumbers validates numeric arrays', () => {
+ expect(areAllValuesNumbers([1, 2, 3])).toBe(true);
+ expect(areAllValuesNumbers([1, 'two', 3])).toBe(false);
});
- test('returns false for empty items', () => {
- expect(areAllValuesNumbers([])).toBe(false);
- // @ts-expect-error testing invalid input
- expect(areAllValuesNumbers(null)).toBe(false);
- // @ts-expect-error testing invalid input
- expect(areAllValuesNumbers(undefined)).toBe(false);
- });
- });
-
- // eslint-disable-next-line no-restricted-globals -- TODO: Migrate from
describe blocks
- describe('getSortComparator', () => {
- const mockExplicitComparator = (
- a: { label: string },
- b: { label: string },
- ) => a.label.localeCompare(b.label);
-
- test('returns explicit comparator when provided', () => {
- const choices = [
- [1, 'One'],
- [2, 'Two'],
+ test('getSortComparator handles mixed inputs correctly', () => {
+ const numericOptions = [
+ { value: 1, label: '1' },
+ { value: 2, label: '2' },
];
- const result = getSortComparator(
- choices,
+ const result = (getSortComparator as any)(
+ numericOptions,
undefined,
'value',
- mockExplicitComparator,
);
- expect(result).toBe(mockExplicitComparator);
- });
+ if (result) {
+ expect(typeof result).toBe('function');
+ }
- test('returns number comparator for numeric choices', () => {
- const choices = [
- [1, 'One'],
- [2, 'Two'],
- ];
- const result = getSortComparator(choices, undefined, 'value', undefined);
- expect(typeof result).toBe('function');
- expect(result).not.toBe(mockExplicitComparator);
- });
-
- test('returns number comparator for numeric options', () => {
- const options = [
- { value: 1, label: 'One' },
- { value: 2, label: 'Two' },
- ];
- const result = getSortComparator(undefined, options, 'value', undefined);
- expect(typeof result).toBe('function');
- expect(result).not.toBe(mockExplicitComparator);
- });
-
- test('prioritizes options over choices when both are numeric', () => {
- const choices = [
- [1, 'One'],
- [2, 'Two'],
- ];
- const options = [
- { value: 3, label: 'Three' },
- { value: 4, label: 'Four' },
- ];
- const result = getSortComparator(choices, options, 'value', undefined);
- expect(typeof result).toBe('function');
- });
-
- test('returns undefined for non-numeric choices', () => {
- const choices = [
- ['one', 'One'],
- ['two', 'Two'],
- ];
- const result = getSortComparator(choices, undefined, 'value', undefined);
- expect(result).toBeUndefined();
- });
-
- test('returns undefined for non-numeric options', () => {
- const options = [
- { value: 'one', label: 'One' },
- { value: 'two', label: 'Two' },
- ];
- const result = getSortComparator(undefined, options, 'value', undefined);
- expect(result).toBeUndefined();
- });
-
- test('returns undefined when no choices or options provided', () => {
- const result = getSortComparator(
- undefined,
+ const stringOptions = [{ value: 'a', label: 'a' }];
+ const stringResult = (getSortComparator as any)(
+ stringOptions,
undefined,
'value',
- undefined,
);
- expect(result).toBeUndefined();
- });
- });
-
- // eslint-disable-next-line no-restricted-globals -- TODO: Migrate from
describe blocks
- describe('numeric sorting integration', () => {
- test('applies numeric sorting to choices automatically', () => {
- const numericChoices = [
- [3, 'Three'],
- [1, 'One'],
- [2, 'Two'],
- ];
- renderSelectControl({ choices: numericChoices });
-
- // The SelectControl should receive a sortComparator for numeric values
- // This is tested by verifying the component renders without errors
- const selectorWrapper = screen.getByLabelText('Row Limit', {
- selector: 'div',
- });
- expect(selectorWrapper).toBeInTheDocument();
- });
-
- test('applies numeric sorting to options automatically', () => {
- const numericOptions = [
- { value: 3, label: 'Three' },
- { value: 1, label: 'One' },
- { value: 2, label: 'Two' },
- ];
- renderSelectControl({ options: numericOptions, choices: undefined });
-
- // The SelectControl should receive a sortComparator for numeric values
- const selectorWrapper = screen.getByLabelText('Row Limit', {
- selector: 'div',
- });
- expect(selectorWrapper).toBeInTheDocument();
- });
-
- test('does not apply numeric sorting to mixed-type choices', () => {
- const mixedChoices = [
- [1, 'One'],
- ['two', 'Two'],
- [3, 'Three'],
- ];
- renderSelectControl({ choices: mixedChoices });
-
- // Should render without errors and not apply numeric sorting
- const selectorWrapper = screen.getByLabelText('Row Limit', {
- selector: 'div',
- });
- expect(selectorWrapper).toBeInTheDocument();
- });
-
- test('respects explicit sortComparator over automatic numeric sorting', ()
=> {
- const numericChoices = [
- [3, 'Three'],
- [1, 'One'],
- [2, 'Two'],
- ];
- const explicitComparator = jest.fn((a, b) =>
- a.label.localeCompare(b.label),
- );
-
- renderSelectControl({
- choices: numericChoices,
- sortComparator: explicitComparator,
- });
-
- const selectorWrapper = screen.getByLabelText('Row Limit', {
- selector: 'div',
- });
- expect(selectorWrapper).toBeInTheDocument();
+ expect(stringResult).toBeUndefined();
});
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Incomplete getSortComparator test coverage</b></div>
<div id="fix">
The new test for getSortComparator only checks numeric choices and string
choices, but misses critical cases like explicitComparator handling,
prioritization of options over choices, non-numeric inputs, and undefined
scenarios. The removed integration tests also verified that sorting applies
correctly in the component, which is now untested.
</div>
</div>
<small><i>Code Review Run #b6327f</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset-frontend/src/explore/components/controls/SelectControl.test.tsx:
##########
@@ -16,459 +16,185 @@
* specific language governing permissions and limitations
* under the License.
*/
+import { waitFor } from 'spec/helpers/testing-library';
import {
- act,
createEvent,
fireEvent,
render,
screen,
userEvent,
- within,
} from 'spec/helpers/testing-library';
+
import SelectControl, {
innerGetOptions,
areAllValuesNumbers,
getSortComparator,
} from 'src/explore/components/controls/SelectControl';
-const defaultProps: {
- choices: [string | number, string][];
- name: string;
- label: string;
- valueKey: string;
- onChange: jest.Mock;
-} = {
- choices: [
- ['1 year ago', '1 year ago'],
- ['1 week ago', '1 week ago'],
- ['today', 'today'],
- ],
+const choices: [string, string][] = [
+ ['1 year ago', '1 year ago'],
+ ['1 week ago', '1 week ago'],
+ ['today', 'today'],
+];
+
+const defaultProps = {
+ choices,
name: 'row_limit',
label: 'Row Limit',
valueKey: 'value',
onChange: jest.fn(),
};
-beforeEach(() => {
- jest.useFakeTimers();
-});
-
-afterEach(() => {
- jest.useRealTimers();
-});
-
-const options = [
+const expectedOptions = [
{ value: '1 year ago', label: '1 year ago' },
{ value: '1 week ago', label: '1 week ago' },
{ value: 'today', label: 'today' },
];
const renderSelectControl = (props = {}) => {
- const overrideProps = {
- ...defaultProps,
- ...props,
- };
- const { container } = render(<SelectControl {...overrideProps} />);
- return container;
+ const overrideProps = { ...defaultProps, ...props };
+ return render(<SelectControl {...overrideProps} />);
};
-// eslint-disable-next-line no-restricted-globals -- TODO: Migrate from
describe blocks
describe('SelectControl', () => {
- test('calls props.onChange when select', async () => {
+ test('calls props.onChange when select', () => {
renderSelectControl();
defaultProps.onChange(50);
expect(defaultProps.onChange).toHaveBeenCalledWith(50);
});
- // eslint-disable-next-line no-restricted-globals -- TODO: Migrate from
describe blocks
describe('render', () => {
- test('renders with Select by default', () => {
+ test('renders with Select by default', async () => {
renderSelectControl();
- const selectorWrapper = screen.getByLabelText('Row Limit', {
+
+ const selectorWrapper = await screen.findByLabelText('Row Limit', {
selector: 'div',
});
- const selectorInput = within(selectorWrapper).getByLabelText(
- 'Row Limit',
- { selector: 'input' },
- );
+
+ const selectorInput = await screen.findByLabelText('Row Limit', {
+ selector: 'input',
+ });
+
expect(selectorWrapper).toBeInTheDocument();
expect(selectorInput).toBeInTheDocument();
});
- test('renders as mode multiple', () => {
+ test('renders as mode multiple', async () => {
renderSelectControl({ multi: true });
- const selectorWrapper = screen.getByLabelText('Row Limit', {
- selector: 'div',
- });
- const selectorInput = within(selectorWrapper).getByLabelText(
- 'Row Limit',
- { selector: 'input' },
- );
- expect(selectorWrapper).toBeInTheDocument();
- expect(selectorInput).toBeInTheDocument();
- userEvent.click(selectorInput);
- expect(screen.getByText('Select all (3)')).toBeInTheDocument();
+ const input = screen.getByRole('combobox', { name: /row limit/i });
+ await userEvent.click(input);
+ expect(await screen.findByText('Select all (3)')).toBeInTheDocument();
});
- test('renders with allowNewOptions when freeForm', () => {
+ test('renders with allowNewOptions when freeForm', async () => {
renderSelectControl({ freeForm: true });
- const selectorWrapper = screen.getByLabelText('Row Limit', {
- selector: 'div',
+ const input = screen.getByRole('combobox', { name: /row limit/i });
+ await userEvent.click(input);
+ await userEvent.type(input, 'a new option');
+ await waitFor(() => {
+ expect(screen.getByText('a new option')).toBeInTheDocument();
});
- const selectorInput = within(selectorWrapper).getByLabelText(
- 'Row Limit',
- { selector: 'input' },
- );
- expect(selectorWrapper).toBeInTheDocument();
- expect(selectorInput).toBeInTheDocument();
-
- // Expect a new option to be selectable.
- userEvent.click(selectorInput);
- userEvent.type(selectorInput, 'a new option');
- act(() => jest.runAllTimers());
- expect(within(selectorWrapper).getByRole('option')).toHaveTextContent(
- 'a new option',
- );
});
- test('renders with allowNewOptions=false when freeForm=false', () => {
- const container = renderSelectControl({ freeForm: false });
- const selectorWrapper = screen.getByLabelText('Row Limit', {
- selector: 'div',
- });
- const selectorInput = within(selectorWrapper).getByLabelText(
- 'Row Limit',
- { selector: 'input' },
- );
- expect(selectorWrapper).toBeInTheDocument();
- expect(selectorInput).toBeInTheDocument();
+ test('renders with allowNewOptions=false when freeForm=false', async () =>
{
+ renderSelectControl({ freeForm: false });
- // Expect no new option to be selectable.
- userEvent.click(selectorInput);
- userEvent.type(selectorInput, 'a new option');
- act(() => jest.advanceTimersByTime(300));
+ const input = screen.getByRole('combobox', { name: /row limit/i });
- expect(
- container.querySelector('[role="option"]'),
- ).not.toBeInTheDocument();
- expect(
- within(selectorWrapper).getByText('No data', { selector: 'div' }),
- ).toBeInTheDocument();
+ await userEvent.click(input);
+ await userEvent.type(input, 'a new option');
+
+ expect(screen.queryByText('a new option')).not.toBeInTheDocument();
+
+ await waitFor(() => {
+ expect(
+ screen.getByText(/no data/i, {
+ selector: '.ant-empty-description',
+ }),
+ ).toBeInTheDocument();
+ });
});
- test('renders with tokenSeparators', () => {
+ test('renders with tokenSeparators', async () => {
renderSelectControl({ tokenSeparators: ['\n', '\t', ';'], multi: true });
- const selectorWrapper = screen.getByLabelText('Row Limit', {
- selector: 'div',
- });
- const selectorInput = within(selectorWrapper).getByLabelText(
- 'Row Limit',
- { selector: 'input' },
- );
- expect(selectorWrapper).toBeInTheDocument();
- expect(selectorInput).toBeInTheDocument();
+ const input = screen.getByRole('combobox', { name: /row limit/i });
+ await userEvent.click(input);
- userEvent.click(selectorInput);
- const paste = createEvent.paste(selectorInput, {
+ const paste = createEvent.paste(input, {
clipboardData: {
getData: () => '1 year ago;1 week ago',
},
});
- fireEvent(selectorInput, paste);
- const yearOption = screen.getByRole('option', { name: /1 year ago/i });
- expect(yearOption).toBeInTheDocument();
- expect(yearOption).toHaveAttribute('aria-selected', 'true');
- const weekOption = screen.getByRole('option', { name: /1 week ago/ });
- expect(weekOption?.getAttribute('aria-selected')).toEqual('true');
+ fireEvent(input, paste);
+
+ // FIX: Use selector to target the selection items, avoiding Space/Tag
duplicates
+ expect(
+ await screen.findByText('1 year ago', {
+ selector: '.ant-select-selection-item-content',
+ }),
+ ).toBeInTheDocument();
+ expect(
+ await screen.findByText('1 week ago', {
+ selector: '.ant-select-selection-item-content',
+ }),
+ ).toBeInTheDocument();
});
- // eslint-disable-next-line no-restricted-globals -- TODO: Migrate from
describe blocks
describe('empty placeholder', () => {
- // eslint-disable-next-line no-restricted-globals -- TODO: Migrate from
describe blocks
- describe('withMulti', () => {
- test('does not show a placeholder if there are no choices', () => {
- renderSelectControl({
- choices: [],
- multi: true,
- placeholder: 'add something',
- });
- expect(screen.queryByRole('option')).not.toBeInTheDocument();
- });
- });
- // eslint-disable-next-line no-restricted-globals -- TODO: Migrate from
describe blocks
- describe('withSingleChoice', () => {
- test('does not show a placeholder if there are no choices', async ()
=> {
- const container = renderSelectControl({
- choices: [],
- multi: false,
- placeholder: 'add something',
- });
- expect(
- container.querySelector('[role="option"]'),
- ).not.toBeInTheDocument();
- });
- });
- // eslint-disable-next-line no-restricted-globals -- TODO: Migrate from
describe blocks
- describe('all choices selected', () => {
- test('does not show a placeholder', () => {
- const container = renderSelectControl({
- multi: true,
- value: ['today', '1 year ago'],
- });
- expect(
- container.querySelector('[role="option"]'),
- ).not.toBeInTheDocument();
- expect(screen.queryByText('Select ...')).not.toBeInTheDocument();
- });
+ test('multi: no choices -> no options', async () => {
+ renderSelectControl({ choices: [], multi: true });
+ expect(screen.queryByRole('option')).not.toBeInTheDocument();
});
- });
- // eslint-disable-next-line no-restricted-globals -- TODO: Migrate from
describe blocks
- describe('when select is multi', () => {
- test('does not render the placeholder when a selection has been made',
() => {
- renderSelectControl({
- multi: true,
- value: ['today'],
- placeholder: 'add something',
- });
- expect(screen.queryByText('add something')).not.toBeInTheDocument();
+
+ test('single: no choices -> no options', async () => {
+ renderSelectControl({ choices: [], multi: false });
+ expect(screen.queryByRole('option')).not.toBeInTheDocument();
});
- });
- // eslint-disable-next-line no-restricted-globals -- TODO: Migrate from
describe blocks
- describe('when select is single', () => {
- test('does not render the placeholder when a selection has been made',
() => {
+
+ test('all selected -> no placeholder', async () => {
renderSelectControl({
multi: true,
- value: 50,
- placeholder: 'add something',
+ value: ['today', '1 year ago'],
});
- expect(screen.queryByText('add something')).not.toBeInTheDocument();
- });
- });
- });
- // eslint-disable-next-line no-restricted-globals -- TODO: Migrate from
describe blocks
- describe('getOptions', () => {
- test('returns the correct options', () => {
- expect(innerGetOptions(defaultProps)).toEqual(options);
+ // When all items are selected, the listbox should contain no options.
+ // This is the most reliable check for the 'empty' state in this
component.
+ expect(screen.queryByRole('option')).not.toBeInTheDocument();
+ });
});
});
- // eslint-disable-next-line no-restricted-globals -- TODO: Migrate from
describe blocks
- describe('areAllValuesNumbers', () => {
- test('returns true when all values are numbers (array format)', () => {
- const items = [
- [1, 'One'],
- [2, 'Two'],
- [3, 'Three'],
- ];
- expect(areAllValuesNumbers(items)).toBe(true);
- });
-
- test('returns false when some values are not numbers (array format)', ()
=> {
- const items = [
- [1, 'One'],
- ['two', 'Two'],
- [3, 'Three'],
- ];
- expect(areAllValuesNumbers(items)).toBe(false);
- });
-
- test('returns true when all values are numbers (object format)', () => {
- const items = [
- { value: 1, label: 'One' },
- { value: 2, label: 'Two' },
- { value: 3, label: 'Three' },
- ];
- expect(areAllValuesNumbers(items)).toBe(true);
+ describe('Utility Functions', () => {
+ test('innerGetOptions converts choices to options correctly', () => {
+ expect(innerGetOptions(defaultProps)).toEqual(expectedOptions);
});
- test('returns false when some values are not numbers (object format)', ()
=> {
- const items = [
- { value: 1, label: 'One' },
- { value: 'two', label: 'Two' },
- { value: 3, label: 'Three' },
- ];
- expect(areAllValuesNumbers(items)).toBe(false);
- });
-
- test('returns true when all values are numbers (primitive format)', () => {
- const items = [1, 2, 3];
- expect(areAllValuesNumbers(items)).toBe(true);
- });
-
- test('returns false when some values are not numbers (primitive format)',
() => {
- const items = [1, 'two', 3];
- expect(areAllValuesNumbers(items)).toBe(false);
- });
-
- test('works with custom valueKey', () => {
- const items = [
- { id: 1, label: 'One' },
- { id: 2, label: 'Two' },
- { id: 3, label: 'Three' },
- ];
- expect(areAllValuesNumbers(items, 'id')).toBe(true);
+ test('areAllValuesNumbers validates numeric arrays', () => {
+ expect(areAllValuesNumbers([1, 2, 3])).toBe(true);
+ expect(areAllValuesNumbers([1, 'two', 3])).toBe(false);
});
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Incomplete areAllValuesNumbers test coverage</b></div>
<div id="fix">
The new test for areAllValuesNumbers only validates primitive number arrays,
but the function handles object properties, custom value keys, and edge cases
like empty arrays. The removed tests covered these scenarios comprehensively.
</div>
</div>
<small><i>Code Review Run #b6327f</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
