bito-code-review[bot] commented on code in PR #37479:
URL: https://github.com/apache/superset/pull/37479#discussion_r2737919698
##########
superset-frontend/src/filters/components/Select/SelectFilterPlugin.test.tsx:
##########
@@ -680,6 +683,90 @@ 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', () => {
+ // Create mock data with 1000 rows to simulate truncation
+ const largeData = Array.from({ length: 1000 }, (_, i) => ({
+ gender: `value_${i}`,
+ }));
+
+ const { container } = getWrapper({
+ searchAllOptions: true,
+ multiSelect: true,
+ queriesData: [
+ {
+ rowcount: 1000,
+ colnames: ['gender'],
+ coltypes: [1],
+ data: largeData,
+ applied_filters: [{ column: 'gender' }],
+ rejected_filters: [],
+ },
+ ],
+ });
+ // Verify the Select component is rendered with multiSelect mode
+ const selectElement = container.querySelector('.ant-select-multiple');
+ expect(selectElement).toBeInTheDocument();
+ // Note: allowSelectAll is disabled because with searchAllOptions + 1000
rows,
+ // data is likely truncated, so "Select All" would be misleading
+ });
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Insufficient test assertions</b></div>
<div id="fix">
The new tests for allowSelectAll behavior only check component rendering but
do not verify the actual allowSelectAll logic (e.g., whether 'Select All'
appears in the UI). This violates the guideline to assert business logic
directly rather than just rendering. Consider adding DOM checks for the 'Select
All' option.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
````suggestion
expect(selectElement).toBeInTheDocument();
// Verify that "Select All" is not present
expect(container.textContent).not.toMatch(/Select All/);
// Note: allowSelectAll is disabled because with searchAllOptions + 1000
rows,
// data is likely truncated, so "Select All" would be misleading
});
````
</div>
</details>
</div>
<small><i>Code Review Run #098c25</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset-frontend/src/filters/components/Select/SelectFilterPlugin.test.tsx:
##########
@@ -680,6 +683,90 @@ 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', () => {
+ // Create mock data with 1000 rows to simulate truncation
+ const largeData = Array.from({ length: 1000 }, (_, i) => ({
+ gender: `value_${i}`,
+ }));
+
+ const { container } = getWrapper({
+ searchAllOptions: true,
+ multiSelect: true,
+ queriesData: [
+ {
+ rowcount: 1000,
+ colnames: ['gender'],
+ coltypes: [1],
+ data: largeData,
+ applied_filters: [{ column: 'gender' }],
+ rejected_filters: [],
+ },
+ ],
+ });
+ // Verify the Select component is rendered with multiSelect mode
+ const selectElement = container.querySelector('.ant-select-multiple');
+ expect(selectElement).toBeInTheDocument();
+ // Note: allowSelectAll is disabled because with searchAllOptions + 1000
rows,
+ // data is likely truncated, so "Select All" would be misleading
+ });
+
+ test('allowSelectAll is enabled when searchAllOptions is enabled but data is
less than 1000 rows', () => {
+ const { container } = getWrapper({
+ searchAllOptions: true,
+ multiSelect: true,
+ });
+ // With only 3 rows (default test data), Select All should work fine
+ const selectElement = container.querySelector('.ant-select-multiple');
+ expect(selectElement).toBeInTheDocument();
+ });
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Insufficient test assertions</b></div>
<div id="fix">
Similar to the above test, this one also lacks verification of
allowSelectAll being enabled.
</div>
</div>
<small><i>Code Review Run #098c25</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
