geido commented on code in PR #31858:
URL: https://github.com/apache/superset/pull/31858#discussion_r1916860082
##########
superset-frontend/spec/helpers/theming.tsx:
##########
@@ -55,3 +57,6 @@ export function styledShallow(
},
});
}
+
+export const renderWithTheme = (component: JSX.Element) =>
Review Comment:
Does this belong to `superset-frontend/spec/helpers/testing-library.tsx`?
##########
superset-frontend/spec/helpers/theming.tsx:
##########
@@ -55,3 +57,6 @@ export function styledShallow(
},
});
}
+
+export const renderWithTheme = (component: JSX.Element) =>
Review Comment:
Also, it seems our RTL helpers should already be wrapping components around
a `ThemeProvider`?
##########
superset-frontend/src/components/ErrorMessage/InvalidSQLErrorMessage.test.tsx:
##########
@@ -44,83 +43,66 @@ const defaultProps = {
subtitle: 'Test subtitle',
};
-const setup = (overrides = {}) => (
- <ThemeProvider theme={supersetTheme}>
- <InvalidSQLErrorMessage {...defaultProps} {...overrides} />;
- </ThemeProvider>
-);
-
-// Mock the ErrorAlert component
-jest.mock('./ErrorAlert', () => ({
- __esModule: true,
- default: ({
- title,
- subtitle,
- level,
- source,
- body,
- }: {
- title: React.ReactNode;
- subtitle?: React.ReactNode;
- level: ErrorLevel;
- source: ErrorSource;
- body: React.ReactNode;
- }) => (
- <div data-test="error-alert">
- <div data-test="title">{title}</div>
- <div data-test="subtitle">{subtitle}</div>
- <div data-test="level">{level}</div>
- <div data-test="source">{source}</div>
- <div data-test="body">{body}</div>
- </div>
- ),
-}));
+const renderComponent = (overrides = {}) =>
+ render(
+ <ThemeProvider theme={supersetTheme}>
+ <InvalidSQLErrorMessage {...defaultProps} {...overrides} />
+ </ThemeProvider>,
+ );
describe('InvalidSQLErrorMessage', () => {
- it('renders ErrorAlert with correct props', () => {
- const { getByTestId } = render(setup());
+ it('renders the error message with correct properties', () => {
+ const { getByText } = renderComponent();
- expect(getByTestId('error-alert')).toBeInTheDocument();
- expect(getByTestId('title')).toHaveTextContent('Unable to parse SQL');
- expect(getByTestId('subtitle')).toHaveTextContent('Test subtitle');
- expect(getByTestId('level')).toHaveTextContent('error');
- expect(getByTestId('source')).toHaveTextContent('test');
+ // Validate main properties
+ expect(getByText('Unable to parse SQL')).toBeInTheDocument();
+ expect(getByText('Test subtitle')).toBeInTheDocument();
+ expect(getByText('SELECT * FFROM table')).toBeInTheDocument();
});
- it('displays the error line and column indicator', () => {
- const { getByTestId } = render(setup());
+ it('displays the SQL error line and column indicator', () => {
+ const { getByText, container } = renderComponent();
+
+ // Validate SQL and caret indicator
+ expect(getByText('SELECT * FFROM table')).toBeInTheDocument();
- const body = getByTestId('body');
- expect(body).toContainHTML('<pre>SELECT * FFROM table</pre>');
- expect(body).toContainHTML('<pre> ^</pre>');
+ // Check for caret (`^`) under the error column
+ const preTags = container.querySelectorAll('pre');
+ const secondPre = preTags[1];
+ console.log({ secondPre });
Review Comment:
```suggestion
```
##########
superset-frontend/src/components/ErrorMessage/ErrorMessageWithStackTrace.tsx:
##########
@@ -66,28 +68,27 @@ export default function ErrorMessageWithStackTrace({
if (fallback) {
return <>{fallback}</>;
}
+ const computedDescriptionDetails =
+ descriptionDetails ||
+ (link || stackTrace ? (
+ <>
+ {link && (
+ <a href={link} target="_blank" rel="noopener noreferrer">
+ (Request Access)
Review Comment:
Should we localize this?
--
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]
