codeant-ai-for-open-source[bot] commented on code in PR #37482:
URL: https://github.com/apache/superset/pull/37482#discussion_r2738321923
##########
superset-frontend/src/dashboard/components/SliceHeaderControls/SliceHeaderControls.test.tsx:
##########
@@ -616,3 +616,118 @@ test('Should pass formData to Share menu for embed code
feature', () => {
openMenu();
expect(screen.getByText('Share')).toBeInTheDocument();
});
+
+test('Should show single fetched query tooltip with timestamp', () => {
+ const updatedDttm = Date.parse('2024-01-28T10:00:00.000Z');
+ const props = createProps();
+ props.isCached = [false];
+ props.cachedDttm = [''];
+ props.updatedDttm = updatedDttm;
+
+ renderWrapper(props);
+ openMenu();
+
+ const refreshButton = screen.getByText('Force refresh');
+ expect(refreshButton).toBeInTheDocument();
+
+ const tooltipContainer = refreshButton.closest('div');
+ expect(tooltipContainer?.textContent).toMatch(/Fetched/);
+});
+
+test('Should show single cached query tooltip with timestamp', () => {
+ const cachedDttm = '2024-01-28T10:00:00.000Z';
+ const props = createProps();
+ props.isCached = [true];
+ props.cachedDttm = [cachedDttm];
+ props.updatedDttm = null;
+
+ renderWrapper(props);
+ openMenu();
+
+ const refreshButton = screen.getByText('Force refresh');
+ expect(refreshButton).toBeInTheDocument();
+
+ const tooltipContainer = refreshButton.closest('div');
+ expect(tooltipContainer?.textContent).toMatch(/Cached/);
Review Comment:
**Suggestion:** The test checks tooltip content by inspecting a parent div
synchronously, but cached tooltips are shown on hover and may be rendered
elsewhere asynchronously; change the test to be async, hover the refresh
button, and await the tooltip text. [race condition]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Flaky unit test for SliceHeaderControls.
- ❌ CI Jest run may fail intermittently.
```
</details>
```suggestion
test('Should show single cached query tooltip with timestamp', async () => {
const cachedDttm = '2024-01-28T10:00:00.000Z';
const props = createProps();
props.isCached = [true];
props.cachedDttm = [cachedDttm];
props.updatedDttm = null;
renderWrapper(props);
openMenu();
const refreshButton = screen.getByText('Force refresh');
expect(refreshButton).toBeInTheDocument();
userEvent.hover(refreshButton);
expect(await screen.findByText(/Cached/)).toBeInTheDocument();
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Run the test file
superset-frontend/src/dashboard/components/SliceHeaderControls/SliceHeaderControls.test.tsx
that contains the cached tooltip test starting at line 637.
2. The test sets props to indicate the query is cached (lines 639-642),
renders the
component (line 644) and opens the menu via openMenu() (line 645).
3. It then selects the "Force refresh" button (line 647) and immediately
inspects
refreshButton.closest('div') for /Cached/ (lines 650-651), without
triggering hover or
awaiting tooltip rendering.
4. Since the tooltip is displayed on hover and can be rendered outside the
immediate DOM
tree asynchronously, the synchronous inspection can miss the tooltip —
reproduce by
running the single test (`jest -t "single cached"`) and observing
intermittent failures
(no /Cached/).
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/src/dashboard/components/SliceHeaderControls/SliceHeaderControls.test.tsx
**Line:** 637:651
**Comment:**
*Race Condition: The test checks tooltip content by inspecting a parent
div synchronously, but cached tooltips are shown on hover and may be rendered
elsewhere asynchronously; change the test to be async, hover the refresh
button, and await the tooltip text.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
##########
superset-frontend/src/dashboard/components/SliceHeaderControls/SliceHeaderControls.test.tsx:
##########
@@ -616,3 +616,118 @@ test('Should pass formData to Share menu for embed code
feature', () => {
openMenu();
expect(screen.getByText('Share')).toBeInTheDocument();
});
+
+test('Should show single fetched query tooltip with timestamp', () => {
+ const updatedDttm = Date.parse('2024-01-28T10:00:00.000Z');
+ const props = createProps();
+ props.isCached = [false];
+ props.cachedDttm = [''];
+ props.updatedDttm = updatedDttm;
+
+ renderWrapper(props);
+ openMenu();
+
+ const refreshButton = screen.getByText('Force refresh');
+ expect(refreshButton).toBeInTheDocument();
+
+ const tooltipContainer = refreshButton.closest('div');
+ expect(tooltipContainer?.textContent).toMatch(/Fetched/);
Review Comment:
**Suggestion:** The test asserts tooltip content by reading a parent div
immediately after opening the menu, but tooltips are rendered on hover (often
in a portal) and appear asynchronously; the test never triggers hover or awaits
the tooltip, making it flaky or failing. Change the test to be async, hover the
refresh button, and await the tooltip via `findByText`. [race condition]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Flaky unit test for SliceHeaderControls.
- ❌ CI Jest run may fail intermittently.
- ⚠️ Dashboard test suite instability in PR validation.
```
</details>
```suggestion
test('Should show single fetched query tooltip with timestamp', async () => {
const updatedDttm = Date.parse('2024-01-28T10:00:00.000Z');
const props = createProps();
props.isCached = [false];
props.cachedDttm = [''];
props.updatedDttm = updatedDttm;
renderWrapper(props);
openMenu();
const refreshButton = screen.getByText('Force refresh');
expect(refreshButton).toBeInTheDocument();
userEvent.hover(refreshButton);
expect(await screen.findByText(/Fetched/)).toBeInTheDocument();
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Run the unit test suite for the file at
superset-frontend/src/dashboard/components/SliceHeaderControls/SliceHeaderControls.test.tsx.
2. The test defined at line 620 (test 'Should show single fetched query
tooltip with
timestamp') sets props and calls renderWrapper(props) (see lines 622-627)
then openMenu()
(openMenu is defined at lines 121-123 and clicks the "More Options" button).
3. At line 630 the test locates the "Force refresh" button via
screen.getByText('Force
refresh') and immediately inspects refreshButton.closest('div') for the
tooltip text (line
633).
4. In the running component the per-query tooltip is shown on hover and
rendered
asynchronously (the test never triggers hover nor awaits rendered tooltip),
so the
assertion at line 634 can read the DOM before the tooltip exists — reproduce
by running
only this test (`jest
superset-frontend/src/dashboard/components/SliceHeaderControls/SliceHeaderControls.test.tsx
-t "single fetched"`) and observe intermittent failure where /Fetched/ is
not found.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/src/dashboard/components/SliceHeaderControls/SliceHeaderControls.test.tsx
**Line:** 620:634
**Comment:**
*Race Condition: The test asserts tooltip content by reading a parent
div immediately after opening the menu, but tooltips are rendered on hover
(often in a portal) and appear asynchronously; the test never triggers hover or
awaits the tooltip, making it flaky or failing. Change the test to be async,
hover the refresh button, and await the tooltip via `findByText`.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
##########
superset-frontend/src/dashboard/components/SliceHeaderControls/SliceHeaderControls.test.tsx:
##########
@@ -616,3 +616,118 @@ test('Should pass formData to Share menu for embed code
feature', () => {
openMenu();
expect(screen.getByText('Share')).toBeInTheDocument();
});
+
+test('Should show single fetched query tooltip with timestamp', () => {
+ const updatedDttm = Date.parse('2024-01-28T10:00:00.000Z');
+ const props = createProps();
+ props.isCached = [false];
+ props.cachedDttm = [''];
+ props.updatedDttm = updatedDttm;
+
+ renderWrapper(props);
+ openMenu();
+
+ const refreshButton = screen.getByText('Force refresh');
+ expect(refreshButton).toBeInTheDocument();
+
+ const tooltipContainer = refreshButton.closest('div');
+ expect(tooltipContainer?.textContent).toMatch(/Fetched/);
+});
+
+test('Should show single cached query tooltip with timestamp', () => {
+ const cachedDttm = '2024-01-28T10:00:00.000Z';
+ const props = createProps();
+ props.isCached = [true];
+ props.cachedDttm = [cachedDttm];
+ props.updatedDttm = null;
+
+ renderWrapper(props);
+ openMenu();
+
+ const refreshButton = screen.getByText('Force refresh');
+ expect(refreshButton).toBeInTheDocument();
+
+ const tooltipContainer = refreshButton.closest('div');
+ expect(tooltipContainer?.textContent).toMatch(/Cached/);
+});
+
+test('Should show multiple per-query tooltips when all queries are fetched',
() => {
+ const cachedDttm1 = '';
+ const cachedDttm2 = '';
+ const updatedDttm = Date.parse('2024-01-28T10:10:00.000Z');
+ const props = createProps(VizType.Table);
+ props.isCached = [false, false];
+ props.cachedDttm = [cachedDttm1, cachedDttm2];
+ props.updatedDttm = updatedDttm;
+
+ renderWrapper(props);
+ openMenu();
+
+ const refreshButton = screen.getByText('Force refresh');
+ expect(refreshButton).toBeInTheDocument();
+
+ const tooltipContainer = refreshButton.closest('div');
+ expect(tooltipContainer?.textContent).toMatch(/Fetched/);
Review Comment:
**Suggestion:** For multi-query "fetched" assertions the test reads a nearby
div synchronously instead of triggering hover and awaiting the tooltip; make
the test async, hover the refresh button, and await the tooltip content to
ensure the tooltip is visible before asserting. [race condition]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Flaky unit test for table chart tooltip behavior.
- ❌ CI Jest run may produce intermittent failures.
```
</details>
```suggestion
test('Should show multiple per-query tooltips when all queries are fetched',
async () => {
const cachedDttm1 = '';
const cachedDttm2 = '';
const updatedDttm = Date.parse('2024-01-28T10:10:00.000Z');
const props = createProps(VizType.Table);
props.isCached = [false, false];
props.cachedDttm = [cachedDttm1, cachedDttm2];
props.updatedDttm = updatedDttm;
renderWrapper(props);
openMenu();
const refreshButton = screen.getByText('Force refresh');
expect(refreshButton).toBeInTheDocument();
userEvent.hover(refreshButton);
expect(await screen.findByText(/Fetched/)).toBeInTheDocument();
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Execute the test at line 654 in
superset-frontend/src/dashboard/components/SliceHeaderControls/SliceHeaderControls.test.tsx
which prepares two fetched queries (lines 656-661) and renders the component
(line 663).
2. The test opens the menu with openMenu() (line 664) and selects the "Force
refresh"
button (line 666).
3. Immediately the test inspects refreshButton.closest('div') and asserts
/Fetched/ (lines
669-670) without hovering or awaiting the tooltip.
4. Because multi-query tooltips are shown on hover and may be rendered
asynchronously (or
via a portal), the synchronous DOM snapshot can miss tooltip content —
reproduce by
running the specific test (`jest -t "multiple per-query tooltips when all
queries are
fetched"`) and observing race failures where /Fetched/ is not present.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/src/dashboard/components/SliceHeaderControls/SliceHeaderControls.test.tsx
**Line:** 654:670
**Comment:**
*Race Condition: For multi-query "fetched" assertions the test reads a
nearby div synchronously instead of triggering hover and awaiting the tooltip;
make the test async, hover the refresh button, and await the tooltip content to
ensure the tooltip is visible before asserting.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
##########
superset-frontend/src/dashboard/components/SliceHeaderControls/SliceHeaderControls.test.tsx:
##########
@@ -616,3 +616,118 @@ test('Should pass formData to Share menu for embed code
feature', () => {
openMenu();
expect(screen.getByText('Share')).toBeInTheDocument();
});
+
+test('Should show single fetched query tooltip with timestamp', () => {
+ const updatedDttm = Date.parse('2024-01-28T10:00:00.000Z');
+ const props = createProps();
+ props.isCached = [false];
+ props.cachedDttm = [''];
+ props.updatedDttm = updatedDttm;
+
+ renderWrapper(props);
+ openMenu();
+
+ const refreshButton = screen.getByText('Force refresh');
+ expect(refreshButton).toBeInTheDocument();
+
+ const tooltipContainer = refreshButton.closest('div');
+ expect(tooltipContainer?.textContent).toMatch(/Fetched/);
+});
+
+test('Should show single cached query tooltip with timestamp', () => {
+ const cachedDttm = '2024-01-28T10:00:00.000Z';
+ const props = createProps();
+ props.isCached = [true];
+ props.cachedDttm = [cachedDttm];
+ props.updatedDttm = null;
+
+ renderWrapper(props);
+ openMenu();
+
+ const refreshButton = screen.getByText('Force refresh');
+ expect(refreshButton).toBeInTheDocument();
+
+ const tooltipContainer = refreshButton.closest('div');
+ expect(tooltipContainer?.textContent).toMatch(/Cached/);
+});
+
+test('Should show multiple per-query tooltips when all queries are fetched',
() => {
+ const cachedDttm1 = '';
+ const cachedDttm2 = '';
+ const updatedDttm = Date.parse('2024-01-28T10:10:00.000Z');
+ const props = createProps(VizType.Table);
+ props.isCached = [false, false];
+ props.cachedDttm = [cachedDttm1, cachedDttm2];
+ props.updatedDttm = updatedDttm;
+
+ renderWrapper(props);
+ openMenu();
+
+ const refreshButton = screen.getByText('Force refresh');
+ expect(refreshButton).toBeInTheDocument();
+
+ const tooltipContainer = refreshButton.closest('div');
+ expect(tooltipContainer?.textContent).toMatch(/Fetched/);
+});
+
+test('Should show multiple per-query tooltips when all queries are cached', ()
=> {
+ const cachedDttm1 = '2025-01-28T10:00:00.000Z';
+ const cachedDttm2 = '2024-01-28T10:05:00.000Z';
+ const props = createProps(VizType.Table);
+ props.isCached = [true, true];
+ props.cachedDttm = [cachedDttm1, cachedDttm2];
+ props.updatedDttm = null;
+
+ renderWrapper(props);
+ openMenu();
+
+ const refreshButton = screen.getByText('Force refresh');
+ expect(refreshButton).toBeInTheDocument();
+
+ const tooltipContainer = refreshButton.closest('div');
+ expect(tooltipContainer?.textContent).toMatch(/Query 1:/);
+ expect(tooltipContainer?.textContent).toMatch(/Query 2:/);
+ expect(tooltipContainer?.textContent).toMatch(/Cached/);
Review Comment:
**Suggestion:** The test asserts presence of per-query labels and "Cached"
by reading a parent div immediately; since the tooltip is shown on hover and
rendered asynchronously (possibly in a portal), update the test to be async,
hover the refresh button, and await the specific tooltip items using
`findByText`. [race condition]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Flaky unit test for per-query cached tooltips.
- ❌ CI Jest run may fail intermittently for dashboard tests.
```
</details>
```suggestion
test('Should show multiple per-query tooltips when all queries are cached',
async () => {
const cachedDttm1 = '2025-01-28T10:00:00.000Z';
const cachedDttm2 = '2024-01-28T10:05:00.000Z';
const props = createProps(VizType.Table);
props.isCached = [true, true];
props.cachedDttm = [cachedDttm1, cachedDttm2];
props.updatedDttm = null;
renderWrapper(props);
openMenu();
const refreshButton = screen.getByText('Force refresh');
expect(refreshButton).toBeInTheDocument();
userEvent.hover(refreshButton);
expect(await screen.findByText(/Query 1:/)).toBeInTheDocument();
expect(await screen.findByText(/Query 2:/)).toBeInTheDocument();
expect(await screen.findByText(/Cached/)).toBeInTheDocument();
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Run the test defined at line 673 in
superset-frontend/src/dashboard/components/SliceHeaderControls/SliceHeaderControls.test.tsx
which sets two cached query timestamps (lines 674-679) and renders the
component (line
681).
2. The test opens the menu with openMenu() (line 682) and obtains the "Force
refresh"
button (line 684).
3. It then synchronously reads refreshButton.closest('div') and asserts that
per-query
labels "Query 1:", "Query 2:" and "Cached" exist (lines 687-690) without
hover/await.
4. Because tooltips are activated by hover and shown asynchronously (often
via portal
rendering), the synchronous check can run before the tooltip is created —
reproduce by
running this test alone and observing intermittent misses of Query labels or
Cached text.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/src/dashboard/components/SliceHeaderControls/SliceHeaderControls.test.tsx
**Line:** 673:690
**Comment:**
*Race Condition: The test asserts presence of per-query labels and
"Cached" by reading a parent div immediately; since the tooltip is shown on
hover and rendered asynchronously (possibly in a portal), update the test to be
async, hover the refresh button, and await the specific tooltip items using
`findByText`.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
##########
superset-frontend/src/dashboard/components/SliceHeaderControls/index.tsx:
##########
@@ -325,16 +325,18 @@ const SliceHeaderControls = (
const updatedWhen = updatedDttm
? (extendedDayjs.utc(updatedDttm) as any).fromNow()
: '';
- const getCachedTitle = (itemCached: boolean) => {
+ const getCachedTitle = (itemCached: boolean, index: number) => {
if (itemCached) {
- return t('Cached %s', cachedWhen);
+ return t('Cached %s', cachedWhen[index]);
}
if (updatedWhen) {
return t('Fetched %s', updatedWhen);
}
return '';
};
- const refreshTooltipData = [...new Set(isCached.map(getCachedTitle) || '')];
+ const refreshTooltipData = [
+ ...new Set(isCached.map((itemCached, index) => getCachedTitle(itemCached,
index))),
+ ];
Review Comment:
**Suggestion:** Logic error: deduping titles with a Set and always
collapsing to unique titles breaks the mapping between tooltip entries and
original query indexes, so using `index + 1` later will show wrong query
numbers; only collapse to a single entry when all queries truly have the same
title to preserve per-query ordering/numbering. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Force refresh tooltip shows incorrect query numbers.
- ⚠️ Users misled about which query was cached.
- ⚠️ Affects SliceHeaderControls Force refresh menu UI.
```
</details>
```suggestion
const refreshTooltipData = (() => {
const titles = isCached.map((itemCached, index) =>
getCachedTitle(itemCached, index),
);
// If all queries have same cache time we can unit them to one,
otherwise preserve per-query titles
return new Set(titles).size === 1 ? [titles[0]] : titles;
})();
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Render the SliceHeaderControls component for a slice in the dashboard:
open file
superset-frontend/src/dashboard/components/SliceHeaderControls/index.tsx and
locate the
component SliceHeaderControls (component root). Confirm props include
isCached (boolean[])
and cachedDttm (string[]). See props destructuring at lines ~315-319.
2. Provide props where isCached has multiple entries with differing
per-index titles; e.g.
isCached = [true, false] and cachedDttm = ['2026-01-28T12:00:00Z',
'2026-01-28T12:05:00Z']
so getCachedTitle(index 0) !== getCachedTitle(index 1). getCachedTitle is
defined at lines
328-336 in the same file.
3. Open the chart menu (three-dot button) for that slice and hover the Force
refresh menu
item — the Force refresh label renders RefreshTooltip which uses
refreshTooltipData. The
code building refreshTooltipData is at lines 337-339 and the tooltip mapping
is at lines
341-346.
4. Observe that because the implementation unconditionally dedupes via new
Set(...), the
resulting refreshTooltipData loses per-query ordering. The subsequent
tooltip mapping uses
the reduced array indices to generate "Query %s: %s" labels (line ~341),
which produces
incorrect query numbers (e.g., "Query 1: ..." shown for the wrong original
query). This
reproduces the logic error described.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/src/dashboard/components/SliceHeaderControls/index.tsx
**Line:** 337:339
**Comment:**
*Logic Error: Logic error: deduping titles with a Set and always
collapsing to unique titles breaks the mapping between tooltip entries and
original query indexes, so using `index + 1` later will show wrong query
numbers; only collapse to a single entry when all queries truly have the same
title to preserve per-query ordering/numbering.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
##########
superset-frontend/src/dashboard/components/SliceHeaderControls/SliceHeaderControls.test.tsx:
##########
@@ -616,3 +616,118 @@ test('Should pass formData to Share menu for embed code
feature', () => {
openMenu();
expect(screen.getByText('Share')).toBeInTheDocument();
});
+
+test('Should show single fetched query tooltip with timestamp', () => {
+ const updatedDttm = Date.parse('2024-01-28T10:00:00.000Z');
+ const props = createProps();
+ props.isCached = [false];
+ props.cachedDttm = [''];
+ props.updatedDttm = updatedDttm;
+
+ renderWrapper(props);
+ openMenu();
+
+ const refreshButton = screen.getByText('Force refresh');
+ expect(refreshButton).toBeInTheDocument();
+
+ const tooltipContainer = refreshButton.closest('div');
+ expect(tooltipContainer?.textContent).toMatch(/Fetched/);
+});
+
+test('Should show single cached query tooltip with timestamp', () => {
+ const cachedDttm = '2024-01-28T10:00:00.000Z';
+ const props = createProps();
+ props.isCached = [true];
+ props.cachedDttm = [cachedDttm];
+ props.updatedDttm = null;
+
+ renderWrapper(props);
+ openMenu();
+
+ const refreshButton = screen.getByText('Force refresh');
+ expect(refreshButton).toBeInTheDocument();
+
+ const tooltipContainer = refreshButton.closest('div');
+ expect(tooltipContainer?.textContent).toMatch(/Cached/);
+});
+
+test('Should show multiple per-query tooltips when all queries are fetched',
() => {
+ const cachedDttm1 = '';
+ const cachedDttm2 = '';
+ const updatedDttm = Date.parse('2024-01-28T10:10:00.000Z');
+ const props = createProps(VizType.Table);
+ props.isCached = [false, false];
+ props.cachedDttm = [cachedDttm1, cachedDttm2];
+ props.updatedDttm = updatedDttm;
+
+ renderWrapper(props);
+ openMenu();
+
+ const refreshButton = screen.getByText('Force refresh');
+ expect(refreshButton).toBeInTheDocument();
+
+ const tooltipContainer = refreshButton.closest('div');
+ expect(tooltipContainer?.textContent).toMatch(/Fetched/);
+});
+
+test('Should show multiple per-query tooltips when all queries are cached', ()
=> {
+ const cachedDttm1 = '2025-01-28T10:00:00.000Z';
+ const cachedDttm2 = '2024-01-28T10:05:00.000Z';
+ const props = createProps(VizType.Table);
+ props.isCached = [true, true];
+ props.cachedDttm = [cachedDttm1, cachedDttm2];
+ props.updatedDttm = null;
+
+ renderWrapper(props);
+ openMenu();
+
+ const refreshButton = screen.getByText('Force refresh');
+ expect(refreshButton).toBeInTheDocument();
+
+ const tooltipContainer = refreshButton.closest('div');
+ expect(tooltipContainer?.textContent).toMatch(/Query 1:/);
+ expect(tooltipContainer?.textContent).toMatch(/Query 2:/);
+ expect(tooltipContainer?.textContent).toMatch(/Cached/);
+});
+
+
+test('Should deduplicate identical cache times in tooltip', () => {
+ const sameCachedDttm = '2024-01-28T10:00:00.000Z';
+ const props = createProps(VizType.Table);
+ props.isCached = [true, true];
+ props.cachedDttm = [sameCachedDttm, sameCachedDttm];
+ props.updatedDttm = null;
+
+ renderWrapper(props);
+ openMenu();
+
+ const refreshButton = screen.getByText('Force refresh');
+ expect(refreshButton).toBeInTheDocument();
+
+ const tooltipContainer = refreshButton.closest('div');
+ const tooltipText = tooltipContainer?.textContent || '';
Review Comment:
**Suggestion:** The deduplication test reads a parent div synchronously to
assert the absence of "Query 1:"; tooltips are asynchronous and triggered by
hover—update test to be async, hover the refresh button, await the tooltip, and
then assert the absence to avoid flakiness. [race condition]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Flaky deduplication unit test.
- ❌ CI Jest runs for dashboard tests affected.
```
</details>
```suggestion
test('Should deduplicate identical cache times in tooltip', async () => {
const sameCachedDttm = '2024-01-28T10:00:00.000Z';
const props = createProps(VizType.Table);
props.isCached = [true, true];
props.cachedDttm = [sameCachedDttm, sameCachedDttm];
props.updatedDttm = null;
renderWrapper(props);
openMenu();
const refreshButton = screen.getByText('Force refresh');
expect(refreshButton).toBeInTheDocument();
userEvent.hover(refreshButton);
const cachedNode = await screen.findByText(/Cached/);
const tooltipText = cachedNode.textContent || '';
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Execute the deduplication test at line 694 in
superset-frontend/src/dashboard/components/SliceHeaderControls/SliceHeaderControls.test.tsx
which configures identical cached timestamps (lines 695-700) and renders the
component
(line 701).
2. The test opens the menu (line 702) and grabs "Force refresh" (line 704)
then
immediately inspects the parent div for tooltip content (lines 706-709)
without hover or
awaiting.
3. Because tooltip content is produced on hover and may be mounted
asynchronously/in a
portal, the synchronous check can miss the tooltip and incorrectly fail;
reproduce by
running only this test (`jest -t "deduplicate identical cache times"`) and
observing
intermittent absence of expected Cached text.
4. Note: changing the test to hover the refresh button and await the tooltip
(as proposed)
stabilizes the assertion by ensuring tooltip is visible before reading.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/src/dashboard/components/SliceHeaderControls/SliceHeaderControls.test.tsx
**Line:** 694:708
**Comment:**
*Race Condition: The deduplication test reads a parent div
synchronously to assert the absence of "Query 1:"; tooltips are asynchronous
and triggered by hover—update test to be async, hover the refresh button, await
the tooltip, and then assert the absence to avoid flakiness.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
--
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]
