codeant-ai-for-open-source[bot] commented on code in PR #38792:
URL: https://github.com/apache/superset/pull/38792#discussion_r3004443283
##########
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);
Review Comment:
**Suggestion:** The "calls props.onChange when select" test does not
exercise the SelectControl at all; it directly invokes the Jest mock `onChange`
and then asserts that it was called, so the test will still pass even if the
component stops calling `onChange` when the user selects an option. This hides
regressions in the actual wiring between the Select and the `onChange` prop.
Instead, the test should simulate a real user selection via `userEvent` and
then assert that the injected `onChange` mock was called with the expected
value. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ onChange wiring regressions not caught by this test.
- ⚠️ Chart control dropdown changes may silently stop updating state.
```
</details>
```suggestion
test('calls props.onChange when select', async () => {
const onChange = jest.fn();
renderSelectControl({ onChange });
const input = screen.getByRole('combobox', { name: /row limit/i });
await userEvent.click(input);
const option = await screen.findByText('1 year ago');
await userEvent.click(option);
expect(onChange).toHaveBeenCalledWith('1 year ago');
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open
`superset-frontend/src/explore/components/controls/SelectControl.tsx` and locate
the `onChange` implementation at lines 195–219, where it ultimately calls
`this.props.onChange?.(onChangeVal, []);`.
2. Introduce a regression by commenting out or removing the
`this.props.onChange?.(onChangeVal, []);` call so that `SelectControl` no
longer
propagates changes to its `onChange` prop.
3. Run the unit tests for this file (for example, `npm test --
SelectControl.test.tsx`) so
that
`superset-frontend/src/explore/components/controls/SelectControl.test.tsx`
executes
the test `"calls props.onChange when select"` at lines 60–64.
4. Observe that the test still passes because it directly calls
`defaultProps.onChange(50);` instead of simulating a user selection through
the rendered
`<SelectControl />`, so the broken wiring between the Select component and
`onChange` is
not detected, even though real usages of `SelectControl` like the chart
controls
configured with `type: 'SelectControl'` in
`plugins/legacy-plugin-chart-rose/src/controlPanel.tsx:65-81` would now fail
to receive
change notifications.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/src/explore/components/controls/SelectControl.test.tsx
**Line:** 60:63
**Comment:**
*Logic Error: The "calls props.onChange when select" test does not
exercise the SelectControl at all; it directly invokes the Jest mock `onChange`
and then asserts that it was called, so the test will still pass even if the
component stops calling `onChange` when the user selects an option. This hides
regressions in the actual wiring between the Select and the `onChange` prop.
Instead, the test should simulate a real user selection via `userEvent` and
then assert that the injected `onChange` mock was called with the expected
value.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38792&comment_hash=1010023365e2866de1377e7628c50f8bbd1746325e83585b238320ad442dbedf&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38792&comment_hash=1010023365e2866de1377e7628c50f8bbd1746325e83585b238320ad442dbedf&reaction=dislike'>👎</a>
##########
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'],
});
Review Comment:
**Suggestion:** The "empty placeholder" tests assert that no `role="option"`
elements exist without ever opening the Select dropdown, so they trivially pass
regardless of how the component behaves once opened. This means they do not
actually verify that there are no options rendered in the listbox for the "no
choices" and "all selected" cases. To correctly validate the behavior, each
test should first open the combobox via `userEvent.click` and then check for
the absence of options in the rendered listbox. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Empty-state dropdown regressions not validated by current tests.
- ⚠️ Multi-select chart controls may show incorrect placeholder options.
```
</details>
```suggestion
const input = screen.getByRole('combobox', { name: /row limit/i });
await userEvent.click(input);
expect(screen.queryByRole('option')).not.toBeInTheDocument();
});
test('single: no choices -> no options', async () => {
renderSelectControl({ choices: [], multi: false });
const input = screen.getByRole('combobox', { name: /row limit/i });
await userEvent.click(input);
expect(screen.queryByRole('option')).not.toBeInTheDocument();
});
test('all selected -> no placeholder', async () => {
renderSelectControl({
multi: true,
value: ['today', '1 year ago'],
});
const input = screen.getByRole('combobox', { name: /row limit/i });
await userEvent.click(input);
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. In `superset-frontend/src/explore/components/controls/SelectControl.tsx`,
or in the
underlying Select implementation from `@superset-ui/core/components`,
introduce a bug that
causes at least one option element to be rendered in the dropdown even when
`choices` is
empty or all options are already selected (for example, by always rendering
a dummy
option).
2. Run the unit tests for
`superset-frontend/src/explore/components/controls/SelectControl.test.tsx`
so that the
`"empty placeholder"` suite at lines 143–164 executes.
3. Observe that each test (`'multi: no choices -> no options'`, `'single: no
choices -> no
options'`, and `'all selected -> no placeholder'`) only calls
`renderSelectControl(...)`
and immediately asserts `screen.queryByRole('option')` on the closed
component, never
clicking the combobox to open the listbox, so they continue to pass even
though the
dropdown would show options when opened.
4. In real chart configuration UIs where `SelectControl` is used for
multi-select and
time-shift controls, such as the `time_compare` control with `multi: true`
and `choices`
configured in
`plugins/legacy-plugin-chart-rose/src/controlPanel.tsx:189-195`, users would
see incorrect non-empty dropdowns for empty/all-selected states, but these
regressions
would not be caught by the existing tests.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/src/explore/components/controls/SelectControl.test.tsx
**Line:** 146:158
**Comment:**
*Logic Error: The "empty placeholder" tests assert that no
`role="option"` elements exist without ever opening the Select dropdown, so
they trivially pass regardless of how the component behaves once opened. This
means they do not actually verify that there are no options rendered in the
listbox for the "no choices" and "all selected" cases. To correctly validate
the behavior, each test should first open the combobox via `userEvent.click`
and then check for the absence of options in the rendered listbox.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38792&comment_hash=59fd2e56549ee5928b3eadaf2d16b5b647a8d6148781c1eda7f4909d2ed980da&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38792&comment_hash=59fd2e56549ee5928b3eadaf2d16b5b647a8d6148781c1eda7f4909d2ed980da&reaction=dislike'>👎</a>
--
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]
