Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

fix: dashboard unit tests#8111

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
ktx-vaidehi merged 146 commits intomainfromdashboard-unit-tests
Sep 1, 2025
Merged

fix: dashboard unit tests#8111

ktx-vaidehi merged 146 commits intomainfromdashboard-unit-tests
Sep 1, 2025

Conversation

@ktx-vaidehi
Copy link
Collaborator

@ktx-vaidehiktx-vaidehi commentedAug 22, 2025
edited by github-actionsbot
Loading

PR Type

Tests, Enhancement


Description

  • Add extensive dashboard utility unit tests

  • Introduce Sankey builder component tests

  • Export helper for PromQL series props

  • Temporarily skip flaky UI tests


Diagram Walkthrough

flowchart LR  utils["Dashboard utils tests"] -- add --> convertDataIntoUnitValue.spec  component["Sankey builder tests"] -- add --> DashboardSankeyChartBuilder.spec  promql["PromQL utils"] -- export helper --> getPropsByChartTypeForSeries  ui_tests["UI specs"] -- skip flaky --> MenuLink.spec & AppSessions.spec
Loading

File Walkthrough

Relevant files
Tests
19 files
convertDataIntoUnitValue.spec.ts
Expand coverage for formatting, colors, validation             
+1869/-1
DashboardSankeyChartBuilder.spec.ts
Add full test suite for Sankey builder                                     
+1279/-0
MenuLink.spec.ts
Skip unstable external-link test case                                       
+1/-1     
AppSessions.spec.ts
Skip flaky mount-time data fetch test                                       
+1/-1     
convertSQLData.spec.ts
Add SQL data conversion unit tests                                             
+8632/-0
useDashboardPanel.spec.ts
Add unit tests for dashboard panel composable                       
+4723/-0
AddPanel.spec.ts
Add tests for add panel flow                                                         
+3540/-0
convertPromQLData.spec.ts
Add unit tests for PromQL conversion                                         
+2086/-0
DashboardQueryBuilder.spec.ts
Add tests for dashboard query builder                                       
+1170/-0
VariableQueryValueSelector.spec.ts
Add tests for variable query selector                                       
+1128/-0
VariablesValueSelector.spec.ts
Add tests for variables value selector                                     
+1137/-0
DashboardMapsQueryBuilder.spec.ts
Add tests for maps query builder                                                 
+1112/-0
viewDashboard.spec.ts
Add tests for dashboard view flow                                               
+1175/-0
DashboardGeoMapsQueryBuilder.spec.ts
Add tests for geomap query builder                                             
+1079/-0
FieldList.spec.ts
Add tests for field list interactions                                       
+971/-0 
DashboardErrors.spec.ts
Add tests for dashboard errors display                                     
+898/-0 
CustomMarkdownEditor.spec.ts
Add tests for custom markdown editor                                         
+916/-0 
convertGeoMapData.spec.ts
Add unit tests for geomap conversion                                         
+1061/-0
TabsSettings.spec.ts
Add tests for tabs settings component                                       
+820/-0 
Enhancement
1 files
convertPromQLData.ts
Export chart-type series props helper                                       
+1/-1     
Additional files
83 files
DateTimePickerDashboard.spec.ts+498/-0 
AddDashboard.spec.ts+338/-0 
AddFolder.spec.ts+690/-0 
ExportDashboard.spec.ts+409/-0 
MoveDashboardToAnotherFolder.spec.ts+643/-0 
OverrideConfigPopup.spec.ts+549/-0 
PanelContainer.spec.ts+497/-0 
PanelSchemaRenderer.spec.ts+827/-0 
QueryInspector.spec.ts+679/-0 
SelectDashboardDropdown.spec.ts+717/-0 
SelectFolderDropdown.spec.ts+658/-0 
SelectTabDropdown.spec.ts+780/-0 
AddAnnotation.spec.ts+242/-0 
BackGroundColorConfig.spec.ts+651/-0 
ChartSelection.spec.ts+177/-0 
ColorBySeries.spec.ts+224/-0 
ColorBySeriesPopUp.spec.ts+224/-0 
ColorPaletteDropDown.spec.ts+757/-0 
CommonAutoComplete.spec.ts+588/-0 
ConfigPanel.spec.ts+617/-0 
CustomChartEditor.spec.ts+693/-0 
CustomHTMLEditor.spec.ts+546/-0 
DashboardQueryEditor.spec.ts+507/-0 
Drilldown.spec.ts+605/-0 
DrilldownPopUp.spec.ts+723/-0 
DrilldownUserGuide.spec.ts+603/-0 
HistogramIntervalDropDown.spec.ts+368/-0 
MarkLineConfig.spec.ts+503/-0 
OverrideConfig.spec.ts+571/-0 
PanelSidebar.spec.ts+622/-0 
QueryTypeSelector.spec.ts+777/-0 
SortByBtnGrp.spec.ts+405/-0 
ValueMapping.spec.ts+424/-0 
ValueMappingPopUp.spec.ts+553/-0 
ChartRenderer.spec.ts+464/-0 
CustomChartRenderer.spec.ts+679/-0 
GeoMapRenderer.spec.ts+878/-0 
HTMLRenderer.spec.ts+492/-0 
MapsRenderer.spec.ts+490/-0 
MarkdownRenderer.spec.ts+600/-0 
TableRenderer.spec.ts+555/-0 
AddSettingVariable.spec.ts+661/-0 
GeneralSettings.spec.ts+767/-0 
SinglePanelMove.spec.ts+565/-0 
TabsDeletePopUp.spec.ts+611/-0 
VariableAdHocValueSelector.spec.ts+422/-0 
VariableCustomValueSelector.spec.ts+635/-0 
VariableSettings.spec.ts+696/-0 
VariablesDependenciesGraph.spec.ts+397/-0 
DashboardHeader.spec.ts+440/-0 
AddTab.spec.ts+767/-0 
TabList.spec.ts+604/-0 
ViewPanel.spec.ts+744/-0 
ApiDashboard.spec.ts+550/-0 
ErrorsDashboard.spec.ts+178/-0 
WebVitalsDashboard.spec.ts+547/-0 
VisualizeLogsQuery.spec.ts+674/-0 
TraceBlock.spec.ts+1/-1     
alertChartData.spec.ts+259/-0 
calculateGridForSubPlot.spec.ts+374/-0 
colorPatette.spec.ts+576/-0 
convertCustomChartData.spec.ts+771/-0 
convertDashboardSchemaVersion.spec.ts+537/-0 
convertPanelData.spec.ts+889/-0 
convertSankeyData.spec.ts+588/-0 
convertTableData.spec.ts+599/-0 
datetimeStartPoint.spec.ts+214/-0 
getAnnotationsData.spec.ts+351/-0 
useCustomDebouncer.spec.ts+307/-0 
variablesDependencyUtils.spec.ts+523/-0 
variablesUtils.spec.ts+533/-0 
sqlUtils.spec.ts+1283/-8
visualizationUtils.spec.ts+55/-0   
DashboardJsonEditor.spec.ts+551/-0 
DashboardSettings.spec.ts+640/-0 
Dashboards.spec.ts+813/-0 
ImportDashboard.spec.ts+827/-0 
PanelLayoutSettings.spec.ts+423/-0 
RenderDashboardCharts.spec.ts+700/-0 
ScheduledDashboards.spec.ts+519/-0 
AddCondition.spec.ts+425/-0 
DashboardFiltersOption.spec.ts+878/-0 
Group.spec.ts+718/-0 

