[GitHub] [superset] etr2460 commented on a change in pull request #16483: refactor: Changes the DatabaseSelector and TableSelector to use the new Select component

[GitBox <gi...@apache.org>]

etr2460 commented on a change in pull request #16483:
URL: https://github.com/apache/superset/pull/16483#discussion_r697534012



##########
File path: superset-frontend/src/components/DatabaseSelector/DatabaseSelector.test.tsx
##########
@@ -153,50 +153,62 @@ test('Refresh should work', async () => {
 
   render(<DatabaseSelector {...props} />);
 
+  const select = screen.getByRole('combobox', {
+    name: 'Select a schema',
+  });
+
+  userEvent.click(select);
+
   await waitFor(() => {
-    expect(SupersetClientGet).toBeCalledTimes(2);
-    expect(props.getDbList).toBeCalledTimes(1);
+    expect(SupersetClientGet).toBeCalledTimes(1);
+    expect(props.getDbList).toBeCalledTimes(0);
     expect(props.getTableList).toBeCalledTimes(0);
     expect(props.handleError).toBeCalledTimes(0);
     expect(props.onDbChange).toBeCalledTimes(0);
     expect(props.onSchemaChange).toBeCalledTimes(0);
-    expect(props.onSchemasLoad).toBeCalledTimes(1);
+    expect(props.onSchemasLoad).toBeCalledTimes(0);
     expect(props.onUpdate).toBeCalledTimes(0);
   });
 
-  userEvent.click(screen.getByRole('button'));
+  userEvent.click(screen.getByRole('button', { name: 'refresh' }));
 
   await waitFor(() => {
-    expect(SupersetClientGet).toBeCalledTimes(3);
-    expect(props.getDbList).toBeCalledTimes(1);
+    expect(SupersetClientGet).toBeCalledTimes(2);
+    expect(props.getDbList).toBeCalledTimes(0);
     expect(props.getTableList).toBeCalledTimes(0);
     expect(props.handleError).toBeCalledTimes(0);
-    expect(props.onDbChange).toBeCalledTimes(1);
-    expect(props.onSchemaChange).toBeCalledTimes(1);
+    expect(props.onDbChange).toBeCalledTimes(0);
+    expect(props.onSchemaChange).toBeCalledTimes(0);
     expect(props.onSchemasLoad).toBeCalledTimes(2);
-    expect(props.onUpdate).toBeCalledTimes(1);
+    expect(props.onUpdate).toBeCalledTimes(0);
   });
 });
 
 test('Should database select display options', async () => {
   const props = createProps();
   render(<DatabaseSelector {...props} />);
-  const selector = await screen.findByText('Database:');
-  expect(selector).toBeInTheDocument();
-  expect(selector.parentElement).toHaveTextContent(
-    'Database:postgresql examples',
-  );
+  const select = screen.getByRole('combobox', {
+    name: 'Select a database',
+  });
+  expect(select).toBeInTheDocument();
+  userEvent.click(select);
+  expect(
+    await screen.findByRole('option', { name: 'postgresql: test' }),

Review comment:
       should we also add `expect(props.onDbChange).toBeCalledTimes(1);` here to catch the previous regression?

##########
File path: superset-frontend/src/explore/components/controls/DatasourceControl/index.jsx
##########
@@ -227,10 +227,7 @@ class DatasourceControl extends React.PureComponent {
             </Tooltip>
           )}
           {extra?.warning_markdown && (
-            <WarningIconWithTooltip
-              warningMarkdown={extra.warning_markdown}
-              size={30}

Review comment:
       can you attach a screenshot of this change? i seem to recall a reason for the different size




-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org