codeant-ai-for-open-source[bot] commented on code in PR #37621:
URL: https://github.com/apache/superset/pull/37621#discussion_r2759393457
##########
superset-frontend/src/components/Datasource/components/DatasourceEditor/tests/DatasourceEditorCurrency.test.tsx:
##########
@@ -196,10 +219,22 @@ test('currency code column dropdown shows only string
columns', async () => {
expect(currencyCodeOption).toBeDefined();
});
- // Verify NUMERIC column is NOT available
+ // Verify CALCULATED column is available despite null type_generic
const options = document.querySelectorAll('.ant-select-item-option');
+ const derivedCurrencyOption = Array.from(options).find(o =>
+ o.textContent?.includes('derived_currency'),
+ );
+ expect(derivedCurrencyOption).toBeDefined();
+
+ // Verify NUMERIC physical column is NOT available
const amountOption = Array.from(options).find(o =>
o.textContent?.includes('amount'),
);
Review Comment:
**Suggestion:** The check for the numeric physical column uses
`includes('amount')`, which can also match the `total_amount` option text, so
the test may incorrectly detect the calculated `total_amount` column as the
physical `amount` column and either fail or pass for the wrong reason. To avoid
this ambiguity, the match should explicitly exclude `total_amount` so that only
the intended `amount` option is considered. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Test for currency column becomes flaky or misleading.
- ⚠️ CI could show intermittent failures for dropdown tests.
- ⚠️ Developer time wasted diagnosing false negatives.
```
</details>
```suggestion
const amountOption = Array.from(options).find(o => {
const text = o.textContent ?? '';
return text.includes('amount') && !text.includes('total_amount');
});
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Run the test suite including the file at
superset-frontend/src/components/Datasource/components/DatasourceEditor/tests/DatasourceEditorCurrency.test.tsx.
2. The test navigates to the Columns tab and opens the "Currency code
column" dropdown
(see test block starting around line 141 and the dropdown interaction at
lines ~206-211).
3. After opening the dropdown, the test collects options via
document.querySelectorAll('.ant-select-item-option') and stores them in
`options` (lines
~214-221 and 222-227).
4. The existing code at lines 230-233 uses Array.from(options).find(o =>
o.textContent?.includes('amount')) which matches any option whose visible
text contains
the substring "amount". Because the dropdown contains both 'amount'
(physical column) and
'total_amount' (calculated column), the find may return the 'total_amount'
option instead
of the intended 'amount' option.
5. If the test runner or DOM ordering changes such that 'total_amount'
appears before
'amount' in the option list, the current assertion
(expect(amountOption).toBeUndefined())
may pass or fail for the wrong reason, making the test flaky or misleading.
6. The improved code (proposed) disambiguates the match by ensuring the
matched option
includes "amount" but not "total_amount", eliminating the substring
collision and making
the expectation target the physical 'amount' option explicitly.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/src/components/Datasource/components/DatasourceEditor/tests/DatasourceEditorCurrency.test.tsx
**Line:** 230:232
**Comment:**
*Logic Error: The check for the numeric physical column uses
`includes('amount')`, which can also match the `total_amount` option text, so
the test may incorrectly detect the calculated `total_amount` column as the
physical `amount` column and either fail or pass for the wrong reason. To avoid
this ambiguity, the match should explicitly exclude `total_amount` so that only
the intended `amount` option is considered.
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]
