You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "sadpandajoe (via GitHub)" <gi...@apache.org> on 2026/10/01 00:17:21 UTC
[PR] test(explore): cover column config, conditional formatting, and contour controls [superset]
sadpandajoe opened a new pull request, #44843:
URL: https://github.com/apache/superset/pull/44843
### SUMMARY
Adds React Testing Library coverage for three CRUD-style Explore controls. This change touches test files only.
- **Column config** (`ColumnConfigControl.test.tsx` and `ControlForm.test.tsx`, both new)
- Rows: one row per column, filtering by `appliedColumnNames`, and the 12-column "show all" threshold with its toggle.
- Popover edits are debounced, then merged into the stored config.
- Configs for columns that are no longer known or are hidden get pruned.
- Time comparison columns get a General tab.
- Child columns configured as not visible show the hidden-column icon.
- The `__timestamp` alias label.
- `ControlForm`: field seeding, merged and debounced edits, validation errors, and empty values.
- **Conditional formatting** (`ConditionalFormattingControl.test.tsx`)
- Add, edit and delete flows through the popover. Each checks that the `onChange` list keeps the other formatters in place.
- Formatters whose column leaves the column options get pruned, with an opt-out.
- **Contours** (`ContourControl/*.test.tsx`, new)
- `ContourPopoverControl`: picks the isoline or isoband tab, saves the right shape, switches between tabs, and validates integers and threshold order.
- `ContourPopoverTrigger`: controlled and uncontrolled visibility.
- `ContourOption`: labels, remove, and edit.
- The `ContourControl` list: defaults, z-index ordering, add, edit and remove.
Each new test was checked by breaking the related product code, and the test failed each time.
Some behavior that looks like a bug was left unpinned on purpose, so a later fix won't have to rewrite these tests:
- `ColumnConfigControl` saves edits to the time column under its display alias ("Time"). The table plugins look the config up under `__timestamp`.
- The contour popover saves a missing upper threshold or stroke width as `NaN`.
- An isoband with an upper threshold of `0` is treated as an isoline, because of a truthiness check.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A: tests only.
### TESTING INSTRUCTIONS
```bash
cd superset-frontend
npx jest \
src/explore/components/controls/ColumnConfigControl \
src/explore/components/controls/ConditionalFormattingControl \
src/explore/components/controls/ContourControl
```
### ADDITIONAL INFORMATION
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
π€ Generated with [Claude Code](https://claude.com/claude-code)
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] test(explore): cover column config, conditional formatting, and contour controls [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #44843:
URL: https://github.com/apache/superset/pull/44843#discussion_r4167687423
##########
superset-frontend/src/explore/components/controls/ConditionalFormattingControl/ConditionalFormattingControl.test.tsx:
##########
@@ -101,3 +108,210 @@ test('renders verbose column name when available', () => {
);
expect(screen.getByText('My Column is false')).toBeInTheDocument();
});
+
+const numericColumns = [
+ { label: 'Column A', value: 'col_a', dataType: GenericDataType.Numeric },
+ { label: 'Column B', value: 'col_b', dataType: GenericDataType.Numeric },
+];
+
+const greaterThanA: ConditionalFormattingConfig = {
+ column: 'col_a',
+ operator: Comparator.GreaterThan,
+ targetValue: 1,
+ colorScheme: 'colorSuccess',
+};
+
+const lessThanB: ConditionalFormattingConfig = {
+ column: 'col_b',
+ operator: Comparator.LessThan,
+ targetValue: 2,
+ colorScheme: 'colorSuccess',
+};
+
+const renderCrud = (
+ props: Partial<ComponentProps<typeof ConditionalFormattingControl>> = {},
+) => {
+ const onChange = jest.fn();
+ const utils = render(
+ <ConditionalFormattingControl
+ {...defaultProps}
+ columnOptions={numericColumns}
+ onChange={onChange}
+ {...props}
+ />,
+ );
+ return { onChange, ...utils };
+};
+
+const lastChange = (onChange: jest.Mock) =>
+ onChange.mock.calls[onChange.mock.calls.length - 1][0];
+
+// The popover Selects render their options inline next to the trigger, so
+// scope the option lookup to the Select that was opened.
+const pickColumn = async (optionLabel: string) => {
+ const trigger = await screen.findByRole('combobox', {
+ name: 'Select column',
+ });
+ const container = trigger.closest('.ant-select')!
+ .parentElement as HTMLElement;
+ await userEvent.click(trigger);
+ await userEvent.click(await within(container).findByText(optionLabel));
+};
+
+test('opens the add formatter popover from the add label', async () => {
+ renderCrud();
+ expect(screen.queryByText('Add new formatter')).not.toBeInTheDocument();
+
+ await userEvent.click(screen.getByText('Add new color formatter'));
+
+ expect(await screen.findByText('Add new formatter')).toBeInTheDocument();
+ expect(screen.getByText('Apply')).toBeInTheDocument();
+});
+
+test('adds a formatter and reports it through onChange when the popover is applied', async () => {
+ const { onChange } = renderCrud();
+
+ await userEvent.click(screen.getByText('Add new color formatter'));
+ await pickColumn('Column B');
+ await userEvent.click(await screen.findByText('Apply'));
+
+ await waitFor(() =>
+ expect(lastChange(onChange)).toEqual([
+ expect.objectContaining({ column: 'col_b' }),
+ ]),
+ );
+ expect(screen.getByText('col_b')).toBeInTheDocument();
+ await waitFor(() =>
+ expect(screen.queryByText('Add new formatter')).not.toBeInTheDocument(),
+ );
+});
+
+test('appends a new formatter after the existing ones', async () => {
+ const { onChange } = renderCrud({ value: [greaterThanA] });
+
+ await userEvent.click(screen.getByText('Add new color formatter'));
+ await pickColumn('Column B');
+ await userEvent.click(await screen.findByText('Apply'));
+
+ await waitFor(() => expect(lastChange(onChange)).toHaveLength(2));
+ expect(lastChange(onChange)[0]).toEqual(greaterThanA);
+ expect(lastChange(onChange)[1]).toEqual(
+ expect.objectContaining({ column: 'col_b' }),
+ );
+});
+
+test('opens the edit popover prefilled from the clicked formatter', async () => {
+ renderCrud({ value: [greaterThanA, lessThanB] });
+
+ await userEvent.click(screen.getByText('col_b < 2'));
+
+ expect(await screen.findByText('Edit formatter')).toBeInTheDocument();
+ expect(screen.getByLabelText('Target value')).toHaveValue('2');
+});
+
+test('replaces the edited formatter at its index and keeps the others', async () => {
+ const { onChange } = renderCrud({ value: [greaterThanA, lessThanB] });
+
+ await userEvent.click(screen.getByText('col_b < 2'));
+ await screen.findByText('Edit formatter');
+ const targetValue = screen.getByLabelText('Target value');
+ await userEvent.clear(targetValue);
+ await userEvent.type(targetValue, '7');
+ await userEvent.click(screen.getByText('Apply'));
+
+ await waitFor(() =>
+ expect(lastChange(onChange)).toEqual([
+ greaterThanA,
+ expect.objectContaining({
+ column: 'col_b',
+ operator: Comparator.LessThan,
+ targetValue: 7,
+ }),
+ ]),
+ );
+ expect(screen.getByText('col_b < 7')).toBeInTheDocument();
+ expect(screen.queryByText('col_b < 2')).not.toBeInTheDocument();
Review Comment:
β
**Customized review instruction saved!**
**Instruction:**
> Do not require truly concurrent edit tests for this control; only one popover can be open at a time, so cover sequential edits and delete-then-edit behavior instead.
**Applied to:**
- `**/ConditionalFormattingControl.test.tsx`
---
π‘ *To manage or update this instruction, visit: [CodeAnt AI Settings](https://app.codeant.ai/org/settings/learnings)*
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] test(explore): cover column config, conditional formatting, and contour controls [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on code in PR #44843:
URL: https://github.com/apache/superset/pull/44843#discussion_r4167684031
##########
superset-frontend/src/explore/components/controls/ColumnConfigControl/ControlForm/ControlForm.test.tsx:
##########
@@ -0,0 +1,108 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import {
+ render,
+ screen,
+ userEvent,
+ within,
+ waitFor,
+} from 'spec/helpers/testing-library';
+import ControlForm, { ControlFormItem, ControlFormRow } from '.';
+
+const noDigits = (value: unknown) =>
+ /\d/.test(String(value)) ? 'Digits are not allowed' : false;
+
+const renderForm = (
+ props: {
+ value?: Record<string, string>;
+ validators?: ((value: unknown) => string | false)[];
+ debounceDelay?: number;
+ } = {},
+) => {
+ const onChange = jest.fn();
+ render(
+ <ControlForm onChange={onChange} value={props.value}>
+ <ControlFormRow>
+ <ControlFormItem
+ name="title"
+ controlType="Input"
+ label="Title"
+ description="Title description"
+ validators={props.validators}
+ debounceDelay={props.debounceDelay}
+ />
+ <ControlFormItem
+ name="note"
+ controlType="Input"
+ label="Note"
+ description="Note description"
+ />
+ </ControlFormRow>
+ </ControlForm>,
+ );
+ return { onChange };
+};
+
+test('seeds each field from the matching key of the form value', () => {
+ renderForm({ value: { title: 'Hello', note: 'World' } });
+ expect(
+ screen
+ .getAllByRole('textbox')
+ .map(input => (input as HTMLInputElement).value),
+ ).toEqual(['Hello', 'World']);
+});
+
+test('propagates an edit to the form merged with the existing value', async () => {
+ const { onChange } = renderForm({ value: { note: 'World' } });
+ await userEvent.type(screen.getAllByRole('textbox')[0], 'Hi');
+
+ await waitFor(() =>
+ expect(onChange).toHaveBeenLastCalledWith({ note: 'World', title: 'Hi' }),
+ );
+});
Review Comment:
Added a test with a stateful parent (the debounced form change is fed back as the value) that edits two fields in sequence after the first change flushes, and asserts both values persist. It fails if the form payload stops merging the existing value. Editing two fields inside the 250ms debounce window does drop the first edit: the shared debounced handler only fires the last call, built from the stale `value`. That is a component bug, so it isn't pinned in this test-only change and is tracked separately.
##########
superset-frontend/src/explore/components/controls/ConditionalFormattingControl/ConditionalFormattingControl.test.tsx:
##########
@@ -101,3 +108,210 @@ test('renders verbose column name when available', () => {
);
expect(screen.getByText('My Column is false')).toBeInTheDocument();
});
+
+const numericColumns = [
+ { label: 'Column A', value: 'col_a', dataType: GenericDataType.Numeric },
+ { label: 'Column B', value: 'col_b', dataType: GenericDataType.Numeric },
+];
+
+const greaterThanA: ConditionalFormattingConfig = {
+ column: 'col_a',
+ operator: Comparator.GreaterThan,
+ targetValue: 1,
+ colorScheme: 'colorSuccess',
+};
+
+const lessThanB: ConditionalFormattingConfig = {
+ column: 'col_b',
+ operator: Comparator.LessThan,
+ targetValue: 2,
+ colorScheme: 'colorSuccess',
+};
+
+const renderCrud = (
+ props: Partial<ComponentProps<typeof ConditionalFormattingControl>> = {},
+) => {
+ const onChange = jest.fn();
+ const utils = render(
+ <ConditionalFormattingControl
+ {...defaultProps}
+ columnOptions={numericColumns}
+ onChange={onChange}
+ {...props}
+ />,
+ );
+ return { onChange, ...utils };
+};
+
+const lastChange = (onChange: jest.Mock) =>
+ onChange.mock.calls[onChange.mock.calls.length - 1][0];
+
+// The popover Selects render their options inline next to the trigger, so
+// scope the option lookup to the Select that was opened.
+const pickColumn = async (optionLabel: string) => {
+ const trigger = await screen.findByRole('combobox', {
+ name: 'Select column',
+ });
+ const container = trigger.closest('.ant-select')!
+ .parentElement as HTMLElement;
+ await userEvent.click(trigger);
+ await userEvent.click(await within(container).findByText(optionLabel));
+};
+
+test('opens the add formatter popover from the add label', async () => {
+ renderCrud();
+ expect(screen.queryByText('Add new formatter')).not.toBeInTheDocument();
+
+ await userEvent.click(screen.getByText('Add new color formatter'));
+
+ expect(await screen.findByText('Add new formatter')).toBeInTheDocument();
+ expect(screen.getByText('Apply')).toBeInTheDocument();
+});
+
+test('adds a formatter and reports it through onChange when the popover is applied', async () => {
+ const { onChange } = renderCrud();
+
+ await userEvent.click(screen.getByText('Add new color formatter'));
+ await pickColumn('Column B');
+ await userEvent.click(await screen.findByText('Apply'));
+
+ await waitFor(() =>
+ expect(lastChange(onChange)).toEqual([
+ expect.objectContaining({ column: 'col_b' }),
+ ]),
+ );
+ expect(screen.getByText('col_b')).toBeInTheDocument();
+ await waitFor(() =>
+ expect(screen.queryByText('Add new formatter')).not.toBeInTheDocument(),
+ );
+});
+
+test('appends a new formatter after the existing ones', async () => {
+ const { onChange } = renderCrud({ value: [greaterThanA] });
+
+ await userEvent.click(screen.getByText('Add new color formatter'));
+ await pickColumn('Column B');
+ await userEvent.click(await screen.findByText('Apply'));
+
+ await waitFor(() => expect(lastChange(onChange)).toHaveLength(2));
+ expect(lastChange(onChange)[0]).toEqual(greaterThanA);
+ expect(lastChange(onChange)[1]).toEqual(
+ expect.objectContaining({ column: 'col_b' }),
+ );
+});
+
+test('opens the edit popover prefilled from the clicked formatter', async () => {
+ renderCrud({ value: [greaterThanA, lessThanB] });
+
+ await userEvent.click(screen.getByText('col_b < 2'));
+
+ expect(await screen.findByText('Edit formatter')).toBeInTheDocument();
+ expect(screen.getByLabelText('Target value')).toHaveValue('2');
+});
+
+test('replaces the edited formatter at its index and keeps the others', async () => {
+ const { onChange } = renderCrud({ value: [greaterThanA, lessThanB] });
+
+ await userEvent.click(screen.getByText('col_b < 2'));
+ await screen.findByText('Edit formatter');
+ const targetValue = screen.getByLabelText('Target value');
+ await userEvent.clear(targetValue);
+ await userEvent.type(targetValue, '7');
+ await userEvent.click(screen.getByText('Apply'));
+
+ await waitFor(() =>
+ expect(lastChange(onChange)).toEqual([
+ greaterThanA,
+ expect.objectContaining({
+ column: 'col_b',
+ operator: Comparator.LessThan,
+ targetValue: 7,
+ }),
+ ]),
+ );
+ expect(screen.getByText('col_b < 7')).toBeInTheDocument();
+ expect(screen.queryByText('col_b < 2')).not.toBeInTheDocument();
Review Comment:
Added a delete-then-edit test (the remaining formatter is edited at its shifted index) and a test that edits two formatters one after the other and asserts both persist. Both fail if `onEdit` builds from stale props instead of component state. Truly concurrent edits can't occur, since only one popover is open at a time and its handler is rebuilt on every state change.
##########
superset-frontend/src/explore/components/controls/ContourControl/ContourOption.test.tsx:
##########
@@ -0,0 +1,111 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { render, screen, userEvent } from 'spec/helpers/testing-library';
+import ContourOption from './ContourOption';
+import { ContourOptionProps, ContourType } from './types';
+
+const color = { r: 10, g: 20, b: 30, a: 100 };
+
+const isoline: ContourType = {
+ lowerThreshold: 4,
+ color,
+ strokeWidth: 3,
+ zIndex: 10,
+};
+
+const isoband: ContourType = {
+ lowerThreshold: 6,
+ upperThreshold: 10,
+ color,
+ zIndex: 20,
+};
+
+const renderOption = (props: Partial<ContourOptionProps> = {}) => {
+ const handlers = {
+ saveContour: jest.fn(),
+ onClose: jest.fn(),
+ onShift: jest.fn(),
+ };
+ render(
+ <ContourOption contour={isoline} index={0} {...handlers} {...props} />,
+ { useDnd: true },
+ );
+ return handlers;
+};
+
+test('labels an isoline with its threshold, color and stroke width', () => {
+ renderOption({ contour: isoline });
+ expect(
+ screen.getByText(
+ 'Threshold: 4, color: rgba(10, 20, 30, 1), stroke width: 3',
+ ),
+ ).toBeInTheDocument();
+});
+
+test('labels an isoband with its threshold range and color but no stroke width', () => {
+ renderOption({ contour: isoband });
+ expect(
+ screen.getByText('Threshold: [6, 10], color: rgba(10, 20, 30, 1)'),
+ ).toBeInTheDocument();
+ expect(screen.queryByText(/stroke width/)).not.toBeInTheDocument();
+});
+
+test('treats a contour with an upper threshold as an isoband', () => {
+ renderOption({ contour: { ...isoline, upperThreshold: 8 } });
+ expect(screen.getByText(/Threshold: \[4, 8\]/)).toBeInTheDocument();
+});
+
+test('treats a contour without an upper threshold as an isoline', () => {
+ renderOption({
+ contour: { ...isoband, upperThreshold: undefined, strokeWidth: 2 },
+ });
+ expect(screen.getByText(/stroke width: 2/)).toBeInTheDocument();
+});
+
+test('calls onClose with the option index when removed', async () => {
+ const { onClose } = renderOption({ index: 2 });
+
+ await userEvent.click(screen.getByTestId('remove-control-button'));
+
+ expect(onClose).toHaveBeenCalledWith(2);
+});
+
+test('opens the contour popover prefilled from the contour when clicked', async () => {
+ renderOption({ contour: isoband });
+
+ await userEvent.click(screen.getByTestId('option-label'));
+
+ expect(await screen.findByRole('tab', { name: 'Isoband' })).toHaveAttribute(
+ 'aria-selected',
+ 'true',
+ );
+ const inputs = screen.getAllByTestId('inline-name') as HTMLInputElement[];
+ expect(inputs.map(input => input.value)).toEqual(['6', '10']);
+});
+
+test('saves an edited contour through saveContour', async () => {
Review Comment:
Renamed to "saves the prefilled contour values unchanged through saveContour", which matches what it does. Editing a field is covered by "edits a contour in place through its popover" in `index.test.tsx`.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] test(explore): cover column config, conditional formatting, and contour controls [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #44843:
URL: https://github.com/apache/superset/pull/44843#issuecomment-5922151566
<!-- Bito Reply -->
The flagged issue is correct. Testing only a single field in a form that uses debounced updates can fail to detect race conditions where concurrent updates to different fields overwrite each other with stale state. To resolve this, you should update the test to perform sequential interactions on multiple fields and verify that the final state correctly reflects all changes, rather than just the last one.
Would you like me to fetch all other comments on this PR to validate them and implement fixes for those as well?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] test(explore): cover column config, conditional formatting, and contour controls [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on PR #44843:
URL: https://github.com/apache/superset/pull/44843#issuecomment-6352675640
## CodeAnt PR Risk: Medium Risk
- The PR needs attention before merging because rapid edits to separate form fields can overwrite an earlier value during the debounce window.
- ControlForm updates built from stale values can discard an earlier field edit; the added test only checks edits made after the first change flushes.
**Assessed commit:** `ffadc610f6be`
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] test(explore): cover column config, conditional formatting, and contour controls [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #44843:
URL: https://github.com/apache/superset/pull/44843#discussion_r4150582465
##########
superset-frontend/src/explore/components/controls/ConditionalFormattingControl/ConditionalFormattingControl.test.tsx:
##########
@@ -101,3 +108,210 @@ test('renders verbose column name when available', () => {
);
expect(screen.getByText('My Column is false')).toBeInTheDocument();
});
+
+const numericColumns = [
+ { label: 'Column A', value: 'col_a', dataType: GenericDataType.Numeric },
+ { label: 'Column B', value: 'col_b', dataType: GenericDataType.Numeric },
+];
+
+const greaterThanA: ConditionalFormattingConfig = {
+ column: 'col_a',
+ operator: Comparator.GreaterThan,
+ targetValue: 1,
+ colorScheme: 'colorSuccess',
+};
+
+const lessThanB: ConditionalFormattingConfig = {
+ column: 'col_b',
+ operator: Comparator.LessThan,
+ targetValue: 2,
+ colorScheme: 'colorSuccess',
+};
+
+const renderCrud = (
+ props: Partial<ComponentProps<typeof ConditionalFormattingControl>> = {},
+) => {
+ const onChange = jest.fn();
+ const utils = render(
+ <ConditionalFormattingControl
+ {...defaultProps}
+ columnOptions={numericColumns}
+ onChange={onChange}
+ {...props}
+ />,
+ );
+ return { onChange, ...utils };
+};
+
+const lastChange = (onChange: jest.Mock) =>
+ onChange.mock.calls[onChange.mock.calls.length - 1][0];
+
+// The popover Selects render their options inline next to the trigger, so
+// scope the option lookup to the Select that was opened.
+const pickColumn = async (optionLabel: string) => {
+ const trigger = await screen.findByRole('combobox', {
+ name: 'Select column',
+ });
+ const container = trigger.closest('.ant-select')!
+ .parentElement as HTMLElement;
+ await userEvent.click(trigger);
+ await userEvent.click(await within(container).findByText(optionLabel));
+};
+
+test('opens the add formatter popover from the add label', async () => {
+ renderCrud();
+ expect(screen.queryByText('Add new formatter')).not.toBeInTheDocument();
+
+ await userEvent.click(screen.getByText('Add new color formatter'));
+
+ expect(await screen.findByText('Add new formatter')).toBeInTheDocument();
+ expect(screen.getByText('Apply')).toBeInTheDocument();
+});
+
+test('adds a formatter and reports it through onChange when the popover is applied', async () => {
+ const { onChange } = renderCrud();
+
+ await userEvent.click(screen.getByText('Add new color formatter'));
+ await pickColumn('Column B');
+ await userEvent.click(await screen.findByText('Apply'));
+
+ await waitFor(() =>
+ expect(lastChange(onChange)).toEqual([
+ expect.objectContaining({ column: 'col_b' }),
+ ]),
+ );
+ expect(screen.getByText('col_b')).toBeInTheDocument();
+ await waitFor(() =>
+ expect(screen.queryByText('Add new formatter')).not.toBeInTheDocument(),
+ );
+});
+
+test('appends a new formatter after the existing ones', async () => {
+ const { onChange } = renderCrud({ value: [greaterThanA] });
+
+ await userEvent.click(screen.getByText('Add new color formatter'));
+ await pickColumn('Column B');
+ await userEvent.click(await screen.findByText('Apply'));
+
+ await waitFor(() => expect(lastChange(onChange)).toHaveLength(2));
+ expect(lastChange(onChange)[0]).toEqual(greaterThanA);
+ expect(lastChange(onChange)[1]).toEqual(
+ expect.objectContaining({ column: 'col_b' }),
+ );
+});
+
+test('opens the edit popover prefilled from the clicked formatter', async () => {
+ renderCrud({ value: [greaterThanA, lessThanB] });
+
+ await userEvent.click(screen.getByText('col_b < 2'));
+
+ expect(await screen.findByText('Edit formatter')).toBeInTheDocument();
+ expect(screen.getByLabelText('Target value')).toHaveValue('2');
+});
+
+test('replaces the edited formatter at its index and keeps the others', async () => {
+ const { onChange } = renderCrud({ value: [greaterThanA, lessThanB] });
+
+ await userEvent.click(screen.getByText('col_b < 2'));
+ await screen.findByText('Edit formatter');
+ const targetValue = screen.getByLabelText('Target value');
+ await userEvent.clear(targetValue);
+ await userEvent.type(targetValue, '7');
+ await userEvent.click(screen.getByText('Apply'));
+
+ await waitFor(() =>
+ expect(lastChange(onChange)).toEqual([
+ greaterThanA,
+ expect.objectContaining({
+ column: 'col_b',
+ operator: Comparator.LessThan,
+ targetValue: 7,
+ }),
+ ]),
+ );
+ expect(screen.getByText('col_b < 7')).toBeInTheDocument();
+ expect(screen.queryByText('col_b < 2')).not.toBeInTheDocument();
Review Comment:
**Suggestion:** These edit tests serialize updates, so they do not detect stale render-state updates that lose another formatter during overlapping edits or delete-then-edit operations.
**Assessment:** π `Major` Β· π `Occurrence: Rarely` Β· π·οΈ `Race condition`
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill) [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=0711d0ffa57d44828a4c517c37f52ad6&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=0711d0ffa57d44828a4c517c37f52ad6&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<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/ConditionalFormattingControl/ConditionalFormattingControl.test.tsx
**Line:** 213:233
**Comment:**
*Race Condition: These edit tests serialize updates, so they do not detect stale render-state updates that lose another formatter during overlapping edits or delete-then-edit operations.
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.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F44843&comment_hash=fef235cdb306d1b92a8bfd932e1c9fa037b3255c2e5b03dada5cccc301b1e571&reaction=like'>π</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F44843&comment_hash=fef235cdb306d1b92a8bfd932e1c9fa037b3255c2e5b03dada5cccc301b1e571&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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] test(explore): cover column config, conditional formatting, and contour controls [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #44843:
URL: https://github.com/apache/superset/pull/44843#discussion_r4167686475
##########
superset-frontend/src/explore/components/controls/ContourControl/ContourOption.test.tsx:
##########
@@ -0,0 +1,111 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { render, screen, userEvent } from 'spec/helpers/testing-library';
+import ContourOption from './ContourOption';
+import { ContourOptionProps, ContourType } from './types';
+
+const color = { r: 10, g: 20, b: 30, a: 100 };
+
+const isoline: ContourType = {
+ lowerThreshold: 4,
+ color,
+ strokeWidth: 3,
+ zIndex: 10,
+};
+
+const isoband: ContourType = {
+ lowerThreshold: 6,
+ upperThreshold: 10,
+ color,
+ zIndex: 20,
+};
+
+const renderOption = (props: Partial<ContourOptionProps> = {}) => {
+ const handlers = {
+ saveContour: jest.fn(),
+ onClose: jest.fn(),
+ onShift: jest.fn(),
+ };
+ render(
+ <ContourOption contour={isoline} index={0} {...handlers} {...props} />,
+ { useDnd: true },
+ );
+ return handlers;
+};
+
+test('labels an isoline with its threshold, color and stroke width', () => {
+ renderOption({ contour: isoline });
+ expect(
+ screen.getByText(
+ 'Threshold: 4, color: rgba(10, 20, 30, 1), stroke width: 3',
+ ),
+ ).toBeInTheDocument();
+});
+
+test('labels an isoband with its threshold range and color but no stroke width', () => {
+ renderOption({ contour: isoband });
+ expect(
+ screen.getByText('Threshold: [6, 10], color: rgba(10, 20, 30, 1)'),
+ ).toBeInTheDocument();
+ expect(screen.queryByText(/stroke width/)).not.toBeInTheDocument();
+});
+
+test('treats a contour with an upper threshold as an isoband', () => {
+ renderOption({ contour: { ...isoline, upperThreshold: 8 } });
+ expect(screen.getByText(/Threshold: \[4, 8\]/)).toBeInTheDocument();
+});
+
+test('treats a contour without an upper threshold as an isoline', () => {
+ renderOption({
+ contour: { ...isoband, upperThreshold: undefined, strokeWidth: 2 },
+ });
+ expect(screen.getByText(/stroke width: 2/)).toBeInTheDocument();
+});
+
+test('calls onClose with the option index when removed', async () => {
+ const { onClose } = renderOption({ index: 2 });
+
+ await userEvent.click(screen.getByTestId('remove-control-button'));
+
+ expect(onClose).toHaveBeenCalledWith(2);
+});
+
+test('opens the contour popover prefilled from the contour when clicked', async () => {
+ renderOption({ contour: isoband });
+
+ await userEvent.click(screen.getByTestId('option-label'));
+
+ expect(await screen.findByRole('tab', { name: 'Isoband' })).toHaveAttribute(
+ 'aria-selected',
+ 'true',
+ );
+ const inputs = screen.getAllByTestId('inline-name') as HTMLInputElement[];
+ expect(inputs.map(input => input.value)).toEqual(['6', '10']);
+});
+
+test('saves an edited contour through saveContour', async () => {
Review Comment:
<!-- Bito Reply -->
The suggestion to rename the test is appropriate. The current test name implies an edit operation, but the test body performs a save operation on a prefilled contour without modifying any fields, which is better described by the proposed name.
**superset-frontend/src/explore/components/controls/ContourControl/ContourOption.test.tsx**
```
test('saves the prefilled contour values unchanged through saveContour', async () => {
```
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] test(explore): cover column config, conditional formatting, and contour controls [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #44843:
URL: https://github.com/apache/superset/pull/44843#issuecomment-5963809403
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers an incremental AI Review.
- `/review full` - Manually triggers a full AI Review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] test(explore): cover column config, conditional formatting, and contour controls [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #44843:
URL: https://github.com/apache/superset/pull/44843#issuecomment-5922089240
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] test(explore): cover column config, conditional formatting, and contour controls [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #44843:
URL: https://github.com/apache/superset/pull/44843#discussion_r4150580976
##########
superset-frontend/src/explore/components/controls/ColumnConfigControl/ControlForm/ControlForm.test.tsx:
##########
@@ -0,0 +1,108 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import {
+ render,
+ screen,
+ userEvent,
+ within,
+ waitFor,
+} from 'spec/helpers/testing-library';
+import ControlForm, { ControlFormItem, ControlFormRow } from '.';
+
+const noDigits = (value: unknown) =>
+ /\d/.test(String(value)) ? 'Digits are not allowed' : false;
+
+const renderForm = (
+ props: {
+ value?: Record<string, string>;
+ validators?: ((value: unknown) => string | false)[];
+ debounceDelay?: number;
+ } = {},
+) => {
+ const onChange = jest.fn();
+ render(
+ <ControlForm onChange={onChange} value={props.value}>
+ <ControlFormRow>
+ <ControlFormItem
+ name="title"
+ controlType="Input"
+ label="Title"
+ description="Title description"
+ validators={props.validators}
+ debounceDelay={props.debounceDelay}
+ />
+ <ControlFormItem
+ name="note"
+ controlType="Input"
+ label="Note"
+ description="Note description"
+ />
+ </ControlFormRow>
+ </ControlForm>,
+ );
+ return { onChange };
+};
+
+test('seeds each field from the matching key of the form value', () => {
+ renderForm({ value: { title: 'Hello', note: 'World' } });
+ expect(
+ screen
+ .getAllByRole('textbox')
+ .map(input => (input as HTMLInputElement).value),
+ ).toEqual(['Hello', 'World']);
+});
+
+test('propagates an edit to the form merged with the existing value', async () => {
+ const { onChange } = renderForm({ value: { note: 'World' } });
+ await userEvent.type(screen.getAllByRole('textbox')[0], 'Hi');
+
+ await waitFor(() =>
+ expect(onChange).toHaveBeenLastCalledWith({ note: 'World', title: 'Hi' }),
+ );
+});
Review Comment:
**Suggestion:** This test edits only `title`, so it cannot catch debounced edits to two fields overwriting each other from stale form values.
**Assessment:** π `Major` Β· π `Occurrence: Sometimes` Β· π·οΈ `Race condition`
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill) [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=c301b51701dc4cdc96e5eba377d8b531&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=c301b51701dc4cdc96e5eba377d8b531&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<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/ColumnConfigControl/ControlForm/ControlForm.test.tsx
**Line:** 71:78
**Comment:**
*Race Condition: This test edits only `title`, so it cannot catch debounced edits to two fields overwriting each other from stale form values.
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.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F44843&comment_hash=169e37030bf8b436c87b97d8e59241eb3d4203e2840d01557b7d8b8dc2fc248e&reaction=like'>π</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F44843&comment_hash=169e37030bf8b436c87b97d8e59241eb3d4203e2840d01557b7d8b8dc2fc248e&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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] test(explore): cover column config, conditional formatting, and contour controls [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #44843:
URL: https://github.com/apache/superset/pull/44843#issuecomment-5956856324
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/44843?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 82.27%. Comparing base ([`e048389`](https://app.codecov.io/gh/apache/superset/commit/e0483898728a79e5cd517d9048676431163eafe6?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`b3188f6`](https://app.codecov.io/gh/apache/superset/commit/b3188f6151a036dcae083b93b1d09f3e55f0f998?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #44843 +/- ##
==========================================
+ Coverage 82.14% 82.27% +0.12%
==========================================
Files 2995 2995
Lines 184988 184988
Branches 42814 42814
==========================================
+ Hits 151960 152195 +235
+ Misses 30269 30034 -235
Partials 2759 2759
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/44843/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Ξ | |
|---|---|---|
| [javascript](https://app.codecov.io/gh/apache/superset/pull/44843/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `77.98% <ΓΈ> (+0.26%)` | :arrow_up: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/superset/pull/44843?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] test(explore): cover column config, conditional formatting, and contour controls [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #44843:
URL: https://github.com/apache/superset/pull/44843#issuecomment-5959927520
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers an incremental AI Review.
- `/review full` - Manually triggers a full AI Review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] test(explore): cover column config, conditional formatting, and contour controls [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #44843:
URL: https://github.com/apache/superset/pull/44843#discussion_r4150809131
##########
superset-frontend/src/explore/components/controls/ContourControl/ContourOption.test.tsx:
##########
@@ -0,0 +1,111 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { render, screen, userEvent } from 'spec/helpers/testing-library';
+import ContourOption from './ContourOption';
+import { ContourOptionProps, ContourType } from './types';
+
+const color = { r: 10, g: 20, b: 30, a: 100 };
+
+const isoline: ContourType = {
+ lowerThreshold: 4,
+ color,
+ strokeWidth: 3,
+ zIndex: 10,
+};
+
+const isoband: ContourType = {
+ lowerThreshold: 6,
+ upperThreshold: 10,
+ color,
+ zIndex: 20,
+};
+
+const renderOption = (props: Partial<ContourOptionProps> = {}) => {
+ const handlers = {
+ saveContour: jest.fn(),
+ onClose: jest.fn(),
+ onShift: jest.fn(),
+ };
+ render(
+ <ContourOption contour={isoline} index={0} {...handlers} {...props} />,
+ { useDnd: true },
+ );
+ return handlers;
+};
+
+test('labels an isoline with its threshold, color and stroke width', () => {
+ renderOption({ contour: isoline });
+ expect(
+ screen.getByText(
+ 'Threshold: 4, color: rgba(10, 20, 30, 1), stroke width: 3',
+ ),
+ ).toBeInTheDocument();
+});
+
+test('labels an isoband with its threshold range and color but no stroke width', () => {
+ renderOption({ contour: isoband });
+ expect(
+ screen.getByText('Threshold: [6, 10], color: rgba(10, 20, 30, 1)'),
+ ).toBeInTheDocument();
+ expect(screen.queryByText(/stroke width/)).not.toBeInTheDocument();
+});
+
+test('treats a contour with an upper threshold as an isoband', () => {
+ renderOption({ contour: { ...isoline, upperThreshold: 8 } });
+ expect(screen.getByText(/Threshold: \[4, 8\]/)).toBeInTheDocument();
+});
+
+test('treats a contour without an upper threshold as an isoline', () => {
+ renderOption({
+ contour: { ...isoband, upperThreshold: undefined, strokeWidth: 2 },
+ });
+ expect(screen.getByText(/stroke width: 2/)).toBeInTheDocument();
+});
+
+test('calls onClose with the option index when removed', async () => {
+ const { onClose } = renderOption({ index: 2 });
+
+ await userEvent.click(screen.getByTestId('remove-control-button'));
+
+ expect(onClose).toHaveBeenCalledWith(2);
+});
+
+test('opens the contour popover prefilled from the contour when clicked', async () => {
+ renderOption({ contour: isoband });
+
+ await userEvent.click(screen.getByTestId('option-label'));
+
+ expect(await screen.findByRole('tab', { name: 'Isoband' })).toHaveAttribute(
+ 'aria-selected',
+ 'true',
+ );
+ const inputs = screen.getAllByTestId('inline-name') as HTMLInputElement[];
+ expect(inputs.map(input => input.value)).toEqual(['6', '10']);
+});
+
+test('saves an edited contour through saveContour', async () => {
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Misleading test name</b></div>
<div id="fix">
The name says "edited", but the body never modifies a field: it opens the popover prefilled from `isoline` and clicks Save, asserting the original `lowerThreshold: 4` / `strokeWidth: 3` / `color` round-trip through `saveContour`. Rename to describe the actual flow so the suite's names match covered behavior (field-editing is covered in `ContourPopoverControl.test.tsx`).
</div>
</div>
<small><i>Code Review Run #877f71</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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] test(explore): cover column config, conditional formatting, and contour controls [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe merged PR #44843:
URL: https://github.com/apache/superset/pull/44843
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org