codeant-ai-for-open-source[bot] commented on code in PR #38568: URL: https://github.com/apache/superset/pull/38568#discussion_r2940096305
########## superset-frontend/src/filters/components/TimeGrain/TimeGrainFilterPlugin.test.tsx: ########## @@ -0,0 +1,145 @@ +/** + * 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 { screen } from '@testing-library/react'; +import userEvent from '@testing-library/user-event'; +import { render } from 'spec/helpers/testing-library'; +import PluginFilterTimegrain from './TimeGrainFilterPlugin'; +import { PluginFilterTimeGrainProps } from './types'; + +const mockSetDataMask = jest.fn(); +const mockSetFilterActive = jest.fn(); +const mockSetHoveredFilter = jest.fn(); +const mockUnsetHoveredFilter = jest.fn(); +const mockSetFocusedFilter = jest.fn(); +const mockUnsetFocusedFilter = jest.fn(); + +const defaultProps: PluginFilterTimeGrainProps = { + data: [ + { duration: 'P1D', name: 'Day' }, + { duration: 'P1W', name: 'Week' }, + { duration: 'P1M', name: 'Month' }, + { duration: 'P1Y', name: 'Year' }, + ], + formData: { + datasource: '3__table', + viz_type: 'filter_timegrain', + groupby: [], + adhoc_filters: [], + extra_filters: [], + extra_form_data: {}, + granularity_sqla: 'ds', + time_range_endpoints: ['inclusive', 'exclusive'], + url_params: {}, + height: 300, + width: 300, + nativeFilterId: 'filter-1', + defaultValue: null, + inputRef: { current: null }, + }, + filterState: { + value: null, + validateStatus: undefined, + validateMessage: undefined, + }, + height: 300, + width: 300, + setDataMask: mockSetDataMask, + setFilterActive: mockSetFilterActive, + setHoveredFilter: mockSetHoveredFilter, + unsetHoveredFilter: mockUnsetHoveredFilter, + setFocusedFilter: mockSetFocusedFilter, + unsetFocusedFilter: mockUnsetFocusedFilter, + inputRef: { current: null }, +}; + +test('renders all options when time_grains is not set', async () => { + render(<PluginFilterTimegrain {...defaultProps} />); + + // Verify the select component is rendered + const select = screen.getByRole('combobox'); + expect(select).toBeInTheDocument(); + + // Open the dropdown and verify all options are available + await userEvent.click(select); + const options = screen.getAllByRole('option'); Review Comment: **Suggestion:** The assertion reads options synchronously right after opening the Select, but Ant Design renders dropdown options asynchronously in a portal, so this can intermittently fail with "Unable to find role='option'". Wait for options using an async query. [possible bug] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ TimeGrainFilterPlugin Jest test intermittently fails. - ⚠️ CI reliability drops for dashboard filter changes. ``` </details> ```suggestion const options = await screen.findAllByRole('option'); ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Run `TimeGrainFilterPlugin.test.tsx` via Jest; it executes test `renders all options when time_grains is not set` at `superset-frontend/src/filters/components/TimeGrain/TimeGrainFilterPlugin.test.tsx:71`. 2. The test opens the combobox at line 79, triggering `<Select ... options={options} />` in `TimeGrainFilterPlugin.tsx:128-143` (AntD-based select rendered through UI library components). 3. Immediately after click, line 80 calls synchronous `screen.getAllByRole('option')`, which throws if portal options are not mounted yet in that tick. 4. This async timing risk is corroborated by nearby integration tests using `waitFor(() => screen.getAllByRole('option'))` at `TimeGrainPreFilter.integration.test.tsx:80-90`; failure manifests intermittently as missing `role="option"` in CI. ``` </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset-frontend/src/filters/components/TimeGrain/TimeGrainFilterPlugin.test.tsx **Line:** 80:80 **Comment:** *Possible Bug: The assertion reads options synchronously right after opening the Select, but Ant Design renders dropdown options asynchronously in a portal, so this can intermittently fail with "Unable to find role='option'". Wait for options using an async query. 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%2F38568&comment_hash=e8b07acf80a5c64c3ae2e41a37a470e53034930b97f3ed9efc74beee2e4829c6&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38568&comment_hash=e8b07acf80a5c64c3ae2e41a37a470e53034930b97f3ed9efc74beee2e4829c6&reaction=dislike'>👎</a> ########## superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterControls/FilterValue.tsx: ########## @@ -243,7 +276,13 @@ const FilterValue: FC<FilterValueProps> = ({ } else if (response.status === 202) { waitForAsyncData(result as Parameters<typeof waitForAsyncData>[0]) .then((asyncResult: ChartDataResponseResult[]) => { - setState(asyncResult); + setState( + applyTimeGrainAllowlist( + filterType, + allowedTimeGrains, + asyncResult, + ), + ); handleFilterLoadFinish(); }) Review Comment: **Suggestion:** In the async `202` success branch, a previous error is never cleared, so the component can stay stuck rendering the error UI even after a successful refresh. Clear the error state on successful async completion. [possible bug] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ Time Grain filter remains stuck in error view. - ⚠️ Successful async retries do not recover UI. - ⚠️ Users cannot interact with recovered filter data. ``` </details> ```suggestion .then((asyncResult: ChartDataResponseResult[]) => { setState( applyTimeGrainAllowlist( filterType, allowedTimeGrains, asyncResult, ), ); setError(undefined); handleFilterLoadFinish(); }) ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Render native filters through `FilterBar` (`.../FilterBar/index.tsx:67-104`) -> `FilterControl` -> `FilterValue` (`.../FilterControl.tsx:133-146`), then trigger filter data loading (`.../FilterValue.tsx:264-317`). 2. Cause one failed fetch (e.g., transient backend/network failure); catch handlers set component error state (`.../FilterValue.tsx:289-291` or `312-314`). 3. With `error` set, component short-circuits to error UI (`if (error) return ...`) at `.../FilterValue.tsx:422-436`, hiding the normal filter chart. 4. Trigger another refresh that succeeds via async `202` path (`.../FilterValue.tsx:276-287`); this success branch updates `state` but does not clear `error`, unlike non-async success which does `setError(undefined)` (`.../FilterValue.tsx:307`). 5. Because `error` remains truthy, `FilterValue` continues returning the error panel (`.../FilterValue.tsx:422`), even after successful async data arrives. ``` </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterControls/FilterValue.tsx **Line:** 278:287 **Comment:** *Possible Bug: In the async `202` success branch, a previous error is never cleared, so the component can stay stuck rendering the error UI even after a successful refresh. Clear the error state on successful async completion. 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%2F38568&comment_hash=3bc883be1203b0beb88c0c4b1a66a85a79697f22b2c82a4b3f36df6324be59b2&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38568&comment_hash=3bc883be1203b0beb88c0c4b1a66a85a79697f22b2c82a4b3f36df6324be59b2&reaction=dislike'>👎</a> ########## superset-frontend/src/filters/components/TimeGrain/TimeGrainFilterPlugin.test.tsx: ########## @@ -0,0 +1,145 @@ +/** + * 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 { screen } from '@testing-library/react'; +import userEvent from '@testing-library/user-event'; +import { render } from 'spec/helpers/testing-library'; +import PluginFilterTimegrain from './TimeGrainFilterPlugin'; +import { PluginFilterTimeGrainProps } from './types'; + +const mockSetDataMask = jest.fn(); +const mockSetFilterActive = jest.fn(); +const mockSetHoveredFilter = jest.fn(); +const mockUnsetHoveredFilter = jest.fn(); +const mockSetFocusedFilter = jest.fn(); +const mockUnsetFocusedFilter = jest.fn(); + +const defaultProps: PluginFilterTimeGrainProps = { + data: [ + { duration: 'P1D', name: 'Day' }, + { duration: 'P1W', name: 'Week' }, + { duration: 'P1M', name: 'Month' }, + { duration: 'P1Y', name: 'Year' }, + ], + formData: { + datasource: '3__table', + viz_type: 'filter_timegrain', + groupby: [], + adhoc_filters: [], + extra_filters: [], + extra_form_data: {}, + granularity_sqla: 'ds', + time_range_endpoints: ['inclusive', 'exclusive'], + url_params: {}, + height: 300, + width: 300, + nativeFilterId: 'filter-1', + defaultValue: null, + inputRef: { current: null }, + }, + filterState: { + value: null, + validateStatus: undefined, + validateMessage: undefined, + }, + height: 300, + width: 300, + setDataMask: mockSetDataMask, + setFilterActive: mockSetFilterActive, + setHoveredFilter: mockSetHoveredFilter, + unsetHoveredFilter: mockUnsetHoveredFilter, + setFocusedFilter: mockSetFocusedFilter, + unsetFocusedFilter: mockUnsetFocusedFilter, + inputRef: { current: null }, +}; + +test('renders all options when time_grains is not set', async () => { + render(<PluginFilterTimegrain {...defaultProps} />); + + // Verify the select component is rendered + const select = screen.getByRole('combobox'); + expect(select).toBeInTheDocument(); + + // Open the dropdown and verify all options are available + await userEvent.click(select); + const options = screen.getAllByRole('option'); + expect(options.length).toBe(4); + expect(options[0]).toHaveTextContent('Day'); + expect(options[1]).toHaveTextContent('Week'); + expect(options[2]).toHaveTextContent('Month'); + expect(options[3]).toHaveTextContent('Year'); +}); + +test('filters options based on time_grains allowlist', async () => { + const propsWithAllowlist = { + ...defaultProps, + formData: { + ...defaultProps.formData, + time_grains: ['P1D', 'P1W'], + }, + }; + + render(<PluginFilterTimegrain {...propsWithAllowlist} />); + + const select = screen.getByRole('combobox'); + await userEvent.click(select); + + // Only Day and Week should be available + const options = screen.getAllByRole('option'); Review Comment: **Suggestion:** This test has the same timing race: options are queried immediately after click, but dropdown rendering is async, making the test non-deterministic in CI. Use `findAllByRole` to wait until options are present. [possible bug] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ Allowlist behavior test becomes nondeterministic. - ⚠️ Frontend CI can fail despite correct code. ``` </details> ```suggestion const options = await screen.findAllByRole('option'); ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Execute Jest test `filters options based on time_grains allowlist` at `TimeGrainFilterPlugin.test.tsx:88`. 2. Test clicks combobox at line 100; component path enters `PluginFilterTimegrain` select rendering in `TimeGrainFilterPlugin.tsx:128-143`. 3. Line 103 synchronously requests all `option` roles before dropdown portal stabilizes, causing occasional throw when options are not yet attached. 4. Similar async handling patterns exist in codebase (`DatabaseSelector.test.tsx:268` uses `await screen.findAllByRole('option')`), confirming synchronous read is brittle here. ``` </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset-frontend/src/filters/components/TimeGrain/TimeGrainFilterPlugin.test.tsx **Line:** 103:103 **Comment:** *Possible Bug: This test has the same timing race: options are queried immediately after click, but dropdown rendering is async, making the test non-deterministic in CI. Use `findAllByRole` to wait until options are present. 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%2F38568&comment_hash=54696f70a22799b4cfd41302abd0424afa3b33c53d47273a9762c6b5b4ad4ea6&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38568&comment_hash=54696f70a22799b4cfd41302abd0424afa3b33c53d47273a9762c6b5b4ad4ea6&reaction=dislike'>👎</a> ########## superset-frontend/src/filters/components/TimeGrain/TimeGrainFilterPlugin.test.tsx: ########## @@ -0,0 +1,145 @@ +/** + * 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 { screen } from '@testing-library/react'; +import userEvent from '@testing-library/user-event'; +import { render } from 'spec/helpers/testing-library'; +import PluginFilterTimegrain from './TimeGrainFilterPlugin'; +import { PluginFilterTimeGrainProps } from './types'; + +const mockSetDataMask = jest.fn(); +const mockSetFilterActive = jest.fn(); +const mockSetHoveredFilter = jest.fn(); +const mockUnsetHoveredFilter = jest.fn(); +const mockSetFocusedFilter = jest.fn(); +const mockUnsetFocusedFilter = jest.fn(); + +const defaultProps: PluginFilterTimeGrainProps = { + data: [ + { duration: 'P1D', name: 'Day' }, + { duration: 'P1W', name: 'Week' }, + { duration: 'P1M', name: 'Month' }, + { duration: 'P1Y', name: 'Year' }, + ], + formData: { + datasource: '3__table', + viz_type: 'filter_timegrain', + groupby: [], + adhoc_filters: [], + extra_filters: [], + extra_form_data: {}, + granularity_sqla: 'ds', + time_range_endpoints: ['inclusive', 'exclusive'], + url_params: {}, + height: 300, + width: 300, + nativeFilterId: 'filter-1', + defaultValue: null, + inputRef: { current: null }, + }, + filterState: { + value: null, + validateStatus: undefined, + validateMessage: undefined, + }, + height: 300, + width: 300, + setDataMask: mockSetDataMask, + setFilterActive: mockSetFilterActive, + setHoveredFilter: mockSetHoveredFilter, + unsetHoveredFilter: mockUnsetHoveredFilter, + setFocusedFilter: mockSetFocusedFilter, + unsetFocusedFilter: mockUnsetFocusedFilter, + inputRef: { current: null }, +}; + +test('renders all options when time_grains is not set', async () => { + render(<PluginFilterTimegrain {...defaultProps} />); + + // Verify the select component is rendered + const select = screen.getByRole('combobox'); + expect(select).toBeInTheDocument(); + + // Open the dropdown and verify all options are available + await userEvent.click(select); + const options = screen.getAllByRole('option'); + expect(options.length).toBe(4); + expect(options[0]).toHaveTextContent('Day'); + expect(options[1]).toHaveTextContent('Week'); + expect(options[2]).toHaveTextContent('Month'); + expect(options[3]).toHaveTextContent('Year'); +}); + +test('filters options based on time_grains allowlist', async () => { + const propsWithAllowlist = { + ...defaultProps, + formData: { + ...defaultProps.formData, + time_grains: ['P1D', 'P1W'], + }, + }; + + render(<PluginFilterTimegrain {...propsWithAllowlist} />); + + const select = screen.getByRole('combobox'); + await userEvent.click(select); + + // Only Day and Week should be available + const options = screen.getAllByRole('option'); + expect(options.length).toBe(2); + expect(options[0]).toHaveTextContent('Day'); + expect(options[1]).toHaveTextContent('Week'); +}); + +test('shows all options when time_grains is empty array', async () => { + const propsWithEmptyAllowlist = { + ...defaultProps, + formData: { + ...defaultProps.formData, + time_grains: [], + }, + }; + + render(<PluginFilterTimegrain {...propsWithEmptyAllowlist} />); + + const select = screen.getByRole('combobox'); + await userEvent.click(select); + + // All 4 options should be available + const options = screen.getAllByRole('option'); + expect(options.length).toBe(4); +}); + +test('shows all options when time_grains is undefined', async () => { + const propsWithUndefined = { + ...defaultProps, + formData: { + ...defaultProps.formData, + time_grains: undefined, + }, + }; + + render(<PluginFilterTimegrain {...propsWithUndefined} />); + + const select = screen.getByRole('combobox'); + await userEvent.click(select); + + // All 4 options should be available + const options = screen.getAllByRole('option'); Review Comment: **Suggestion:** This test also performs a synchronous fetch of dropdown options immediately after user interaction, which can race against async rendering and make tests flaky. Await the options query instead. [possible bug] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ Undefined-config compatibility test is flaky. - ⚠️ CI reruns waste developer review time. ``` </details> ```suggestion const options = await screen.findAllByRole('option'); ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Execute test `shows all options when time_grains is undefined` at `TimeGrainFilterPlugin.test.tsx:128`. 2. Click action at line 140 opens the Select dropdown rendered by `PluginFilterTimegrain` (`TimeGrainFilterPlugin.tsx:128-143`). 3. Immediate synchronous query at line 143 can race with portal option mounting, producing `Unable to find role="option"`. 4. This is a realistic async-test race, not hypothetical: same repository uses async option lookup (`findAllByRole`) in `DatabaseSelector.test.tsx:268` for equivalent combobox behavior. ``` </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset-frontend/src/filters/components/TimeGrain/TimeGrainFilterPlugin.test.tsx **Line:** 143:143 **Comment:** *Possible Bug: This test also performs a synchronous fetch of dropdown options immediately after user interaction, which can race against async rendering and make tests flaky. Await the options query instead. 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%2F38568&comment_hash=c85b36a56b46c6912347ba512be7ce608c3c2b8ae60baddc518829c7fdb897cd&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38568&comment_hash=c85b36a56b46c6912347ba512be7ce608c3c2b8ae60baddc518829c7fdb897cd&reaction=dislike'>👎</a> ########## superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/transformers/timeGrains.test.ts: ########## @@ -0,0 +1,99 @@ +/** + * 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 { NativeFilterType, NativeFilterScope } from '@superset-ui/core'; +import { getInitialDataMask } from 'src/dataMask/reducer'; + +const mockScope: NativeFilterScope = { + rootPath: ['ROOT_ID'], + excluded: [], +}; + +const mockDataMask = getInitialDataMask(); + +test('time_grains field is persisted when subset is selected', () => { + // This tests the contract that time_grains are properly saved + // when a subset of grains are selected + const timeGrains = ['P1D', 'P1W']; + const expectedOutput = { + time_grains: timeGrains, + }; + + // Verify the structure matches what the transformer should produce + expect(expectedOutput.time_grains).toEqual(timeGrains); + expect(expectedOutput.time_grains?.length).toBe(2); Review Comment: **Suggestion:** This test is self-referential (`expectedOutput` is built from `timeGrains` and then compared back to `timeGrains`), so it will pass even if persistence behavior regresses. Validate an actual persistence boundary (serialize/deserialize) so the test can catch real failures. [logic error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ CI may miss transformer persistence regressions. - ❌ Time grain allowlist feature can silently break. - ⚠️ Dashboard filter save flow lacks meaningful unit guard. ``` </details> ```suggestion const timeGrains = ['P1D', 'P1W']; const serialized = JSON.stringify({ time_grains: timeGrains }); const parsed = JSON.parse(serialized); expect(parsed.time_grains).toEqual(timeGrains); expect(parsed.time_grains).toHaveLength(2); ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Open save path used by dashboard filter configuration: `transformFilterForSave()` is invoked from `superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/hooks/useModalSaveLogic.ts:310`, and persistence of `time_grains` happens in `superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/transformers/filterTransformer.ts:130`. 2. Verify runtime depends on persisted `time_grains`: `FilterValue` reads it at `superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterControls/FilterValue.tsx:77-80` and applies filtering via `applyTimeGrainAllowlist()` at `:22-45`, then uses it during result handling at `:280-305`. 3. Run this test file (`superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/transformers/timeGrains.test.ts`) and observe the first test (`:29-40`) passes without calling `transformFilterForSave` or any save/runtime function; it only compares a local object to the same local array. 4. Introduce a realistic regression in transformer behavior (e.g., stop propagating `formInputs.time_grains` at `filterTransformer.ts:130`) and rerun this test; it still passes, proving the current assertion cannot catch the real persistence regression. ``` </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/transformers/timeGrains.test.ts **Line:** 32:39 **Comment:** *Logic Error: This test is self-referential (`expectedOutput` is built from `timeGrains` and then compared back to `timeGrains`), so it will pass even if persistence behavior regresses. Validate an actual persistence boundary (serialize/deserialize) so the test can catch real failures. 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%2F38568&comment_hash=152ee3ae42ade379c47f314a6372a56c42f2a70810845cd29c0b7613d31d7960&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38568&comment_hash=152ee3ae42ade379c47f314a6372a56c42f2a70810845cd29c0b7613d31d7960&reaction=dislike'>👎</a> ########## superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/TimeGrainPreFilter.test.tsx: ########## @@ -0,0 +1,132 @@ +/** + * 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 { Form, Select } from '@superset-ui/core/components'; +import { CollapsibleControl } from './CollapsibleControl'; +import { getTimeGrainOptions } from './utils'; + +/** + * Tests for the Time Grain Pre-filter feature (CollapsibleControl + Select). + * + * These tests verify: + * - Options are rendered in database order (no client-side sorting) + * - A saved subset of time_grains is loaded and shown as selected values + * - The CollapsibleControl checkbox shows/hides the Select + * - Toggling the checkbox off clears the underlying selection + */ + +const TIME_GRAIN_TUPLES: [string, string][] = [ + ['PT1S', 'Second'], + ['PT1M', 'Minute'], + ['PT1H', 'Hour'], + ['P1D', 'Day'], + ['P1W', 'Week'], +]; + +const renderPreFilter = (props: { + savedGrains?: string[]; + initialChecked?: boolean; + onChangeMock?: jest.Mock; +}) => { + const { + savedGrains, + initialChecked = false, + onChangeMock = jest.fn(), + } = props; + const options = getTimeGrainOptions(TIME_GRAIN_TUPLES); + return render( + <Form> + <CollapsibleControl + title="Pre-filter available values" + initialValue={initialChecked} + onChange={onChangeMock} + > + <Form.Item name="time_grains" initialValue={savedGrains}> + <Select + mode="multiple" + ariaLabel="Time grain options" + options={options} + sortComparator={() => 0} + /> + </Form.Item> + </CollapsibleControl> + </Form>, + ); +}; + +test('time grain options preserve database order (no sorting)', () => { + const options = getTimeGrainOptions(TIME_GRAIN_TUPLES); + const labels = options.map(option => option.label); Review Comment: **Suggestion:** The current "preserve database order" test only validates `getTimeGrainOptions`, not the rendered `Select` behavior, so it will still pass if the UI starts reordering options. Render the control, open the dropdown, and assert the visible option order to catch real regressions in the pre-filter UI. [logic error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ CI misses dropdown ordering regressions in filter UI. - ⚠️ Dashboard authors see unexpected time-grain ordering. - ⚠️ Existing test duplicates `utils.test.ts` transformation coverage. ``` </details> ```suggestion test('time grain options preserve database order (no sorting)', async () => { renderPreFilter({ initialChecked: true }); const combobox = screen.getByRole('combobox', { name: /Time grain options/i, }); await userEvent.click(combobox); const labels = screen .getAllByRole('option') .map(option => option.textContent); ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Open the real feature path in `superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FiltersConfigForm.tsx:1248-1264`, where the Time Grain allow-list `Select` is rendered with `sortComparator={() => 0}`. 2. Confirm UI ordering is determined at render-time in `superset-frontend/packages/superset-ui-core/src/components/Select/Select.tsx:118-203` (default sorting exists, and comparator is applied in dropdown option sorting flow). 3. Run current test `TimeGrainPreFilter.test.tsx:73-79`; it only calls `getTimeGrainOptions` (`utils.ts:33-46`) and never renders/open the dropdown. 4. Introduce a realistic UI regression (for example, remove `sortComparator={() => 0}` at `FiltersConfigForm.tsx:1263`); dropdown order changes in product UI, but this test still passes because it validates transformation output, not rendered option order. ``` </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/TimeGrainPreFilter.test.tsx **Line:** 73:75 **Comment:** *Logic Error: The current "preserve database order" test only validates `getTimeGrainOptions`, not the rendered `Select` behavior, so it will still pass if the UI starts reordering options. Render the control, open the dropdown, and assert the visible option order to catch real regressions in the pre-filter UI. 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%2F38568&comment_hash=88fbe859dc8b82b3e50ad3543c31ab7a2f79c2a1b685a1907eec8a4a9457134c&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38568&comment_hash=88fbe859dc8b82b3e50ad3543c31ab7a2f79c2a1b685a1907eec8a4a9457134c&reaction=dislike'>👎</a> ########## superset-frontend/src/filters/components/TimeGrain/TimeGrainFilterPlugin.test.tsx: ########## @@ -0,0 +1,145 @@ +/** + * 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 { screen } from '@testing-library/react'; +import userEvent from '@testing-library/user-event'; +import { render } from 'spec/helpers/testing-library'; +import PluginFilterTimegrain from './TimeGrainFilterPlugin'; +import { PluginFilterTimeGrainProps } from './types'; + +const mockSetDataMask = jest.fn(); +const mockSetFilterActive = jest.fn(); +const mockSetHoveredFilter = jest.fn(); +const mockUnsetHoveredFilter = jest.fn(); +const mockSetFocusedFilter = jest.fn(); +const mockUnsetFocusedFilter = jest.fn(); + +const defaultProps: PluginFilterTimeGrainProps = { + data: [ + { duration: 'P1D', name: 'Day' }, + { duration: 'P1W', name: 'Week' }, + { duration: 'P1M', name: 'Month' }, + { duration: 'P1Y', name: 'Year' }, + ], + formData: { + datasource: '3__table', + viz_type: 'filter_timegrain', + groupby: [], + adhoc_filters: [], + extra_filters: [], + extra_form_data: {}, + granularity_sqla: 'ds', + time_range_endpoints: ['inclusive', 'exclusive'], + url_params: {}, + height: 300, + width: 300, + nativeFilterId: 'filter-1', + defaultValue: null, + inputRef: { current: null }, + }, + filterState: { + value: null, + validateStatus: undefined, + validateMessage: undefined, + }, + height: 300, + width: 300, + setDataMask: mockSetDataMask, + setFilterActive: mockSetFilterActive, + setHoveredFilter: mockSetHoveredFilter, + unsetHoveredFilter: mockUnsetHoveredFilter, + setFocusedFilter: mockSetFocusedFilter, + unsetFocusedFilter: mockUnsetFocusedFilter, + inputRef: { current: null }, +}; + +test('renders all options when time_grains is not set', async () => { + render(<PluginFilterTimegrain {...defaultProps} />); + + // Verify the select component is rendered + const select = screen.getByRole('combobox'); + expect(select).toBeInTheDocument(); + + // Open the dropdown and verify all options are available + await userEvent.click(select); + const options = screen.getAllByRole('option'); + expect(options.length).toBe(4); + expect(options[0]).toHaveTextContent('Day'); + expect(options[1]).toHaveTextContent('Week'); + expect(options[2]).toHaveTextContent('Month'); + expect(options[3]).toHaveTextContent('Year'); +}); + +test('filters options based on time_grains allowlist', async () => { + const propsWithAllowlist = { + ...defaultProps, + formData: { + ...defaultProps.formData, + time_grains: ['P1D', 'P1W'], + }, + }; + + render(<PluginFilterTimegrain {...propsWithAllowlist} />); + + const select = screen.getByRole('combobox'); + await userEvent.click(select); + + // Only Day and Week should be available + const options = screen.getAllByRole('option'); + expect(options.length).toBe(2); + expect(options[0]).toHaveTextContent('Day'); + expect(options[1]).toHaveTextContent('Week'); +}); + +test('shows all options when time_grains is empty array', async () => { + const propsWithEmptyAllowlist = { + ...defaultProps, + formData: { + ...defaultProps.formData, + time_grains: [], + }, + }; + + render(<PluginFilterTimegrain {...propsWithEmptyAllowlist} />); + + const select = screen.getByRole('combobox'); + await userEvent.click(select); + + // All 4 options should be available + const options = screen.getAllByRole('option'); Review Comment: **Suggestion:** The options list is read synchronously after opening the combobox, which can fail sporadically before portal options mount. Switching to an awaited role query removes this flakiness. [possible bug] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ Empty-allowlist regression test can randomly fail. - ⚠️ Noise increases in PR validation pipeline. ``` </details> ```suggestion const options = await screen.findAllByRole('option'); ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Run test `shows all options when time_grains is empty array` in `TimeGrainFilterPlugin.test.tsx:109`. 2. At line 121, user-event clicks the combobox linked to `<Select>` in `TimeGrainFilterPlugin.tsx:128`. 3. At line 124, `screen.getAllByRole('option')` executes synchronously; if dropdown options render one tick later, query throws. 4. Re-running this file multiple times under CI load can expose intermittent failures; integration tests in same folder already wrap option reads in `waitFor` (`TimeGrainPreFilter.integration.test.tsx:134-140`). ``` </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset-frontend/src/filters/components/TimeGrain/TimeGrainFilterPlugin.test.tsx **Line:** 124:124 **Comment:** *Possible Bug: The options list is read synchronously after opening the combobox, which can fail sporadically before portal options mount. Switching to an awaited role query removes this flakiness. 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%2F38568&comment_hash=6cc36f75a30607c2aab458dc8623f9cb3195030e50adaa78597253cbad24f9cf&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38568&comment_hash=6cc36f75a30607c2aab458dc8623f9cb3195030e50adaa78597253cbad24f9cf&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]
