bito-code-review[bot] commented on code in PR #42053:
URL: https://github.com/apache/superset/pull/42053#discussion_r3707503983
##########
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('colorSuccess');
+});
+
+test('handles theme with nested colors object', () => {
+ jest
+ .spyOn(require('@apache-superset/core/theme'), 'useTheme')
+ .mockReturnValue({
+ colors: { primary: '#007bff' },
+ });
+
+ const { container } = render(<ColorPickerControl {...defaultProps} />);
+ expect(
+ container.querySelector('.ant-color-picker-trigger'),
+ ).toBeInTheDocument();
+});
+
+test('handles theme without colors field', () => {
+ jest
+ .spyOn(require('@apache-superset/core/theme'), 'useTheme')
+ .mockReturnValue({
+ primary: '#007bff',
+ });
+
+ const { container } = render(<ColorPickerControl {...defaultProps} />);
+ expect(
+ container.querySelector('.ant-color-picker-trigger'),
+ ).toBeInTheDocument();
+});
+
+test('handles undefined theme gracefully', () => {
+ jest
+ .spyOn(require('@apache-superset/core/theme'), 'useTheme')
+ .mockReturnValue(undefined);
+
+ expect(() => render(<ColorPickerControl {...defaultProps} />)).not.toThrow();
+});
Review Comment:
<!-- Bito Reply -->
The suggestion to move the tests inside the `describe('ColorPickerControl')`
block is correct. Placing tests outside the `describe` block prevents them from
benefiting from the setup and teardown logic (such as `beforeAll` and
`beforeEach`) defined within the block, which can lead to test isolation issues
and potential failures in CI due to stale mocks.
--
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]
