codeant-ai-for-open-source[bot] commented on code in PR #37479:
URL: https://github.com/apache/superset/pull/37479#discussion_r2741945343
##########
superset-frontend/src/filters/components/Select/SelectFilterPlugin.test.tsx:
##########
@@ -680,6 +683,115 @@ describe('SelectFilterPlugin', () => {
expect(options[1]).toHaveTextContent('alpha');
expect(options[2]).toHaveTextContent('beta');
});
+
+ test('allowSelectAll is disabled when searchAllOptions is enabled and data
is truncated at 1000 rows', async () => {
+ // Create mock data with 1000 rows to simulate truncation
+ const largeData = Array.from({ length: 1000 }, (_, i) => ({
+ gender: `value_${i}`,
+ }));
+
+ getWrapper({
+ searchAllOptions: true,
+ multiSelect: true,
+ queriesData: [
+ {
+ rowcount: 1000,
+ colnames: ['gender'],
+ coltypes: [1],
+ data: largeData,
+ applied_filters: [{ column: 'gender' }],
+ rejected_filters: [],
+ },
+ ],
+ });
+
+ // Open the dropdown
+ const filterSelect = screen.getAllByRole('combobox')[0];
+ userEvent.click(filterSelect);
+
+ // Wait for dropdown to be visible
+ await waitFor(() => {
+ expect(screen.getByRole('combobox')).toBeInTheDocument();
+ });
+
+ // Verify "Select All" button is NOT present (disabled due to truncation)
+ expect(screen.queryByText(/Select all/i)).not.toBeInTheDocument();
+ });
+
+ test('allowSelectAll is enabled when searchAllOptions is enabled but data is
less than 1000 rows', async () => {
+ getWrapper({
+ searchAllOptions: true,
+ multiSelect: true,
+ });
+
+ // Open the dropdown
+ const filterSelect = screen.getAllByRole('combobox')[0];
+ userEvent.click(filterSelect);
+
+ // Wait for dropdown to be visible
+ await waitFor(() => {
+ expect(screen.getByRole('combobox')).toBeInTheDocument();
+ });
+
+ // With only 3 rows (default test data), "Select all" button should be
present
+ expect(await screen.findByText('Select all (3)')).toBeInTheDocument();
+ });
+
+ test('allowSelectAll is enabled for multi-select without searchAllOptions',
async () => {
+ getWrapper({
+ searchAllOptions: false,
+ multiSelect: true,
+ });
+
+ // Open the dropdown
+ const filterSelect = screen.getAllByRole('combobox')[0];
+ userEvent.click(filterSelect);
+
+ // Wait for dropdown to be visible
+ await waitFor(() => {
+ expect(screen.getByRole('combobox')).toBeInTheDocument();
+ });
+
+ // "Select all" button should be present for static filters
+ expect(await screen.findByText('Select all (3)')).toBeInTheDocument();
Review Comment:
**Suggestion:** Another test asserts the exact "Select all (3)" text which
is fragile for the same reason; use a regex-based presence check instead so the
test doesn't fail if the numeric count formatting changes. [possible bug]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Unit test may produce intermittent failures.
- ⚠️ CI stability for SelectFilterPlugin impacted.
- ⚠️ Test asserts presentation formatting tightly.
```
</details>
```suggestion
expect(await screen.findByText(/Select all/i)).toBeInTheDocument();
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Run only the test suite for this file:
- Command: yarn test
packages/superset-frontend/src/filters/components/Select/SelectFilterPlugin.test.tsx
2. Inspect the second occurrence of the exact-string assertion at
`superset-frontend/src/filters/components/Select/SelectFilterPlugin.test.tsx:756`.
3. Alter the test's input so the rendered Select component displays a
different numeric
count:
- For example, pass a `queriesData` prop with a different number of rows
to
`getWrapper()` call in the scope of that test.
4. Re-run the test. The single-line exact match at 756 will fail if the
numeric count
differs (causing a flaky failure).
Explanation:
- This is the same brittleness as the prior suggestion but for a separate
assertion
occurrence in the file.
```
</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/Select/SelectFilterPlugin.test.tsx
**Line:** 756:756
**Comment:**
*Possible Bug: Another test asserts the exact "Select all (3)" text
which is fragile for the same reason; use a regex-based presence check instead
so the test doesn't fail if the numeric count formatting changes.
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>
##########
superset-frontend/src/filters/components/Select/SelectFilterPlugin.test.tsx:
##########
@@ -680,6 +683,115 @@ describe('SelectFilterPlugin', () => {
expect(options[1]).toHaveTextContent('alpha');
expect(options[2]).toHaveTextContent('beta');
});
+
+ test('allowSelectAll is disabled when searchAllOptions is enabled and data
is truncated at 1000 rows', async () => {
+ // Create mock data with 1000 rows to simulate truncation
+ const largeData = Array.from({ length: 1000 }, (_, i) => ({
+ gender: `value_${i}`,
+ }));
+
+ getWrapper({
+ searchAllOptions: true,
+ multiSelect: true,
+ queriesData: [
+ {
+ rowcount: 1000,
+ colnames: ['gender'],
+ coltypes: [1],
+ data: largeData,
+ applied_filters: [{ column: 'gender' }],
+ rejected_filters: [],
+ },
+ ],
+ });
+
+ // Open the dropdown
+ const filterSelect = screen.getAllByRole('combobox')[0];
+ userEvent.click(filterSelect);
+
+ // Wait for dropdown to be visible
+ await waitFor(() => {
+ expect(screen.getByRole('combobox')).toBeInTheDocument();
+ });
+
+ // Verify "Select All" button is NOT present (disabled due to truncation)
+ expect(screen.queryByText(/Select all/i)).not.toBeInTheDocument();
+ });
+
+ test('allowSelectAll is enabled when searchAllOptions is enabled but data is
less than 1000 rows', async () => {
+ getWrapper({
+ searchAllOptions: true,
+ multiSelect: true,
+ });
+
+ // Open the dropdown
+ const filterSelect = screen.getAllByRole('combobox')[0];
+ userEvent.click(filterSelect);
+
+ // Wait for dropdown to be visible
+ await waitFor(() => {
+ expect(screen.getByRole('combobox')).toBeInTheDocument();
+ });
+
+ // With only 3 rows (default test data), "Select all" button should be
present
+ expect(await screen.findByText('Select all (3)')).toBeInTheDocument();
Review Comment:
**Suggestion:** Test is brittle by asserting the exact "Select all (3)"
string; the count in the button can vary depending on test data (e.g. null
values or different queriesData), causing false negatives. Use a looser match
so the test only verifies the presence of the Select All control. [possible bug]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Unit test may fail spuriously on data count changes.
- ⚠️ CI flaky failure for SelectFilterPlugin tests.
- ⚠️ Affects test validation for Select all visibility.
```
</details>
```suggestion
expect(await screen.findByText(/Select all/i)).toBeInTheDocument();
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Run the test file containing the assertion:
- Command: yarn test
packages/superset-frontend/src/filters/components/Select/SelectFilterPlugin.test.tsx
- This file path is the test under review (`SelectFilterPlugin.test.tsx`).
2. Locate the exact assertion at
`superset-frontend/src/filters/components/Select/SelectFilterPlugin.test.tsx:737`.
- The test under `test('allowSelectAll is enabled when searchAllOptions
is enabled but
data is less than 1000 rows', ...)`
contains the exact-string check at line 737.
3. Cause the numeric count displayed by the component to differ from 3 by
changing the
input data the test uses:
- The test uses default `selectMultipleProps.queriesData` declared
earlier in this
file; modify the default test data length
(see `selectMultipleProps` block around the top of the file) to a
different length
(e.g., remove or add one item).
4. Re-run the single test (same command). The assertion at line 737 will
fail with a
"Unable to find an element with the text: Select all (3)" error
if the rendered button shows a different numeric count or formats it
differently.
Explanation:
- This is a unit-test brittleness issue: asserting the exact "Select all
(3)" text ties
the test to a specific count formatting.
- Using a regex-based presence check avoids false negatives when test data
lengths or
formatting change.
```
</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/Select/SelectFilterPlugin.test.tsx
**Line:** 737:737
**Comment:**
*Possible Bug: Test is brittle by asserting the exact "Select all (3)"
string; the count in the button can vary depending on test data (e.g. null
values or different queriesData), causing false negatives. Use a looser match
so the test only verifies the presence of the Select All control.
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>
##########
superset-frontend/src/filters/components/Select/SelectFilterPlugin.test.tsx:
##########
@@ -680,6 +683,115 @@ describe('SelectFilterPlugin', () => {
expect(options[1]).toHaveTextContent('alpha');
expect(options[2]).toHaveTextContent('beta');
});
+
+ test('allowSelectAll is disabled when searchAllOptions is enabled and data
is truncated at 1000 rows', async () => {
+ // Create mock data with 1000 rows to simulate truncation
+ const largeData = Array.from({ length: 1000 }, (_, i) => ({
+ gender: `value_${i}`,
+ }));
+
+ getWrapper({
+ searchAllOptions: true,
+ multiSelect: true,
+ queriesData: [
+ {
+ rowcount: 1000,
Review Comment:
**Suggestion:** The truncation test aims to simulate "truncated" data, but
it uses exactly the configured rowLimit (1000); whether that is considered
truncated depends on implementation (>= vs >). To guarantee truncation, make
the test data and reported rowcount exceed the limit (e.g. 1001). [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Truncation-related test may misrepresent component behaviour.
- ⚠️ Dynamic-search Select All visibility tests affected.
- ⚠️ CI failures for truncation edge-case tests.
```
</details>
```suggestion
// Create mock data with 1001 rows to simulate truncation beyond the
rowLimit
const largeData = Array.from({ length: 1001 }, (_, i) => ({
gender: `value_${i}`,
}));
getWrapper({
searchAllOptions: true,
multiSelect: true,
queriesData: [
{
rowcount: 1001,
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Run the truncation test in this file:
- Command: yarn test
packages/superset-frontend/src/filters/components/Select/SelectFilterPlugin.test.tsx
-t
"allowSelectAll is disabled when searchAllOptions is enabled and data is
truncated at
1000 rows"
2. Observe the test setup at `SelectFilterPlugin.test.tsx:688-706` where
`rowLimit` (from
`selectMultipleProps.formData.rowLimit`) is 1000 and the test sets
`rowcount: 1000`.
3. Depending on the implementation, the component may treat `rowcount ===
rowLimit` as
truncated or not (>= vs > comparison). If the component treats only
`rowcount > rowLimit`
as truncated, the test's intention (simulate truncation with 1000) will not
hold and the
test will fail.
4. To reproduce the off-by-one failure concretely, change the test
`rowcount` and `data`
to 1001 (as in the proposed improved code) and re-run the test; the behavior
will then
unambiguously represent "truncated" in implementations that use strict
greater-than logic.
Explanation:
- This step-by-step references the actual test setup lines (688-706). The
assertion's
meaning depends on the component's truncation predicate.
```
</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/Select/SelectFilterPlugin.test.tsx
**Line:** 688:698
**Comment:**
*Logic Error: The truncation test aims to simulate "truncated" data,
but it uses exactly the configured rowLimit (1000); whether that is considered
truncated depends on implementation (>= vs >). To guarantee truncation, make
the test data and reported rowcount exceed the limit (e.g. 1001).
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>
--
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]
