EnxDev commented on code in PR #44345:
URL: https://github.com/apache/superset/pull/44345#discussion_r4080943686
##########
superset-frontend/src/dashboard/components/gridComponents/Tabs/Tabs.test.tsx:
##########
@@ -265,6 +265,144 @@ test('Switching tabs', async () => {
expect(props.onChangeTab).toHaveBeenCalled();
});
+test.each([false, true])(
+ 'A childless TABS component does not register an active tab (editMode=%s)',
+ editMode => {
+ // Regression guard for the permalink failure caused by empty tab
containers:
+ // a TABS component with no children has no tab id to activate, so
resolving
+ // `children[tabIndex]` yields `undefined`. Dispatching that into
+ // `dashboardState.activeTabs` puts an `undefined` entry in the array,
which
+ // `JSON.stringify` coerces to `null` in the permalink request body.
+ const props = createProps();
+ props.editMode = editMode;
+ props.component.children = [];
+
+ render(<Tabs {...props} />, {
+ useRedux: true,
+ useDnd: true,
+ });
+
+ expect(props.setActiveTab).not.toHaveBeenCalled();
+ },
+);
+
+test.each([false, true])(
+ 'The first child added to an empty TABS component is activated
(editMode=%s)',
+ editMode => {
+ // Counterpart to the guard above: skipping registration while there is no
+ // tab id must not leave the container permanently unregistered.
`activeKey`
+ // is seeded once from state, so an empty container that later receives its
+ // first child has to resolve and register that child, otherwise the tab
+ // renders unselected and never reaches dashboardState.activeTabs.
+ const props = createProps();
+ const tabId = props.component.children[0];
+ props.editMode = editMode;
+ props.component.children = [];
+ const { rerender } = render(<Tabs {...props} />, {
+ useRedux: true,
+ useDnd: true,
+ });
+
+ expect(props.setActiveTab).not.toHaveBeenCalled();
+ rerender(
+ <Tabs {...props} component={{ ...props.component, children: [tabId] }}
/>,
+ );
+
+ // Exactly one registration, carrying the resolved id and no stale previous
+ // tab -- never an `undefined` that JSON.stringify would turn into `null`.
+ expect(props.setActiveTab.mock.calls).toEqual([[tabId]]);
+ expect(screen.getByRole('tab')).toHaveAttribute('aria-selected', 'true');
+ },
+);
+
+test('A tab added after deleting the last tab is selected and registered',
async () => {
+ const props = createProps();
+ const [deletedTabId, newTabId] = props.component.children;
+ props.component.children = [deletedTabId];
+ const { rerender } = render(<Tabs {...props} />, {
+ useRedux: true,
+ useDnd: true,
+ });
+
+ expect(props.setActiveTab.mock.calls).toEqual([[deletedTabId]]);
+ expect(screen.getByRole('tab')).toHaveAttribute('aria-selected', 'true');
+
+ await userEvent.click(screen.getByRole('button', { name: 'remove' }));
+ await userEvent.click(screen.getByRole('button', { name: 'Delete' }));
+ expect(props.deleteComponent).toHaveBeenCalledWith(
+ deletedTabId,
+ props.component.id,
+ );
+
+ rerender(
+ <Tabs {...props} component={{ ...props.component, children: [] }} />,
+ );
+ expect(screen.queryByRole('tab')).not.toBeInTheDocument();
+ props.setActiveTab.mockClear();
+
+ await userEvent.click(screen.getByRole('button', { name: 'Add tab' }));
+ expect(props.createComponent).toHaveBeenCalled();
+ expect(props.setActiveTab).not.toHaveBeenCalled();
+
+ rerender(
+ <Tabs
+ {...props}
+ component={{ ...props.component, children: [newTabId] }}
+ />,
+ );
+
+ expect(props.setActiveTab.mock.calls).toEqual([[newTabId, deletedTabId]]);
Review Comment:
Not taking this one. `rerender` is wrapped in `act`, so effects flush before
it returns — and an empty `mock.calls` would not pass vacuously in any case,
since `expect([]).toEqual([[newTabId, deletedTabId]])` fails.
Checked it rather than argued it: with the resolve effect in `Tabs.tsx`
disabled, this assertion is one of exactly three that go red.
```
● A tab added after deleting the last tab is selected and registered
● The first child added to an empty TABS component is activated
(editMode=false)
● The first child added to an empty TABS component is activated
(editMode=true)
```
So it is the assertion catching the regression, not hiding it; wrapping it
in `waitFor` would only defer a check that already fires.
Same for the follow-on note on line 345 — there is no call shape to pin when
the assertion is that nothing was called.
--
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]
