sadpandajoe commented on code in PR #37813:
URL: https://github.com/apache/superset/pull/37813#discussion_r2784525351
##########
superset-frontend/src/pages/ChartList/ChartList.test.tsx:
##########
@@ -319,107 +253,32 @@ describe('ChartList - Global Filter Interactions', () =>
{
).mockReset();
});
- test('renders search filter correctly', async () => {
+ test('renders all standard filters', async () => {
renderChartList(mockUser);
await screen.findByTestId('chart-list-view');
-
await waitFor(() => {
expect(screen.getByTestId('listview-table')).toBeInTheDocument();
});
- // Verify search filter renders correctly
+ // Search filter
expect(screen.getByTestId('filters-search')).toBeInTheDocument();
expect(screen.getByPlaceholderText(/type a value/i)).toBeInTheDocument();
- });
- test('renders Type filter correctly', async () => {
- renderChartList(mockUser);
- await screen.findByTestId('chart-list-view');
-
- await waitFor(() => {
- expect(screen.getByTestId('listview-table')).toBeInTheDocument();
+ // All standard select filters
+ const standardFilters = [
+ 'Type',
+ 'Dataset',
+ 'Owner',
+ 'Certified',
+ 'Favorite',
+ 'Dashboard',
+ 'Modified by',
+ ];
+ standardFilters.forEach(filterLabel => {
+ const filter = findFilterByLabel(filterLabel);
+ expect(filter).toBeVisible();
+ expect(filter).toBeEnabled();
});
Review Comment:
The helper already produces a clear Jest failure (`received value must be an
HTMLElement`) when it returns null — it won't silently pass. Changing
`findFilterByLabel` to throw would be an improvement, but it's a pre-existing
helper outside this PR's scope.
##########
superset-frontend/src/pages/ChartList/ChartList.listview.test.tsx:
##########
@@ -585,7 +543,35 @@ test('ChartList list view displays chart with empty on
dashboards column', async
expect(within(dashboardCell).queryByRole('link')).not.toBeInTheDocument();
});
-test('ChartList list view shows tag info when TAGGING_SYSTEM is enabled',
async () => {
+test('renders dashboard crosslinks as navigable links', async () => {
+ renderChartList(mockUser);
+ await waitFor(() => {
+ expect(screen.getByTestId('listview-table')).toBeInTheDocument();
+ });
+
+ const table = screen.getByTestId('listview-table');
+
+ // mockCharts[1] has multiple dashboards - verify all render with correct
hrefs
+ const chartRow = within(table)
+ .getByText(mockCharts[1].slice_name)
+ .closest('[data-test="table-row"]') as HTMLElement;
+ const crosslinks = within(chartRow).getByTestId('crosslinks');
+ const dashboards = mockCharts[1].dashboards as {
+ dashboard_title: string;
+ id: number;
+ }[];
+ const links = within(crosslinks).getAllByRole('link');
+ expect(links).toHaveLength(dashboards.length);
+ dashboards.forEach(dashboard => {
+ expect(
+ within(crosslinks).getByRole('link', {
+ name: new RegExp(dashboard.dashboard_title),
+ }),
+ ).toHaveAttribute('href', `/superset/dashboard/${dashboard.id}`);
Review Comment:
The dashboard titles are defined in our own mock data (`'Sales Dashboard'`,
etc.) — none contain regex metacharacters. The `getByRole + RegExp` pattern is
idiomatic RTL throughout this codebase. Switching to `textContent?.includes()`
would lose RTL's role-based query semantics and weaken the assertion.
--
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]
