codeant-ai-for-open-source[bot] commented on code in PR #41338:
URL: https://github.com/apache/superset/pull/41338#discussion_r3459666208
##########
superset-frontend/src/features/alerts/components/NotificationMethod.test.tsx:
##########
@@ -34,6 +34,82 @@ import {
import { NotificationMethod, mapSlackValues } from './NotificationMethod';
import { NotificationMethodOption, NotificationSetting } from '../types';
+type MockAsyncSelectOption = {
+ label: string;
+ value: string;
+};
+
+type MockAsyncSelectProps = {
+ ariaLabel?: string;
+ 'data-test'?: string;
+ name?: string;
+ onChange?: (value: MockAsyncSelectOption[]) => void;
+ options?: (
+ filterValue: string,
+ page: number,
+ pageSize: number,
+ ) => Promise<{ data: MockAsyncSelectOption[]; totalCount: number }>;
+ placeholder?: string;
+ value?: MockAsyncSelectOption[];
+};
+
+jest.mock('@superset-ui/core/components', () => {
+ const actual = jest.requireActual('@superset-ui/core/components');
+ const React = jest.requireActual('react');
+
+ return {
+ ...actual,
+ AsyncSelect: ({
+ ariaLabel,
+ 'data-test': dataTest,
+ name,
+ onChange,
+ options,
+ placeholder,
+ value = [],
+ }: MockAsyncSelectProps) => {
+ const [loadedOptions, setLoadedOptions] = React.useState<
+ MockAsyncSelectOption[]
+ >([]);
+
+ return (
+ <>
+ <input
+ aria-label={ariaLabel ?? name}
+ data-test={dataTest}
+ placeholder={placeholder}
+ value={value.map(option => option.value).join(',')}
+ onChange={({ target: { value: inputValue } }) =>
+ onChange?.(
+ inputValue
+ .split(',')
+ .map(option => option.trim())
+ .filter(Boolean)
+ .map(option => ({ label: option, value: option })),
Review Comment:
**Suggestion:** The AsyncSelect test mock parses manual input only by
commas, but production behavior supports both comma and semicolon separators
for recipients. This mismatch means tests can pass while missing regressions
for semicolon-separated input, or fail to represent real user behavior. Update
the mock parser to match production separators so the tests validate the real
contract. [api mismatch]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Email recipient manual-entry tests can't validate semicolon separators.
- ⚠️ AsyncSelect manual parsing behavior diverges from production
implementation.
- ⚠️ Potential regressions in semicolon handling go undetected.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. In production code `NotificationMethod.tsx:224-234`, email recipients are
parsed by
`recipientStringToOptions`, which uses `emailRecipientSeparators = /[,;]/`
and
`value.split(emailRecipientSeparators)` to support both comma- and
semicolon-separated
recipient strings.
2. The email AsyncSelect for To/CC/BCC in `NotificationMethod.tsx:128-145`
and
`NotificationMethod.tsx:193-247` uses this parsing when converting selected
values back to
the underlying string via `emailRecipientOptionsToString`, which assumes
recipients were
split on both "," and ";".
3. In the test file `NotificationMethod.test.tsx:70-89`, the AsyncSelect
test double calls
`onChange?.(inputValue.split(',').map(...))`, so a manual input like
`"[email protected];
[email protected]"` is treated as a single option `"[email protected];
[email protected]"`
instead of two separate recipients.
4. This means any tests in `NotificationMethod.test.tsx` that simulate
manual typing of
semicolon-separated recipients into the mocked AsyncSelect will exercise
different parsing
behavior than the real component, allowing regressions in semicolon handling
in
`NotificationMethod.tsx` to pass tests undetected and making test
expectations diverge
from actual user behavior.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=a014a750b4134450a76c065c6d15100f&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=a014a750b4134450a76c065c6d15100f&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/src/features/alerts/components/NotificationMethod.test.tsx
**Line:** 85:88
**Comment:**
*Api Mismatch: The AsyncSelect test mock parses manual input only by
commas, but production behavior supports both comma and semicolon separators
for recipients. This mismatch means tests can pass while missing regressions
for semicolon-separated input, or fail to represent real user behavior. Update
the mock parser to match production separators so the tests validate the real
contract.
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%2F41338&comment_hash=429364915ebfe04310d849161e9ba3535cd4176c0ba900322cefee84a2f37b58&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41338&comment_hash=429364915ebfe04310d849161e9ba3535cd4176c0ba900322cefee84a2f37b58&reaction=dislike'>👎</a>
##########
superset-frontend/src/features/alerts/components/NotificationMethod.test.tsx:
##########
@@ -160,6 +236,47 @@ describe('NotificationMethod', () => {
});
});
+ test('should load email recipient options from report owners', async () => {
+ jest.spyOn(SupersetClient, 'get').mockResolvedValue({
+ json: {
+ count: 1,
+ result: [
+ {
+ text: 'Test User',
+ value: 1,
+ extra: {
+ email: '[email protected]',
+ },
+ },
+ ],
+ },
+ } as JsonResponse);
Review Comment:
**Suggestion:** This spy uses `mockResolvedValue` without being scoped to a
single call, so the mocked `SupersetClient.get` implementation can leak into
later tests in this file. Because `beforeEach` only calls `clearAllMocks`
(which does not restore implementations), subsequent tests may run against the
wrong network behavior and become order-dependent. Use a one-shot mock
(`mockResolvedValueOnce`) or restore mocks after each test to prevent
cross-test contamination. [code quality]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ NotificationMethod tests share SupersetClient.get spy across cases.
- ⚠️ Future tests may become order-dependent around SupersetClient.get.
- ⚠️ Harder reason about SupersetClient.get behavior per test.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. In `NotificationMethod.test.tsx:141-147`, the `beforeEach` only calls
`jest.clearAllMocks()` and `cleanup()`, but does not call
`jest.restoreAllMocks()` or
`jest.resetAllMocks()`, so spy implementations are not restored between
tests.
2. In the test `"should load email recipient options from report owners"` at
`NotificationMethod.test.tsx:239-253`, `jest.spyOn(SupersetClient,
'get').mockResolvedValue(...)` replaces `SupersetClient.get` with a spy whose
implementation always returns the provided `JsonResponse`, and this spy is
never restored
in that test.
3. Because `jest.clearAllMocks()` only clears call history and does not
restore original
implementations, after this test finishes `SupersetClient.get` remains
mocked for
subsequent tests, affecting any code path that calls `SupersetClient.get`,
such as
`fetchEmailRecipientOptions` in `NotificationMethod.tsx:256-260` and
`fetchSlackChannels`
in `NotificationMethod.tsx:99-118`.
4. Later tests in the same file that rely on `SupersetClient.get` but do not
explicitly
set up their own spy (or that assume a fresh default implementation) will
instead see the
leaked mock from the owners test, making their behavior and potential
failures depend on
test execution order and previous mocks rather than isolated setup per test.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=62ff336093d5414282b41b8cde68bca2&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=62ff336093d5414282b41b8cde68bca2&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/src/features/alerts/components/NotificationMethod.test.tsx
**Line:** 240:253
**Comment:**
*Code Quality: This spy uses `mockResolvedValue` without being scoped
to a single call, so the mocked `SupersetClient.get` implementation can leak
into later tests in this file. Because `beforeEach` only calls `clearAllMocks`
(which does not restore implementations), subsequent tests may run against the
wrong network behavior and become order-dependent. Use a one-shot mock
(`mockResolvedValueOnce`) or restore mocks after each test to prevent
cross-test contamination.
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%2F41338&comment_hash=71814db92c2090b6d654e233b9aa2b5aaceeb5f401cc1e04385c3df61b1fad16&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41338&comment_hash=71814db92c2090b6d654e233b9aa2b5aaceeb5f401cc1e04385c3df61b1fad16&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]
