QT/superset2
1
0
Fork 0
mirror of https://github.com/apache/superset.git synced 2026-05-29 20:29:34 +00:00
Code Issues Packages Projects Releases Wiki Activity

test(SaveModal,CopyToClipboard): drop placeholder asserts; add string/number copyNode guard

Browse Source 
Two changes in response to review feedback on the FC conversion:

1. Drop three `expect(true).toBe(true)` placeholder tests in
   `SaveModal.test.tsx` (`dispatches removeChartState ... - placeholder`,
   `onDashboardChange triggers tabs load ... - placeholder`,
   `onTabChange correctly updates selectedTab - placeholder`). They were
   left behind from the class-component version and assert nothing —
   net-negative as regression guards. They'll be re-added in a follow-up
   once the FC-shaped versions are wired up.

2. Add a regression test in `CopyToClipboard.test.tsx` that renders the
   component with a string `copyNode` and with a number `copyNode`,
   covering the `cloneElement`-on-non-element crash that prompted the
   `isValidElement` guard in `src/components/CopyToClipboard/index.tsx`.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This commit is contained in:
Claude
2026-05-12 13:12:51 -07:00
parent f8f04a427e
commit be1dbac02e
2 changed files with 14 additions and 34 deletions
Download Patch File Download Diff File Expand all files Collapse all files

14
superset-frontend/src/components/CopyToClipboard/CopyToClipboard.test.tsx
View File

@@ -37,6 +37,20 @@ test('renders with custom copy node', () => {
expect(screen.getByRole('link')).toBeInTheDocument();
});
// Regression guard: passing a non-element copyNode (string or number) used to
// crash because cloneElement only accepts React elements. The render path now
// gates the cloneElement call behind isValidElement and falls back to a span
// wrapper, so plain primitives should render without throwing.
test('renders with string copyNode without crashing', () => {
render(<CopyToClipboard copyNode="just text" />, { useRedux: true });
expect(screen.getByRole('button')).toHaveTextContent('just text');
});
test('renders with number copyNode without crashing', () => {
render(<CopyToClipboard copyNode={42} />, { useRedux: true });
expect(screen.getByRole('button')).toHaveTextContent('42');
});
test('renders without text showing', () => {
const text = 'Text';
render(<CopyToClipboard text={text} shouldShowText={false} />, {

34
superset-frontend/src/explore/components/SaveModal.test.tsx
View File

@@ -410,18 +410,6 @@ test('createRedirectParams removes form_data_key from URL parameters', () => {
expect(result.get('save_action')).toEqual('overwrite');
});
/**
* TODO: This test was written for the class component version of SaveModal.
* Since SaveModal has been converted to a function component, this test
* needs to be rewritten to test through component rendering and user interaction.
* The test should verify that clicking "Save & go to dashboard" dispatches
* removeChartState with the correct chart ID.
*/
test('dispatches removeChartState when saving and going to dashboard - placeholder', () => {
// See TODO comment above
expect(true).toBe(true);
});
test('disables tab selector when no dashboard selected', () => {
const { getByRole, getByTestId } = setup();
fireEvent.click(getByRole('radio', { name: 'Save as...' }));
@@ -440,28 +428,6 @@ test('renders tab selector when saving as', async () => {
expect(tabSelector).toBeDisabled();
});
/**
* TODO: This test was written for the class component version of SaveModal.
* Since SaveModal has been converted to a function component, this test
* needs to be rewritten to test through component rendering and user interaction.
* The test should verify that selecting a dashboard triggers tab loading.
*/
test('onDashboardChange triggers tabs load for existing dashboard - placeholder', () => {
// See TODO comment above
expect(true).toBe(true);
});
/**
* TODO: This test was written for the class component version of SaveModal.
* Since SaveModal has been converted to a function component, this test
* needs to be rewritten to test through component rendering and user interaction.
* The test should verify that changing the tab selection updates the component state.
*/
test('onTabChange correctly updates selectedTab - placeholder', () => {
// See TODO comment above
expect(true).toBe(true);
});
test('chart placement logic finds row with available space', () => {
// Test case 1: Row has space (8 + 4 = 12 <= 12)
const positionJson1 = {