bito-code-review[bot] commented on code in PR #43805:
URL: https://github.com/apache/superset/pull/43805#discussion_r3993743759
##########
superset-frontend/src/dashboard/components/menu/DownloadMenuItems/DownloadMenuItems.test.tsx:
##########
@@ -368,3 +533,258 @@ test('Enabled screenshot items should not show tooltip
icon', () => {
mockIsFeatureEnabled.mockReset();
});
+
+// ---------------------------------------------------------------------------
+// Delivery follows the requester identity, not iframe presence: a guest or
+// anonymous session (no userId) has no email channel, so the toast must not
+// promise one, and the image export (webdriver-rendered, guests cannot open
+// Explore) is hidden.
+// ---------------------------------------------------------------------------
+
+const guestState = { user: {} };
+
+test('guest session: export toast promises auto-download, not an email', async
() => {
+ mockSupersetClient.post.mockResolvedValue({
+ json: { job_id: 'abc' },
+ } as never);
+
+ render(<MenuWrapper />, { useRedux: true, initialState: guestState });
+
+ await userEvent.click(screen.getByText('Export Data to Excel'));
+
+ await waitFor(() =>
+ expect(mockAddInfoToast).toHaveBeenCalledWith(
+ 'Your export is being generated. Please, do not leave the page.',
+ { noDuplicate: true },
+ ),
+ );
+});
+
+test('guest session: Export Images to Excel is hidden even with the webdriver
enabled', () => {
+ enableWebDriverScreenshot();
+
+ render(<MenuWrapper />, { useRedux: true, initialState: guestState });
+
+ expect(screen.getByText('Export Data to Excel')).toBeInTheDocument();
+ expect(screen.queryByText('Export Images to Excel')).not.toBeInTheDocument();
+});
+
+test('logged-in user without an email gets the delivery-neutral toast', async
() => {
+ mockSupersetClient.post.mockResolvedValue({
+ json: { job_id: 'abc' },
+ } as never);
+
+ render(<MenuWrapper />, {
+ useRedux: true,
+ initialState: { user: { userId: 1 } },
+ });
+
+ await userEvent.click(screen.getByText('Export Data to Excel'));
+
+ await waitFor(() =>
+ expect(mockAddInfoToast).toHaveBeenCalledWith(
+ 'Your export is being generated. Please, do not leave the page.',
+ { noDuplicate: true },
+ ),
+ );
+});
+
+test('a "running" status restarts the wait window, so queue delay is not
counted', async () => {
+ jest.useFakeTimers();
+ mockSupersetClient.post.mockResolvedValue({
+ json: { job_id: 'abc' },
+ } as never);
+ // Worker picks the job up on the first poll; still running just past the
+ // original 12 minute deadline; done on the poll after that.
+ mockSupersetClient.get
+ .mockResolvedValueOnce({ json: { status: 'running' } } as never)
+ .mockResolvedValueOnce({ json: { status: 'running' } } as never)
+ .mockResolvedValueOnce({
+ json: {
+ status: 'ready',
+ download_url: '/api/v1/dashboard/export_xlsx/download/abc/',
+ },
+ } as never);
+
+ render(<MenuWrapper />, { useRedux: true, initialState: loggedInState });
+
+ await userEvent.click(screen.getByText('Export Data to Excel'));
+ await waitFor(() => expect(mockSupersetClient.post).toHaveBeenCalled());
+
+ await act(async () => {
+ jest.advanceTimersByTime(3000);
+ });
+ await waitFor(() => expect(mockSupersetClient.get).toHaveBeenCalledTimes(1));
+
+ // t ~= 12m01s: past the enqueue-based deadline, within the restarted one
+ // (running was observed at t=3s). Without the restart this poll would give
+ // up with a danger toast instead of continuing.
+ await act(async () => {
+ jest.advanceTimersByTime(12 * 60 * 1000 - 2000);
+ });
+ await waitFor(() => expect(mockSupersetClient.get).toHaveBeenCalledTimes(2));
+ expect(mockAddDangerToast).not.toHaveBeenCalled();
+
+ await act(async () => {
+ jest.advanceTimersByTime(3000);
+ });
+ await waitFor(() => {
+
expect(lastIframeSrc()).toBe('/api/v1/dashboard/export_xlsx/download/abc/');
+ expect(mockAddSuccessToast).toHaveBeenCalledWith(
+ 'Your export is ready and downloading.',
+ );
+ });
+});
+
+test('a transient poll failure keeps polling and still downloads', async () =>
{
+ jest.useFakeTimers();
+ mockSupersetClient.post.mockResolvedValue({
+ json: { job_id: 'abc' },
+ } as never);
+ mockSupersetClient.get
+ .mockRejectedValueOnce(new Error('network blip'))
+ .mockResolvedValueOnce({
+ json: {
+ status: 'ready',
+ download_url: '/api/v1/dashboard/export_xlsx/download/abc/',
+ },
+ } as never);
+
+ render(<MenuWrapper />, { useRedux: true, initialState: loggedInState });
+
+ await userEvent.click(screen.getByText('Export Data to Excel'));
+ await waitFor(() => expect(mockSupersetClient.post).toHaveBeenCalled());
+
+ await act(async () => {
+ jest.advanceTimersByTime(3000);
+ });
+ await waitFor(() => expect(mockSupersetClient.get).toHaveBeenCalledTimes(1));
+ expect(mockAddDangerToast).not.toHaveBeenCalled();
+
+ await act(async () => {
+ jest.advanceTimersByTime(3000);
+ });
+ await waitFor(() => {
+
expect(lastIframeSrc()).toBe('/api/v1/dashboard/export_xlsx/download/abc/');
+ expect(mockAddSuccessToast).toHaveBeenCalledWith(
+ 'Your export is ready and downloading.',
+ );
+ });
+});
+
+test('poll failures past the deadline give up with an error toast', async ()
=> {
+ jest.useFakeTimers();
+ mockSupersetClient.post.mockResolvedValue({
+ json: { job_id: 'abc' },
+ } as never);
+ mockSupersetClient.get.mockRejectedValue(new Error('server down'));
+
+ render(<MenuWrapper />, { useRedux: true, initialState: loggedInState });
+
+ await userEvent.click(screen.getByText('Export Data to Excel'));
+ await waitFor(() => expect(mockSupersetClient.post).toHaveBeenCalled());
+
+ await act(async () => {
+ jest.advanceTimersByTime(3000);
+ });
+ await waitFor(() => expect(mockSupersetClient.get).toHaveBeenCalledTimes(1));
+
+ // Jump past the 12 minute deadline; the next failing poll must give up.
+ await act(async () => {
+ jest.advanceTimersByTime(13 * 60 * 1000);
+ });
+ await waitFor(() => {
+ expect(mockAddDangerToast).toHaveBeenCalledWith(
+ 'Sorry, something went wrong. Try again later.',
+ );
+ });
+ expect(mockAddSuccessToast).not.toHaveBeenCalledWith(
+ 'Your export is ready and downloading.',
+ );
+});
+
+test('a "ready" status with no download_url is an error, not a fake success',
async () => {
+ jest.useFakeTimers();
+ mockSupersetClient.post.mockResolvedValue({
+ json: { job_id: 'abc' },
+ } as never);
+ mockSupersetClient.get.mockResolvedValue({
+ json: { status: 'ready' },
+ } as never);
+
+ render(<MenuWrapper />, { useRedux: true, initialState: loggedInState });
+
+ await userEvent.click(screen.getByText('Export Data to Excel'));
+ await waitFor(() => expect(mockSupersetClient.post).toHaveBeenCalled());
+
+ await act(async () => {
+ jest.advanceTimersByTime(3000);
+ });
+
+ await waitFor(() => {
+ expect(mockAddDangerToast).toHaveBeenCalledWith(
+ 'Sorry, something went wrong. Try again later.',
+ );
+ });
+ expect(mockAddSuccessToast).not.toHaveBeenCalled();
+ expect(mockAddSuccessToast).not.toHaveBeenCalledWith(
+ 'Your export is ready and downloading.',
+ );
+});
+
+test('unmounting stops the polling loop', async () => {
+ jest.useFakeTimers();
+ mockSupersetClient.post.mockResolvedValue({
+ json: { job_id: 'abc' },
+ } as never);
+ mockSupersetClient.get.mockResolvedValue({
+ json: { status: 'pending' },
+ } as never);
+
+ const { unmount } = render(<MenuWrapper />, {
+ useRedux: true,
+ initialState: loggedInState,
+ });
+
+ await userEvent.click(screen.getByText('Export Data to Excel'));
+ await waitFor(() => expect(mockSupersetClient.post).toHaveBeenCalled());
+
+ await act(async () => {
+ jest.advanceTimersByTime(3000);
+ });
+ await waitFor(() => expect(mockSupersetClient.get).toHaveBeenCalledTimes(1));
+
+ unmount();
+
+ await act(async () => {
+ jest.advanceTimersByTime(30000);
+ });
+ expect(mockSupersetClient.get).toHaveBeenCalledTimes(1);
+});
+
+test('the pending toast is announced once, not re-emitted on every poll',
async () => {
+ jest.useFakeTimers();
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Fake timers hang click</b></div>
<div id="fix">
`userEvent.click()` in v12.8.3 uses `setTimeout` internally, so under plain
fake timers the click's internal delay never fires and `await
userEvent.click()` hangs, timing out the test. Use `jest.useFakeTimers({
advanceTimers: true })` as done in `Header.test.tsx:732`.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
````suggestion
jest.useFakeTimers({ advanceTimers: true });
````
</div>
</details>
</div>
<small><i>Code Review Run #084afd</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/dashboard/components/menu/DownloadMenuItems/DownloadMenuItems.test.tsx:
##########
@@ -368,3 +533,258 @@ test('Enabled screenshot items should not show tooltip
icon', () => {
mockIsFeatureEnabled.mockReset();
});
+
+// ---------------------------------------------------------------------------
+// Delivery follows the requester identity, not iframe presence: a guest or
+// anonymous session (no userId) has no email channel, so the toast must not
+// promise one, and the image export (webdriver-rendered, guests cannot open
+// Explore) is hidden.
+// ---------------------------------------------------------------------------
+
+const guestState = { user: {} };
+
+test('guest session: export toast promises auto-download, not an email', async
() => {
+ mockSupersetClient.post.mockResolvedValue({
+ json: { job_id: 'abc' },
+ } as never);
+
+ render(<MenuWrapper />, { useRedux: true, initialState: guestState });
+
+ await userEvent.click(screen.getByText('Export Data to Excel'));
+
+ await waitFor(() =>
+ expect(mockAddInfoToast).toHaveBeenCalledWith(
+ 'Your export is being generated. Please, do not leave the page.',
+ { noDuplicate: true },
+ ),
+ );
+});
+
+test('guest session: Export Images to Excel is hidden even with the webdriver
enabled', () => {
+ enableWebDriverScreenshot();
+
+ render(<MenuWrapper />, { useRedux: true, initialState: guestState });
+
+ expect(screen.getByText('Export Data to Excel')).toBeInTheDocument();
+ expect(screen.queryByText('Export Images to Excel')).not.toBeInTheDocument();
+});
+
+test('logged-in user without an email gets the delivery-neutral toast', async
() => {
+ mockSupersetClient.post.mockResolvedValue({
+ json: { job_id: 'abc' },
+ } as never);
+
+ render(<MenuWrapper />, {
+ useRedux: true,
+ initialState: { user: { userId: 1 } },
+ });
+
+ await userEvent.click(screen.getByText('Export Data to Excel'));
+
+ await waitFor(() =>
+ expect(mockAddInfoToast).toHaveBeenCalledWith(
+ 'Your export is being generated. Please, do not leave the page.',
+ { noDuplicate: true },
+ ),
+ );
+});
+
+test('a "running" status restarts the wait window, so queue delay is not
counted', async () => {
+ jest.useFakeTimers();
+ mockSupersetClient.post.mockResolvedValue({
+ json: { job_id: 'abc' },
+ } as never);
+ // Worker picks the job up on the first poll; still running just past the
+ // original 12 minute deadline; done on the poll after that.
+ mockSupersetClient.get
+ .mockResolvedValueOnce({ json: { status: 'running' } } as never)
+ .mockResolvedValueOnce({ json: { status: 'running' } } as never)
+ .mockResolvedValueOnce({
+ json: {
+ status: 'ready',
+ download_url: '/api/v1/dashboard/export_xlsx/download/abc/',
+ },
+ } as never);
+
+ render(<MenuWrapper />, { useRedux: true, initialState: loggedInState });
+
+ await userEvent.click(screen.getByText('Export Data to Excel'));
+ await waitFor(() => expect(mockSupersetClient.post).toHaveBeenCalled());
+
+ await act(async () => {
+ jest.advanceTimersByTime(3000);
+ });
+ await waitFor(() => expect(mockSupersetClient.get).toHaveBeenCalledTimes(1));
+
+ // t ~= 12m01s: past the enqueue-based deadline, within the restarted one
+ // (running was observed at t=3s). Without the restart this poll would give
+ // up with a danger toast instead of continuing.
+ await act(async () => {
+ jest.advanceTimersByTime(12 * 60 * 1000 - 2000);
+ });
+ await waitFor(() => expect(mockSupersetClient.get).toHaveBeenCalledTimes(2));
+ expect(mockAddDangerToast).not.toHaveBeenCalled();
+
+ await act(async () => {
+ jest.advanceTimersByTime(3000);
+ });
+ await waitFor(() => {
+
expect(lastIframeSrc()).toBe('/api/v1/dashboard/export_xlsx/download/abc/');
+ expect(mockAddSuccessToast).toHaveBeenCalledWith(
+ 'Your export is ready and downloading.',
+ );
+ });
+});
+
+test('a transient poll failure keeps polling and still downloads', async () =>
{
+ jest.useFakeTimers();
+ mockSupersetClient.post.mockResolvedValue({
+ json: { job_id: 'abc' },
+ } as never);
+ mockSupersetClient.get
+ .mockRejectedValueOnce(new Error('network blip'))
+ .mockResolvedValueOnce({
+ json: {
+ status: 'ready',
+ download_url: '/api/v1/dashboard/export_xlsx/download/abc/',
+ },
+ } as never);
+
+ render(<MenuWrapper />, { useRedux: true, initialState: loggedInState });
+
+ await userEvent.click(screen.getByText('Export Data to Excel'));
+ await waitFor(() => expect(mockSupersetClient.post).toHaveBeenCalled());
+
+ await act(async () => {
+ jest.advanceTimersByTime(3000);
+ });
+ await waitFor(() => expect(mockSupersetClient.get).toHaveBeenCalledTimes(1));
+ expect(mockAddDangerToast).not.toHaveBeenCalled();
+
+ await act(async () => {
+ jest.advanceTimersByTime(3000);
+ });
+ await waitFor(() => {
+
expect(lastIframeSrc()).toBe('/api/v1/dashboard/export_xlsx/download/abc/');
+ expect(mockAddSuccessToast).toHaveBeenCalledWith(
+ 'Your export is ready and downloading.',
+ );
+ });
+});
+
+test('poll failures past the deadline give up with an error toast', async ()
=> {
+ jest.useFakeTimers();
+ mockSupersetClient.post.mockResolvedValue({
+ json: { job_id: 'abc' },
+ } as never);
+ mockSupersetClient.get.mockRejectedValue(new Error('server down'));
+
+ render(<MenuWrapper />, { useRedux: true, initialState: loggedInState });
+
+ await userEvent.click(screen.getByText('Export Data to Excel'));
+ await waitFor(() => expect(mockSupersetClient.post).toHaveBeenCalled());
+
+ await act(async () => {
+ jest.advanceTimersByTime(3000);
+ });
+ await waitFor(() => expect(mockSupersetClient.get).toHaveBeenCalledTimes(1));
+
+ // Jump past the 12 minute deadline; the next failing poll must give up.
+ await act(async () => {
+ jest.advanceTimersByTime(13 * 60 * 1000);
+ });
+ await waitFor(() => {
+ expect(mockAddDangerToast).toHaveBeenCalledWith(
+ 'Sorry, something went wrong. Try again later.',
+ );
+ });
+ expect(mockAddSuccessToast).not.toHaveBeenCalledWith(
+ 'Your export is ready and downloading.',
+ );
+});
+
+test('a "ready" status with no download_url is an error, not a fake success',
async () => {
+ jest.useFakeTimers();
+ mockSupersetClient.post.mockResolvedValue({
+ json: { job_id: 'abc' },
+ } as never);
+ mockSupersetClient.get.mockResolvedValue({
+ json: { status: 'ready' },
+ } as never);
+
+ render(<MenuWrapper />, { useRedux: true, initialState: loggedInState });
+
+ await userEvent.click(screen.getByText('Export Data to Excel'));
+ await waitFor(() => expect(mockSupersetClient.post).toHaveBeenCalled());
+
+ await act(async () => {
+ jest.advanceTimersByTime(3000);
+ });
+
+ await waitFor(() => {
+ expect(mockAddDangerToast).toHaveBeenCalledWith(
+ 'Sorry, something went wrong. Try again later.',
+ );
+ });
+ expect(mockAddSuccessToast).not.toHaveBeenCalled();
+ expect(mockAddSuccessToast).not.toHaveBeenCalledWith(
+ 'Your export is ready and downloading.',
+ );
+});
+
+test('unmounting stops the polling loop', async () => {
+ jest.useFakeTimers();
+ mockSupersetClient.post.mockResolvedValue({
+ json: { job_id: 'abc' },
+ } as never);
+ mockSupersetClient.get.mockResolvedValue({
+ json: { status: 'pending' },
+ } as never);
+
+ const { unmount } = render(<MenuWrapper />, {
+ useRedux: true,
+ initialState: loggedInState,
+ });
+
+ await userEvent.click(screen.getByText('Export Data to Excel'));
+ await waitFor(() => expect(mockSupersetClient.post).toHaveBeenCalled());
+
+ await act(async () => {
+ jest.advanceTimersByTime(3000);
+ });
+ await waitFor(() => expect(mockSupersetClient.get).toHaveBeenCalledTimes(1));
+
+ unmount();
+
+ await act(async () => {
+ jest.advanceTimersByTime(30000);
+ });
+ expect(mockSupersetClient.get).toHaveBeenCalledTimes(1);
+});
+
+test('the pending toast is announced once, not re-emitted on every poll',
async () => {
+ jest.useFakeTimers();
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Fake timers hang click</b></div>
<div id="fix">
Same hang risk as the sibling test: `await userEvent.click()` at line 266
uses user-event v12's internal setTimeout, which plain fake timers never
advance. Use `jest.useFakeTimers({ advanceTimers: true })`.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
````suggestion
jest.useFakeTimers({ advanceTimers: true });
````
</div>
</details>
</div>
<small><i>Code Review Run #084afd</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/dashboard/components/menu/DownloadMenuItems/DownloadMenuItems.test.tsx:
##########
@@ -368,3 +533,258 @@ test('Enabled screenshot items should not show tooltip
icon', () => {
mockIsFeatureEnabled.mockReset();
});
+
+// ---------------------------------------------------------------------------
+// Delivery follows the requester identity, not iframe presence: a guest or
+// anonymous session (no userId) has no email channel, so the toast must not
+// promise one, and the image export (webdriver-rendered, guests cannot open
+// Explore) is hidden.
+// ---------------------------------------------------------------------------
+
+const guestState = { user: {} };
+
+test('guest session: export toast promises auto-download, not an email', async
() => {
+ mockSupersetClient.post.mockResolvedValue({
+ json: { job_id: 'abc' },
+ } as never);
+
+ render(<MenuWrapper />, { useRedux: true, initialState: guestState });
+
+ await userEvent.click(screen.getByText('Export Data to Excel'));
+
+ await waitFor(() =>
+ expect(mockAddInfoToast).toHaveBeenCalledWith(
+ 'Your export is being generated. Please, do not leave the page.',
+ { noDuplicate: true },
+ ),
+ );
+});
+
+test('guest session: Export Images to Excel is hidden even with the webdriver
enabled', () => {
+ enableWebDriverScreenshot();
+
+ render(<MenuWrapper />, { useRedux: true, initialState: guestState });
+
+ expect(screen.getByText('Export Data to Excel')).toBeInTheDocument();
+ expect(screen.queryByText('Export Images to Excel')).not.toBeInTheDocument();
+});
+
+test('logged-in user without an email gets the delivery-neutral toast', async
() => {
+ mockSupersetClient.post.mockResolvedValue({
+ json: { job_id: 'abc' },
+ } as never);
+
+ render(<MenuWrapper />, {
+ useRedux: true,
+ initialState: { user: { userId: 1 } },
+ });
+
+ await userEvent.click(screen.getByText('Export Data to Excel'));
+
+ await waitFor(() =>
+ expect(mockAddInfoToast).toHaveBeenCalledWith(
+ 'Your export is being generated. Please, do not leave the page.',
+ { noDuplicate: true },
+ ),
+ );
+});
+
+test('a "running" status restarts the wait window, so queue delay is not
counted', async () => {
+ jest.useFakeTimers();
+ mockSupersetClient.post.mockResolvedValue({
+ json: { job_id: 'abc' },
+ } as never);
+ // Worker picks the job up on the first poll; still running just past the
+ // original 12 minute deadline; done on the poll after that.
+ mockSupersetClient.get
+ .mockResolvedValueOnce({ json: { status: 'running' } } as never)
+ .mockResolvedValueOnce({ json: { status: 'running' } } as never)
+ .mockResolvedValueOnce({
+ json: {
+ status: 'ready',
+ download_url: '/api/v1/dashboard/export_xlsx/download/abc/',
+ },
+ } as never);
+
+ render(<MenuWrapper />, { useRedux: true, initialState: loggedInState });
+
+ await userEvent.click(screen.getByText('Export Data to Excel'));
+ await waitFor(() => expect(mockSupersetClient.post).toHaveBeenCalled());
+
+ await act(async () => {
+ jest.advanceTimersByTime(3000);
+ });
+ await waitFor(() => expect(mockSupersetClient.get).toHaveBeenCalledTimes(1));
+
+ // t ~= 12m01s: past the enqueue-based deadline, within the restarted one
+ // (running was observed at t=3s). Without the restart this poll would give
+ // up with a danger toast instead of continuing.
+ await act(async () => {
+ jest.advanceTimersByTime(12 * 60 * 1000 - 2000);
+ });
+ await waitFor(() => expect(mockSupersetClient.get).toHaveBeenCalledTimes(2));
+ expect(mockAddDangerToast).not.toHaveBeenCalled();
+
+ await act(async () => {
+ jest.advanceTimersByTime(3000);
+ });
+ await waitFor(() => {
+
expect(lastIframeSrc()).toBe('/api/v1/dashboard/export_xlsx/download/abc/');
+ expect(mockAddSuccessToast).toHaveBeenCalledWith(
+ 'Your export is ready and downloading.',
+ );
+ });
+});
+
+test('a transient poll failure keeps polling and still downloads', async () =>
{
+ jest.useFakeTimers();
+ mockSupersetClient.post.mockResolvedValue({
+ json: { job_id: 'abc' },
+ } as never);
+ mockSupersetClient.get
+ .mockRejectedValueOnce(new Error('network blip'))
+ .mockResolvedValueOnce({
+ json: {
+ status: 'ready',
+ download_url: '/api/v1/dashboard/export_xlsx/download/abc/',
+ },
+ } as never);
+
+ render(<MenuWrapper />, { useRedux: true, initialState: loggedInState });
+
+ await userEvent.click(screen.getByText('Export Data to Excel'));
+ await waitFor(() => expect(mockSupersetClient.post).toHaveBeenCalled());
+
+ await act(async () => {
+ jest.advanceTimersByTime(3000);
+ });
+ await waitFor(() => expect(mockSupersetClient.get).toHaveBeenCalledTimes(1));
+ expect(mockAddDangerToast).not.toHaveBeenCalled();
+
+ await act(async () => {
+ jest.advanceTimersByTime(3000);
+ });
+ await waitFor(() => {
+
expect(lastIframeSrc()).toBe('/api/v1/dashboard/export_xlsx/download/abc/');
+ expect(mockAddSuccessToast).toHaveBeenCalledWith(
+ 'Your export is ready and downloading.',
+ );
+ });
+});
+
+test('poll failures past the deadline give up with an error toast', async ()
=> {
+ jest.useFakeTimers();
+ mockSupersetClient.post.mockResolvedValue({
+ json: { job_id: 'abc' },
+ } as never);
+ mockSupersetClient.get.mockRejectedValue(new Error('server down'));
+
+ render(<MenuWrapper />, { useRedux: true, initialState: loggedInState });
+
+ await userEvent.click(screen.getByText('Export Data to Excel'));
+ await waitFor(() => expect(mockSupersetClient.post).toHaveBeenCalled());
+
+ await act(async () => {
+ jest.advanceTimersByTime(3000);
+ });
+ await waitFor(() => expect(mockSupersetClient.get).toHaveBeenCalledTimes(1));
+
+ // Jump past the 12 minute deadline; the next failing poll must give up.
+ await act(async () => {
+ jest.advanceTimersByTime(13 * 60 * 1000);
+ });
+ await waitFor(() => {
+ expect(mockAddDangerToast).toHaveBeenCalledWith(
+ 'Sorry, something went wrong. Try again later.',
+ );
+ });
+ expect(mockAddSuccessToast).not.toHaveBeenCalledWith(
+ 'Your export is ready and downloading.',
+ );
+});
+
+test('a "ready" status with no download_url is an error, not a fake success',
async () => {
+ jest.useFakeTimers();
+ mockSupersetClient.post.mockResolvedValue({
+ json: { job_id: 'abc' },
+ } as never);
+ mockSupersetClient.get.mockResolvedValue({
+ json: { status: 'ready' },
+ } as never);
+
+ render(<MenuWrapper />, { useRedux: true, initialState: loggedInState });
+
+ await userEvent.click(screen.getByText('Export Data to Excel'));
+ await waitFor(() => expect(mockSupersetClient.post).toHaveBeenCalled());
+
+ await act(async () => {
+ jest.advanceTimersByTime(3000);
+ });
+
+ await waitFor(() => {
+ expect(mockAddDangerToast).toHaveBeenCalledWith(
+ 'Sorry, something went wrong. Try again later.',
+ );
+ });
+ expect(mockAddSuccessToast).not.toHaveBeenCalled();
+ expect(mockAddSuccessToast).not.toHaveBeenCalledWith(
+ 'Your export is ready and downloading.',
+ );
+});
+
+test('unmounting stops the polling loop', async () => {
+ jest.useFakeTimers();
+ mockSupersetClient.post.mockResolvedValue({
+ json: { job_id: 'abc' },
+ } as never);
+ mockSupersetClient.get.mockResolvedValue({
+ json: { status: 'pending' },
+ } as never);
+
+ const { unmount } = render(<MenuWrapper />, {
+ useRedux: true,
+ initialState: loggedInState,
+ });
+
+ await userEvent.click(screen.getByText('Export Data to Excel'));
+ await waitFor(() => expect(mockSupersetClient.post).toHaveBeenCalled());
+
+ await act(async () => {
+ jest.advanceTimersByTime(3000);
+ });
+ await waitFor(() => expect(mockSupersetClient.get).toHaveBeenCalledTimes(1));
+
+ unmount();
+
+ await act(async () => {
+ jest.advanceTimersByTime(30000);
+ });
+ expect(mockSupersetClient.get).toHaveBeenCalledTimes(1);
+});
+
+test('the pending toast is announced once, not re-emitted on every poll',
async () => {
+ jest.useFakeTimers();
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Fake timers hang click</b></div>
<div id="fix">
Same hang risk: `await userEvent.click()` at line 328 uses user-event v12's
internal setTimeout, which plain fake timers never advance. Use
`jest.useFakeTimers({ advanceTimers: true })`.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
````suggestion
jest.useFakeTimers({ advanceTimers: true });
````
</div>
</details>
</div>
<small><i>Code Review Run #084afd</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/dashboard/components/menu/DownloadMenuItems/DownloadMenuItems.test.tsx:
##########
@@ -368,3 +533,258 @@ test('Enabled screenshot items should not show tooltip
icon', () => {
mockIsFeatureEnabled.mockReset();
});
+
+// ---------------------------------------------------------------------------
+// Delivery follows the requester identity, not iframe presence: a guest or
+// anonymous session (no userId) has no email channel, so the toast must not
+// promise one, and the image export (webdriver-rendered, guests cannot open
+// Explore) is hidden.
+// ---------------------------------------------------------------------------
+
+const guestState = { user: {} };
+
+test('guest session: export toast promises auto-download, not an email', async
() => {
+ mockSupersetClient.post.mockResolvedValue({
+ json: { job_id: 'abc' },
+ } as never);
+
+ render(<MenuWrapper />, { useRedux: true, initialState: guestState });
+
+ await userEvent.click(screen.getByText('Export Data to Excel'));
+
+ await waitFor(() =>
+ expect(mockAddInfoToast).toHaveBeenCalledWith(
+ 'Your export is being generated. Please, do not leave the page.',
+ { noDuplicate: true },
+ ),
+ );
+});
+
+test('guest session: Export Images to Excel is hidden even with the webdriver
enabled', () => {
+ enableWebDriverScreenshot();
+
+ render(<MenuWrapper />, { useRedux: true, initialState: guestState });
+
+ expect(screen.getByText('Export Data to Excel')).toBeInTheDocument();
+ expect(screen.queryByText('Export Images to Excel')).not.toBeInTheDocument();
+});
+
+test('logged-in user without an email gets the delivery-neutral toast', async
() => {
+ mockSupersetClient.post.mockResolvedValue({
+ json: { job_id: 'abc' },
+ } as never);
+
+ render(<MenuWrapper />, {
+ useRedux: true,
+ initialState: { user: { userId: 1 } },
+ });
+
+ await userEvent.click(screen.getByText('Export Data to Excel'));
+
+ await waitFor(() =>
+ expect(mockAddInfoToast).toHaveBeenCalledWith(
+ 'Your export is being generated. Please, do not leave the page.',
+ { noDuplicate: true },
+ ),
+ );
+});
+
+test('a "running" status restarts the wait window, so queue delay is not
counted', async () => {
+ jest.useFakeTimers();
+ mockSupersetClient.post.mockResolvedValue({
+ json: { job_id: 'abc' },
+ } as never);
+ // Worker picks the job up on the first poll; still running just past the
+ // original 12 minute deadline; done on the poll after that.
+ mockSupersetClient.get
+ .mockResolvedValueOnce({ json: { status: 'running' } } as never)
+ .mockResolvedValueOnce({ json: { status: 'running' } } as never)
+ .mockResolvedValueOnce({
+ json: {
+ status: 'ready',
+ download_url: '/api/v1/dashboard/export_xlsx/download/abc/',
+ },
+ } as never);
+
+ render(<MenuWrapper />, { useRedux: true, initialState: loggedInState });
+
+ await userEvent.click(screen.getByText('Export Data to Excel'));
+ await waitFor(() => expect(mockSupersetClient.post).toHaveBeenCalled());
+
+ await act(async () => {
+ jest.advanceTimersByTime(3000);
+ });
+ await waitFor(() => expect(mockSupersetClient.get).toHaveBeenCalledTimes(1));
+
+ // t ~= 12m01s: past the enqueue-based deadline, within the restarted one
+ // (running was observed at t=3s). Without the restart this poll would give
+ // up with a danger toast instead of continuing.
+ await act(async () => {
+ jest.advanceTimersByTime(12 * 60 * 1000 - 2000);
+ });
+ await waitFor(() => expect(mockSupersetClient.get).toHaveBeenCalledTimes(2));
+ expect(mockAddDangerToast).not.toHaveBeenCalled();
+
+ await act(async () => {
+ jest.advanceTimersByTime(3000);
+ });
+ await waitFor(() => {
+
expect(lastIframeSrc()).toBe('/api/v1/dashboard/export_xlsx/download/abc/');
+ expect(mockAddSuccessToast).toHaveBeenCalledWith(
+ 'Your export is ready and downloading.',
+ );
+ });
+});
+
+test('a transient poll failure keeps polling and still downloads', async () =>
{
+ jest.useFakeTimers();
+ mockSupersetClient.post.mockResolvedValue({
+ json: { job_id: 'abc' },
+ } as never);
+ mockSupersetClient.get
+ .mockRejectedValueOnce(new Error('network blip'))
+ .mockResolvedValueOnce({
+ json: {
+ status: 'ready',
+ download_url: '/api/v1/dashboard/export_xlsx/download/abc/',
+ },
+ } as never);
+
+ render(<MenuWrapper />, { useRedux: true, initialState: loggedInState });
+
+ await userEvent.click(screen.getByText('Export Data to Excel'));
+ await waitFor(() => expect(mockSupersetClient.post).toHaveBeenCalled());
+
+ await act(async () => {
+ jest.advanceTimersByTime(3000);
+ });
+ await waitFor(() => expect(mockSupersetClient.get).toHaveBeenCalledTimes(1));
+ expect(mockAddDangerToast).not.toHaveBeenCalled();
+
+ await act(async () => {
+ jest.advanceTimersByTime(3000);
+ });
+ await waitFor(() => {
+
expect(lastIframeSrc()).toBe('/api/v1/dashboard/export_xlsx/download/abc/');
+ expect(mockAddSuccessToast).toHaveBeenCalledWith(
+ 'Your export is ready and downloading.',
+ );
+ });
+});
+
+test('poll failures past the deadline give up with an error toast', async ()
=> {
+ jest.useFakeTimers();
+ mockSupersetClient.post.mockResolvedValue({
+ json: { job_id: 'abc' },
+ } as never);
+ mockSupersetClient.get.mockRejectedValue(new Error('server down'));
+
+ render(<MenuWrapper />, { useRedux: true, initialState: loggedInState });
+
+ await userEvent.click(screen.getByText('Export Data to Excel'));
+ await waitFor(() => expect(mockSupersetClient.post).toHaveBeenCalled());
+
+ await act(async () => {
+ jest.advanceTimersByTime(3000);
+ });
+ await waitFor(() => expect(mockSupersetClient.get).toHaveBeenCalledTimes(1));
+
+ // Jump past the 12 minute deadline; the next failing poll must give up.
+ await act(async () => {
+ jest.advanceTimersByTime(13 * 60 * 1000);
+ });
+ await waitFor(() => {
+ expect(mockAddDangerToast).toHaveBeenCalledWith(
+ 'Sorry, something went wrong. Try again later.',
+ );
+ });
+ expect(mockAddSuccessToast).not.toHaveBeenCalledWith(
+ 'Your export is ready and downloading.',
+ );
+});
+
+test('a "ready" status with no download_url is an error, not a fake success',
async () => {
+ jest.useFakeTimers();
+ mockSupersetClient.post.mockResolvedValue({
+ json: { job_id: 'abc' },
+ } as never);
+ mockSupersetClient.get.mockResolvedValue({
+ json: { status: 'ready' },
+ } as never);
+
+ render(<MenuWrapper />, { useRedux: true, initialState: loggedInState });
+
+ await userEvent.click(screen.getByText('Export Data to Excel'));
+ await waitFor(() => expect(mockSupersetClient.post).toHaveBeenCalled());
+
+ await act(async () => {
+ jest.advanceTimersByTime(3000);
+ });
+
+ await waitFor(() => {
+ expect(mockAddDangerToast).toHaveBeenCalledWith(
+ 'Sorry, something went wrong. Try again later.',
+ );
+ });
+ expect(mockAddSuccessToast).not.toHaveBeenCalled();
+ expect(mockAddSuccessToast).not.toHaveBeenCalledWith(
+ 'Your export is ready and downloading.',
+ );
+});
+
+test('unmounting stops the polling loop', async () => {
+ jest.useFakeTimers();
+ mockSupersetClient.post.mockResolvedValue({
+ json: { job_id: 'abc' },
+ } as never);
+ mockSupersetClient.get.mockResolvedValue({
+ json: { status: 'pending' },
+ } as never);
+
+ const { unmount } = render(<MenuWrapper />, {
+ useRedux: true,
+ initialState: loggedInState,
+ });
+
+ await userEvent.click(screen.getByText('Export Data to Excel'));
+ await waitFor(() => expect(mockSupersetClient.post).toHaveBeenCalled());
+
+ await act(async () => {
+ jest.advanceTimersByTime(3000);
+ });
+ await waitFor(() => expect(mockSupersetClient.get).toHaveBeenCalledTimes(1));
+
+ unmount();
+
+ await act(async () => {
+ jest.advanceTimersByTime(30000);
+ });
+ expect(mockSupersetClient.get).toHaveBeenCalledTimes(1);
+});
+
+test('the pending toast is announced once, not re-emitted on every poll',
async () => {
+ jest.useFakeTimers();
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Fake timers hang click</b></div>
<div id="fix">
Same hang risk: `await userEvent.click()` at line 292 uses user-event v12's
internal setTimeout, which plain fake timers never advance. Use
`jest.useFakeTimers({ advanceTimers: true })`.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
````suggestion
jest.useFakeTimers({ advanceTimers: true });
````
</div>
</details>
</div>
<small><i>Code Review Run #084afd</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]
