sadpandajoe commented on code in PR #36723:
URL: https://github.com/apache/superset/pull/36723#discussion_r2629966625
##########
superset-frontend/src/components/Datasource/components/DatasourceEditor/tests/DatasourceEditorCurrency.test.tsx:
##########
@@ -66,99 +82,57 @@ afterEach(async () => {
test('renders currency section in metrics tab', async () => {
const testProps = createPropsWithCurrency();
- await asyncRender(testProps);
+ fastRender(testProps);
- await dismissDatasourceWarning();
+ await setupCurrencySection();
- // Navigate to metrics tab
- const metricButton = await screen.findByTestId('collection-tab-Metrics');
- await userEvent.click(metricButton);
-
- // Expand the single metric row with currency
- const expandToggles = await screen.findAllByLabelText(/expand row/i);
- await userEvent.click(expandToggles[0]);
-
- // Check for currency section header
- const currencyHeader = await screen.findByText('Metric currency');
- expect(currencyHeader).toBeVisible();
-
- // Verify currency position selector exists
- const positionSelector = screen.getByRole('combobox', {
- name: 'Currency prefix or suffix',
- });
- expect(positionSelector).toBeInTheDocument();
-
- // Verify currency symbol selector exists
- const symbolSelector = screen.getByRole('combobox', {
- name: 'Currency symbol',
- });
- expect(symbolSelector).toBeInTheDocument();
+ // Verify currency selectors exist
+ expect(
+ screen.getByRole('combobox', { name: 'Currency prefix or suffix' }),
+ ).toBeInTheDocument();
+ expect(
+ screen.getByRole('combobox', { name: 'Currency symbol' }),
+ ).toBeInTheDocument();
});
-// Allow extra headroom for dropdown render on slower CI runners
test('changes currency position from prefix to suffix', async () => {
const testProps = createPropsWithCurrency();
+ fastRender(testProps);
- await asyncRender(testProps);
-
- await dismissDatasourceWarning();
+ await setupCurrencySection();
- // Navigate to metrics tab
- const metricButton = await screen.findByTestId('collection-tab-Metrics');
- await userEvent.click(metricButton);
-
- // Expand the metric with currency
- const expandToggles = await screen.findAllByLabelText(/expand row/i);
- await userEvent.click(expandToggles[0]);
-
- // Select suffix option via shared helper (rc-virtual-list aware)
await selectOption('Suffix', 'Currency prefix or suffix');
- await cleanupAsyncOperations();
- // Verify onChange was called with suffix position
await waitFor(() => {
expect(testProps.onChange).toHaveBeenCalledTimes(1);
- const callArg = testProps.onChange.mock.calls[0][0];
-
- const metrics = callArg.metrics || [];
- const updatedMetric = metrics.find(
- (m: MetricType) => m.currency && m.currency.symbolPosition === 'suffix',
- );
-
- expect(updatedMetric?.currency?.symbol).toBe('USD');
});
-}, 30000);
-// Allow extra headroom for dropdown render on slower CI runners
+ // Verify the exact call arguments
+ const callArg = testProps.onChange.mock.calls[0][0];
+ const metrics = callArg.metrics || [];
+ const updatedMetric = metrics.find(
+ (m: MetricType) => m.currency?.symbolPosition === 'suffix',
+ );
+ expect(updatedMetric?.currency?.symbol).toBe('USD');
+}, 60000);
Review Comment:
nit: we shouldn't bump the timeout unless if there is a reason to. I think
we could have bumped it originally to make the test less flakey, but then that
would hide other issues. Is there a reason why we need to bump it now?
--
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]