@github-actions
Copy link
Contributor

github-actionsbot commentedAug 22, 2025
edited
Loading

PR Reviewer Guide 🔍

(Review updated until commit7c4e2c9)

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Fragile Date Mock

The custom global Date override in the "should handle exceptions gracefully" test can leak across tests and cause flaky behavior if not fully restored in all paths. Consider using vi.spyOn(global, 'Date') or scoping with try/finally to ensure restoration even on assertion failures.

it("should handle exceptions gracefully",()=>{// Mock Date constructor to throwconstoriginalDate=Date;global.Date=function(this:any, ...args:any[]){if(new.target){if(args[0]===null)thrownewError("Invalid date");returnneworiginalDate(...args);}returnoriginalDate(...args);}asany;Object.setPrototypeOf(global.Date,originalDate);constresult=convertOffsetToSeconds("1h",nullasany);expect(result.seconds).toBe(0);expect(result.periodAsStr).toBe("");global.Date=originalDate;});});
Skipped Test

A previously active assertion is now skipped; validate whether the flakiness is understood, and track re-enabling. If deterministic failure, consider refactoring to mock click/navigation rather than skipping.

it.skip("should not open external link when external is false",async()=>{constwindowOpen=vi.spyOn(window,"open");awaitwrapper.setProps({external:false});awaitwrapper.find('[data-test="menu-link-#-item"]').trigger("click");
Async Timing Reliance

Multiple tests rely on setTimeout-based waits to settle async DOM updates, which can be brittle. Prefer waiting on specific DOM state or promises (flushPromises) to avoid race conditions.

constwaitForComponent=async(wrapper:VueWrapper<any>)=>{awaitwrapper.vm.$nextTick();// Wait for getDashboard to complete and dashboard data to be populatedawaitnewPromise(resolve=>setTimeout(resolve,50));awaitwrapper.vm.$nextTick();// Additional wait for DOM elements to render from v-for loopawaitnewPromise(resolve=>setTimeout(resolve,10));};

@github-actions
Copy link
Contributor

github-actionsbot commentedAug 22, 2025
edited
Loading

PR Code Suggestions ✨

Latest suggestions up to7c4e2c9
Explore these optional code suggestions:

CategorySuggestion                                                                                                                                   Impact
Possible issue
Avoid clobbering global window

Reassigningglobal.window risks clobbering other test utilities and JSDOM
expectations. Patch only the needed property on an existingwindow if present, or
conditionally create it to avoid side effects across tests.

web/src/composables/useDashboardPanel.spec.ts [47-49]

 // Mock global objects that the composable might need-(global as any).window = { dispatchEvent: vi.fn() };-(global as any).parser = mockParser;+const g: any = globalThis as any;+g.window = g.window || {};+g.window.dispatchEvent = vi.fn();+g.parser = mockParser;
Suggestion importance[1-10]: 7

__

Why: Replacing the entireglobal.window can cause side effects; conditionally patching is safer and improves test isolation. The change is accurate and can prevent hard-to-debug issues across tests.

Medium
Use realistic debounce mock

The mockeddebounce runs synchronously, defeating debounce behavior and making
debounce-dependent logic untestable and potentially masking timing bugs. Replace it
with a timed mock that respects delay to better simulate real behavior and avoid
flaky assertions tied to immediate execution.

web/src/views/Dashboards/addPanel/AddPanel.spec.ts [81-84]

-vi.mock("lodash-es", () => ({-  debounce: vi.fn((fn) => fn),-  isEqual: vi.fn(),-}));+vi.mock("lodash-es", () => {+  return {+    debounce: vi.fn((fn, delay = 300) => {+      let timer: any;+      return (...args: any[]) => {+        clearTimeout(timer);+        timer = setTimeout(() => fn(...args), delay);+      };+    }),+    isEqual: vi.fn(),+  };+});
Suggestion importance[1-10]: 7

__

Why: Replacing the synchronousdebounce mock with a timed mock makes debounce-dependent logic testable and avoids masking timing-related issues; it's accurate to the shown code and improves test reliability.

Medium
Use safer deep clone mock

Replace thedeepCopy mock with a safer clone that preserves non-JSON values and
handles circular references. The current JSON-based clone will throw on circular
structures and strips functions, Dates, Maps, and undefined, causing brittle tests.

web/src/utils/dashboard/convertSQLData.spec.ts [74-76]

 vi.mock("@/utils/zincutils", () => ({-  deepCopy: vi.fn((obj) => JSON.parse(JSON.stringify(obj))),+  deepCopy: vi.fn((obj) => {+    const seen = new WeakMap();+    const clone = (val: any): any => {+      if (val === null || typeof val !== "object") return val;+      if (seen.has(val)) return seen.get(val);+      if (val instanceof Date) return new Date(val.getTime());+      if (Array.isArray(val)) {+        const arr: any[] = [];+        seen.set(val, arr);+        for (const item of val) arr.push(clone(item));+        return arr;+      }+      const out: Record<string, any> = {};+      seen.set(val, out);+      for (const k of Object.keys(val)) out[k] = clone((val as any)[k]);+      return out;+    };+    return clone(obj);+  }), }));
Suggestion importance[1-10]: 5

__

Why: The concern about JSON-based deep cloning in tests is valid for circular refs and non-JSON types, and the improved mock is reasonable. However, given this is a spec file and current tests likely don't rely on complex structures, the impact is moderate rather than critical.

Low
General
Make large dataset test deterministic

RemoveMath.random() to keep tests deterministic and avoid flaky assertions. Use a
seeded generator or fixed values so snapshots and logic relying on ordering or
ranges remain stable.

web/src/utils/dashboard/convertSQLData.spec.ts [841-863]

 it("should handle very large datasets", async () => {+  const deterministic = (i: number) => (i * 37) % 100; // simple deterministic pattern   const largeData = Array.from({ length: 1000 }, (_, i) => ({-    timestamp: `2023-01-${String(i + 1).padStart(2, '0')}`,-    value: Math.random() * 100+    timestamp: `2023-01-${String((i % 28) + 1).padStart(2, "0")}`,+    value: deterministic(i)   }));   const searchData = [largeData];   const result = await convertSQLData(     mockPanelSchema,     searchData,     mockStore,     mockChartPanelRef,     mockHoveredSeriesState,     mockResultMetaData,     mockMetadata,     mockChartPanelStyle,     mockAnnotations   );   expect(result.options).toBeDefined();   expect(result.options.series).toBeDefined(); });
Suggestion importance[1-10]: 7

__

Why: EliminatingMath.random() in tests improves determinism and reduces flakiness, directly enhancing test reliability. The improved code reflects the intended change and maps to the given test block accurately.

Medium
Make date formatter mock realistic

Align theformat mock behavior withdate-fns/format signature by accepting
additional options and returning a value derived from input date. Returning static
strings can mask bugs related to different inputs and formats.

web/src/utils/dashboard/convertSQLData.spec.ts [31-36]

 vi.mock("date-fns", () => ({-  format: vi.fn((date, formatStr) => {-    if (formatStr === "yyyy-MM-dd HH:mm:ss") return "2024-01-01 12:00:00";-    return "2024-01-01";+  format: vi.fn((date: Date | number, formatStr: string) => {+    const d = typeof date === "number" ? new Date(date) : date;+    // Minimal deterministic formatter for tests+    const pad = (n: number) => String(n).padStart(2, "0");+    const yyyy = d.getFullYear();+    const MM = pad(d.getMonth() + 1);+    const dd = pad(d.getDate());+    const HH = pad(d.getHours());+    const mm = pad(d.getMinutes());+    const ss = pad(d.getSeconds());+    if (formatStr === "yyyy-MM-dd HH:mm:ss") return `${yyyy}-${MM}-${dd} ${HH}:${mm}:${ss}`;+    if (formatStr === "yyyy-MM-dd") return `${yyyy}-${MM}-${dd}`;+    return `${yyyy}-${MM}-${dd}`; // fallback   }), }));
Suggestion importance[1-10]: 6

__

Why: Making theformat mock derive output from the input reduces the risk of masking bugs and improves test fidelity. It’s accurate to the shown code and beneficial, but not critical to correctness of the PR.

Low
Make abort assertion robust

Hard-coding the error message can make the test flaky across environments. Assert
rejection without relying on the exact message or check a stable property likename
=== 'AbortError' if your implementation sets it.

web/src/composables/useDashboardPanel.spec.ts [597-605]

 it("should handle abort signals", async () => {   const abortController = new AbortController();   abortController.abort();    await expect(     panel.getResultSchema("SELECT * FROM logs", abortController.signal),-  ).rejects.toThrow("Aborted");+  ).rejects.toBeTruthy(); });
Suggestion importance[1-10]: 6

__

Why: Avoiding an exact error message assertion improves test stability across environments. This is accurate and beneficial, though it slightly weakens specificity of the check.

Low
Remove conflicting router mocks

The tests manually pass$route/$router while also mockinguseRoute/useRouter,
creating dual sources of truth and brittle behavior. Rely on one approach: remove
$route/$router fromglobal.mocks and use the composable mocks consistently to avoid
mismatches and hidden failures.

web/src/views/Dashboards/addPanel/AddPanel.spec.ts [87-103]

-// Mock vue-router hooks-vi.mock("vue-router", async () => {-  const actual = await vi.importActual("vue-router");-  return {-    ...actual,-    onBeforeRouteLeave: vi.fn(),-    useRoute: vi.fn(() => ({-      query: {-        dashboard: "test-dashboard",-      },-      params: {},-    })),-    useRouter: vi.fn(() => ({-      push: vi.fn(),-      replace: vi.fn(),-    })),-  };+// When mounting, do not pass global.mocks.$route/$router.+wrapper = shallowMount(AddPanel, {+  global: {+    plugins: [store, router, i18n],+    // Remove mocks: use composable mocks above+    stubs: {+      'q-input': true,+      'q-btn': true,+      'q-splitter': true,+      'q-splitter-panel': true,+      'ChartSelection': true,+      'FieldList': true,+      'DashboardQueryBuilder': true,+      'DateTimePickerDashboard': true,+      'DashboardErrorsComponent': true,+      'PanelSidebar': true,+      'ConfigPanel': true,+      'VariablesValueSelector': true,+      'PanelSchemaRenderer': true,+      'RelativeTime': true,+      'DashboardQueryEditor': true,+      'QueryInspector': true,+      'CustomHTMLEditor': true,+      'CustomMarkdownEditor': true,+      'CustomChartEditor': true,+    },+  },+  props: { metaData: null }, });
Suggestion importance[1-10]: 6

__

Why: The test currently mixes composable router mocks with$route/$router passed viaglobal.mocks, which can cause inconsistencies; consolidating on one approach improves stability though it's a test-structure preference rather than a critical bug.

Low
Use memory history for tests

UsingcreateWebHistory in unit tests touches browser history and can cause
navigation and timing flakiness. Switch tocreateMemoryHistory for isolated,
deterministic router behavior in tests.

web/src/views/Dashboards/addPanel/AddPanel.spec.ts [130-141]

-// Create mock router+import { createRouter, createMemoryHistory } from "vue-router";+ const createMockRouter = () => {   return createRouter({-    history: createWebHistory(),+    history: createMemoryHistory(),     routes: [       {         path: "/dashboard",         name: "dashboard",         component: { template: "<div>Dashboard</div>" },       },     ],   }); };
Suggestion importance[1-10]: 6

__

Why: Switching fromcreateWebHistory tocreateMemoryHistory in tests avoids touching real browser history and reduces flakiness; it’s a standard testing improvement and directly aligns with the existing router factory code.

Low
Simplify mocked service response

Avoid asserting transport-layer response fields for mocked services when your
production code only readsdata. Keeping extra fields encourages brittle tests and
type drift. Mock only the shape actually consumed by the code under test.

web/src/composables/useDashboardPanel.spec.ts [115-126]

 vi.mocked(queryService.result_schema).mockResolvedValue({   data: {     group_by: ["timestamp"],     projections: ["timestamp", "count"],     timeseries_field: "timestamp",   },-  status: 0,-  statusText: "",-  headers: undefined,-  config: undefined-});+} as any);
Suggestion importance[1-10]: 5

__

Why: The existing mock includes transport-layer fields that the code under test likely doesn't use; simplifying reduces brittleness. Impact is moderate and correct, but largely a test cleanup rather than a functional fix.

Low

Previous suggestions

Suggestions up to commit3f31308
CategorySuggestion                                                                                                                                   Impact
Possible issue
Fix faulty resize observer spy

The spy is placed on a possibly empty object, so it won't observe the actual
disconnect call and the conditional makes the assertion a no-op. Spy on the instance
created by ResizeObserver or inject a mock with a stable reference, then assert
disconnect was called unconditionally.

web/src/components/dashboards/PanelContainer.spec.ts [362-369]

 it("should cleanup resize observer on unmount", () => {+  const disconnect = vi.fn();+  // Provide a stable mock so component uses this instance+  (window as any).ResizeObserver = vi.fn().mockImplementation(() => ({+    observe: vi.fn(),+    unobserve: vi.fn(),+    disconnect+  }));+   wrapper = createWrapper();-  const disconnectSpy = vi.spyOn(wrapper.vm.resizeObserver || {}, 'disconnect');-   wrapper.unmount();--  if (disconnectSpy.getMockImplementation()) {-    expect(disconnectSpy).toHaveBeenCalled();-  }++  expect(disconnect).toHaveBeenCalled(); });
Suggestion importance[1-10]: 8

__

Why: Spying onwrapper.vm.resizeObserver || {} may not hook the actual instance, making the assertion effectively a no-op; injecting a stable mock and assertingdisconnect was called is correct and directly improves test validity. This addresses a concrete flaw in the test setup.

Medium
Replace unsupported selector usage

The selector"button:contains('Save')" is not supported by Vue Test Utils and will
never match, leading to false positives. Query by stable data-test attributes or
component names instead to ensure the test actually inspects the target button.

web/src/components/dashboards/addPanel/AddAnnotation.spec.ts [115-121]

 it("should disable save button when title is empty", async () => {-  const saveButton = wrapper.find("button:contains('Save')");+  const saveButton = wrapper.find("[data-test='add-annotation-save']");+  expect(saveButton.exists()).toBe(true);   expect(saveButton.attributes("disabled")).toBeTruthy();-  await wrapper.find("input[label='Title *']").setValue("Test Title");+  await wrapper.find("[data-test='add-annotation-title']").setValue("Test Title");+  await flushPromises();   expect(saveButton.attributes("disabled")).toBeFalsy(); });
Suggestion importance[1-10]: 8

__

Why:button:contains('Save') is not a supported selector in Vue Test Utils, leading to unreliable tests; recommending data-test selectors is accurate and materially improves test robustness, though added attributes must exist.

Medium
Make propagation test deterministic

The test adds its own DOM click listener and then uses Vue Test Utils' trigger,
which doesn't verify event propagation reliably across Vue handlers. Assert
stopPropagation by emitting and inspecting wrapper-emitted events or by spying on
the component's handler. Use trigger with modifiers or simulate a native event and
check that the tab click handler wasn't invoked.

web/src/components/dashboards/addPanel/DashboardQueryEditor.spec.ts [271-299]

 it("should prevent event propagation when clicking remove button", async () => {-  ...+  const multiQueryPanelData = {+    ...mockDashboardPanelData,+    data: {+      ...mockDashboardPanelData.data,+      queries: [+        { query: "SELECT * FROM stream1", queryType: "sql" },+        { query: "SELECT * FROM stream2", queryType: "sql" }+      ]+    }+  };++  wrapper = createWrapper({+    dashboardPanelData: multiQueryPanelData,+    promqlMode: true+  });++  // Spy on tab select handler via emitted event+  const tab = wrapper.find('[data-test="dashboard-panel-query-tab-1"]');   const removeBtn = wrapper.find('[data-test="dashboard-panel-query-tab-remove-1"]');-  const clickSpy = vi.fn();-  removeBtn.element.addEventListener('click', clickSpy);-++  // Click remove; should not emit tab-select/switch nor change index   await removeBtn.trigger('click');-  // Tab should not be switched when remove button is clicked-  expect(wrapper.vm.dashboardPanelData.layout.currentQueryIndex).not.toBe(1);+  expect(wrapper.emitted('tab-selected')).toBeFalsy();+  expect(wrapper.vm.dashboardPanelData.layout.currentQueryIndex).toBe(0); });
Suggestion importance[1-10]: 7

__

Why: The current test attaches a native listener and conditionally infers stopPropagation, which is brittle; verifying emitted events and index is clearer and more reliable. The proposed change improves test robustness without altering runtime code, but it's not a critical bug fix.

Medium
Use valid unmount cleanup assertions

Spying on$destroy is invalid for Vue 3 components and will always fail silently or
be meaningless. Assert side-effects of unmount instead, e.g., timers cleared,
listeners removed, or reactive flags reset. Replace the spy with concrete checks on
component state or usevi.useFakeTimers() to verify cleared intervals.

web/src/views/Dashboards/viewDashboard.spec.ts [421-429]

-it("should clean up resources on unmount", () => {+it("should clean up resources on unmount", async () => {+  vi.useFakeTimers();   wrapper = createWrapper();-  const cleanupSpy = vi.spyOn(wrapper.vm, '$destroy');-+  // simulate an interval the component would set+  wrapper.vm._testInterval = setInterval(() => {}, 1000);   wrapper.unmount();--  expect(cleanupSpy).toHaveBeenCalled();+  // Advance timers to ensure no pending intervals run+  vi.runOnlyPendingTimers();+  // Assert interval cleared+  expect(wrapper.vm?._testInterval).toBeUndefined();+  vi.useRealTimers(); });
Suggestion importance[1-10]: 7

__

Why: Spying onwrapper.vm.$destroy is incorrect in Vue 3 tests, so replacing it with assertions about actual unmount side effects improves test correctness; however, the proposed mock interval_testInterval is hypothetical and may not reflect real component behavior.

Medium
Guard nested property access

AccessingshareLink.execute directly risks throwing ifshareLink is null or not yet
initialized. Guard the access by checking for the object first to avoid runtime
errors and flaky tests.

web/src/test/integrations/dashboard-workflows.spec.ts [458]

-expect(wrapper.vm.shareLink.execute).toBeDefined();+expect(wrapper.vm.shareLink && wrapper.vm.shareLink.execute).toBeDefined();
Suggestion importance[1-10]: 5

__

Why: The guard avoids a potential runtime error ifshareLink is undefined, improving test robustness; however, the test likely expects initialization before assertion, so impact is moderate and context-dependent.

Low
General
Stabilize throttling test with fake timers

Relying on real timers makes throttling assertions flaky. Use fake timers to
deterministically advance time and verify throttling behavior without race
conditions.

web/src/components/dashboards/panels/ChartRenderer.spec.ts [346-355]

-// Multiple rapid resize events+vi.useFakeTimers(); for (let i = 0; i < 10; i++) {   window.dispatchEvent(new Event('resize')); } await flushPromises();+vi.advanceTimersByTime(200); // advance beyond throttle delay+await flushPromises();+expect(mockChart.resize).toHaveBeenCalledTimes(1);+vi.useRealTimers();-// Should be throttled-expect(mockChart.resize).toHaveBeenCalledTimes(1);-
Suggestion importance[1-10]: 8

__

Why: Throttling behavior tied to timers can be flaky; switching to fake timers makes the test deterministic and reliable, a solid improvement to test correctness.

Medium
Use visibility assertion API

Assertingv-show via attributes is unreliable because Vue removes it at runtime;
this can cause false negatives. Instead, assert visibility via the wrapper’s
isVisible() API or by checking style/display conditions.

web/src/components/dashboards/settings/GeneralSettings.spec.ts [482-486]

-const dateTimePicker = wrapper.findComponent(-  '[data-test="datetime-picker"]',-);-expect(dateTimePicker.attributes("v-show")).toBe("false");+const dateTimePicker = wrapper.findComponent('[data-test="datetime-picker"]');+expect(dateTimePicker.exists()).toBe(true);+expect(dateTimePicker.isVisible()).toBe(false);
Suggestion importance[1-10]: 7

__

Why: Assertingv-show via attributes is unreliable in Vue; usingisVisible() is more accurate and prevents flaky tests, and the improved code reflects the intended check correctly.

Medium
Use findComponent for props access

Accessing props on a plain DOM wrapper can throw if the selector hits a non-Vue
node. Use findComponent to target the component or guard for existence before
reading props to prevent false negatives and runtime errors in case of slot/stub
changes.

web/src/components/dashboards/addPanel/FieldList.spec.ts [194-200]

 it("should bind stream options to filtered streams", () => {   wrapper = createWrapper();-  const streamDropdown = wrapper.find('[data-test="index-dropdown-stream"]');+  const streamDropdown = wrapper.findComponent('[data-test="index-dropdown-stream"]');+  expect(streamDropdown.exists()).toBe(true);   expect(streamDropdown.props('options')).toBeDefined(); });
Suggestion importance[1-10]: 6

__

Why: Accessingprops() on a DOM wrapper can fail; switching tofindComponent makes the test more robust to template changes. This is a reasonable improvement but moderate in impact.

Low
Await render before asserting class

This test asserts a CSS class without awaiting mount lifecycle; if the class is set
conditionally after mount, it can be flaky. Await render completion
(flushPromises/nextTick) before asserting to prevent intermittent failures.

web/src/views/Dashboards/viewDashboard.spec.ts [183-188]

 it("should handle theme changes", async () => {   store.state.theme = "dark";   wrapper = createWrapper();+  await flushPromises();+  await wrapper.vm.$nextTick();    expect(wrapper.find('.dark-mode').exists()).toBe(true); });
Suggestion importance[1-10]: 6

__

Why: AddingflushPromises/$nextTick before asserting the.dark-mode class reduces flakiness and is contextually correct, but the impact is moderate since many mounts are already awaited elsewhere.

Low

@ktx-vaidehiktx-vaidehiforce-pushed thedashboard-unit-tests branch 2 times, most recently from23d404a toa1d857cCompareAugust 25, 2025 05:13
@ktx-abhayktx-abhayforce-pushed thedashboard-unit-tests branch 2 times, most recently fromd3b42e0 to6e2bfa1CompareAugust 25, 2025 07:42
@ktx-vaidehiktx-vaidehi changed the titleDashboard unit testsfix: dashboard unit testsAug 25, 2025
@github-actionsgithub-actionsbot added ☢️ BugSomething isn't working and removed Invalid PR Title labelsAug 25, 2025
@ktx-vaidehiktx-vaidehiforce-pushed thedashboard-unit-tests branch 2 times, most recently from16612fb tocefaa40CompareAugust 26, 2025 04:44
@ktx-vaidehiktx-vaidehiforce-pushed thedashboard-unit-tests branch 3 times, most recently fromfb7dc6d to9b14ca4CompareAugust 26, 2025 11:26
@ktx-vaidehiktx-vaidehiforce-pushed thedashboard-unit-tests branch 4 times, most recently from52f8142 to7c4e2c9CompareAugust 28, 2025 10:28
@ktx-vaidehiktx-vaidehi marked this pull request as ready for reviewAugust 28, 2025 10:30
@github-actions
Copy link
Contributor

Persistent review updated to latest commit7c4e2c9

ktx-vaidehiand others added27 commitsSeptember 1, 2025 16:21
@ktx-vaidehiktx-vaidehi merged commit46fa42a intomainSep 1, 2025
28 of 30 checks passed
@ktx-vaidehiktx-vaidehi deleted the dashboard-unit-tests branchSeptember 1, 2025 11:48
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@ktx-kirtanktx-kirtanktx-kirtan approved these changes

Assignees

No one assigned

Labels

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@ktx-vaidehi@ktx-kirtan@ktx-abhay@Shrinath-O2

[8]ページ先頭

©2009-2025 Movatter.jp