Copilot commented on code in PR #37813:
URL: https://github.com/apache/superset/pull/37813#discussion_r2784356373
##########
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:
This uses `new RegExp(dashboard.dashboard_title)` without escaping. If a
dashboard title ever includes regex metacharacters, the test can fail (or match
multiple links). Prefer asserting against the `links` array returned by
`getAllByRole('link')` (text contains title + href), or escape the title before
building a regex.
```suggestion
const matchingLink = links.find(link =>
link.textContent?.includes(dashboard.dashboard_title),
);
expect(matchingLink).toBeDefined();
expect(matchingLink).toHaveAttribute(
'href',
`/superset/dashboard/${dashboard.id}`,
);
```
##########
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:
`findFilterByLabel()` can return `null`, but the test immediately calls
`.toBeVisible()` / `.toBeEnabled()` on the result. Adding an explicit non-null
assertion (or throwing a descriptive error inside `findFilterByLabel`) will
make failures clearer when a filter label changes.
--
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]
