bito-code-review[bot] commented on code in PR #42053:
URL: https://github.com/apache/superset/pull/42053#discussion_r3700148096
##########
superset-frontend/src/explore/components/controls/ConditionalFormattingControl/FormattingPopoverContent.test.tsx:
##########
@@ -117,23 +114,47 @@ test('renders the correct input fields based on the
selected operator', async ()
});
test('renders None for operator when Green for increase is selected', async ()
=> {
- render(
+ const { container } = render(
<FormattingPopoverContent
onChange={mockOnChange}
columns={columns}
extraColorChoices={extraColorChoices}
/>,
);
- // Select the 'Green for increase' color scheme
- fireEvent.change(screen.getAllByLabelText(/color scheme/i)[0], {
- target: { value: ColorSchemeEnum.Green },
+ const colorPickerTrigger = container.querySelector(
+ '.ant-color-picker-trigger',
+ );
+ expect(colorPickerTrigger).toBeInTheDocument();
+ await userEvent.click(colorPickerTrigger!);
+
+ await waitFor(() => {
+ expect(
+ document.querySelector('.ant-color-picker-presets-items'),
+ ).toBeInTheDocument();
});
- fireEvent.click(await screen.findByTitle(/green for increase/i));
+ const presets = document.querySelectorAll('.ant-color-picker-presets-color');
+ const greenPreset = Array.from(presets).find(preset => {
+ const inner = preset.querySelector('.ant-color-picker-color-block-inner');
+ return inner && inner.getAttribute('style')?.includes('0, 150, 0');
+ });
+
+ expect(greenPreset).toBeDefined();
+ expect(greenPreset).toBeInTheDocument();
+ const safeGreenPreset = greenPreset as HTMLElement;
+
+ const innerColorBlock = safeGreenPreset.querySelector(
+ '.ant-color-picker-color-block-inner',
+ );
+ expect(innerColorBlock).toHaveStyle({ background: 'rgba(0, 150, 0, 0.2)' });
+
+ expect(safeGreenPreset).toBeInTheDocument();
+ await userEvent.click(safeGreenPreset);
+ const operatorSelect = container.querySelector('[data-test="Operator"]');
+ expect(operatorSelect).toBeInTheDocument();
- // Assert that the operator is set to 'None'
- expect(screen.getByText(/none/i)).toBeInTheDocument();
+ expect(operatorSelect).toHaveTextContent(/none/i);
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Test selector targets missing attribute</b></div>
<div id="fix">
The test queries `[data-test="Operator"]` which does not exist in the
`Select` component (line 149-152 of FormattingPopoverContent.tsx shows only
`ariaLabel`, no `data-test` attribute). This causes `operatorSelect` to be
`null`, failing the assertion. Use `screen.getByLabelText('Operator')` to match
the pattern used elsewhere in this file (lines 72, 106).
</div>
</div>
<small><i>Code Review Run #f83edd</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/ColorPickerControl.test.tsx:
##########
@@ -92,3 +97,146 @@ describe('ColorPickerControl', () => {
expect(colorPickerTrigger).toBeInTheDocument();
});
});
+
+test('calls onChange with string key "Green" when resolveThemeTokens is true',
async () => {
+ const onChange = jest.fn();
+
+ render(
+ <ColorPickerControl
+ {...defaultProps}
+ onChange={onChange}
+ resolveThemeTokens
+ presets={[{ label: 'Special Colors', colors: ['Green', 'Red'] }]}
+ />,
+ );
+
+ const colorPickerTrigger = document.querySelector(
+ '.ant-color-picker-trigger',
+ );
+ expect(colorPickerTrigger).toBeInTheDocument();
+ await userEvent.click(colorPickerTrigger!);
+
+ await waitFor(() => {
+ expect(
+ document.querySelector('.ant-color-picker-presets-color'),
+ ).toBeInTheDocument();
+ });
+
+ const presets = document.querySelectorAll('.ant-color-picker-presets-color');
+ const greenPreset = presets[0];
+
+ expect(greenPreset).toBeInTheDocument();
+ await userEvent.click(greenPreset);
+
+ expect(onChange).toHaveBeenCalledWith('Green');
+});
+
+test('calls onChange with RGB object when resolveThemeTokens is false', async
() => {
+ const onChange = jest.fn();
+
+ render(
+ <ColorPickerControl
+ {...defaultProps}
+ onChange={onChange}
+ resolveThemeTokens={false}
+ presets={[{ label: 'Special Colors', colors: ['Green', 'Red'] }]}
+ />,
+ );
+
+ const colorPickerTrigger = document.querySelector(
+ '.ant-color-picker-trigger',
+ );
+ expect(colorPickerTrigger).toBeInTheDocument();
+ await userEvent.click(colorPickerTrigger!);
+
+ await waitFor(() => {
+ expect(
+ document.querySelector('.ant-color-picker-presets-color'),
+ ).toBeInTheDocument();
+ });
+
+ const presets = document.querySelectorAll('.ant-color-picker-presets-color');
+ const greenPreset = presets[0];
+
+ expect(greenPreset).toBeInTheDocument();
+ await userEvent.click(greenPreset);
+
+ expect(onChange).toHaveBeenCalledWith({ r: 0, g: 150, b: 0, a: 0.2 });
+});
+
+test('resolves colorSuccess theme token correctly when matching color is
selected', async () => {
+ const onChange = jest.fn();
+
+ jest
+ .spyOn(require('@apache-superset/core/theme'), 'useTheme')
+ .mockReturnValue({
+ colors: {
+ colorSuccess: 'rgba(82, 196, 26, 1)',
+ },
+ });
+
+ render(
+ <ColorPickerControl
+ {...defaultProps}
+ onChange={onChange}
+ resolveThemeTokens
+ presets={[{ label: 'Theme Tokens', colors: ['colorSuccess'] }]}
+ />,
+ );
+
+ const colorPickerTrigger = document.querySelector(
+ '.ant-color-picker-trigger',
+ );
+ expect(colorPickerTrigger).toBeInTheDocument();
+ await userEvent.click(colorPickerTrigger!);
+
+ await waitFor(() => {
+ expect(
+ document.querySelector('.ant-color-picker-presets-items'),
+ ).toBeInTheDocument();
+ });
+
+ const successPreset = document.querySelector(
+ '.ant-color-picker-presets-color [style*="82, 196, 26"]',
+ ) as HTMLElement | null;
+
+ expect(successPreset).toBeInTheDocument();
+
+ await userEvent.click(successPreset!);
+
+ expect(onChange).toHaveBeenCalledWith({ a: 1, b: 26, g: 196, r: 82 });
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Test regression: wrong expectation</b></div>
<div id="fix">
This test change introduces a regression. The test `'resolves colorSuccess
theme token correctly'` expects `onChange` to receive the resolved token name
`'colorSuccess'`, not the raw RGB object. The diff changes the expectation to
match broken behavior. The root cause is in `handleChange` where `reverseMap`
stores raw theme strings but `rgbaToHex(rgb)` produces hex format — causing the
lookup to fail. Either fix the implementation to store hex keys in `reverseMap`
(line 60), or revert this test change to catch the bug.
</div>
</div>
<small><i>Code Review Run #f83edd</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]
