PR Code Suggestions ✨Latest suggestions up to7c4e2c9 Explore these optional code suggestions: | Category | Suggestion | 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 suggestionsSuggestions up to commit3f31308
| Category | Suggestion | 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 |
|
Uh oh!
There was an error while loading.Please reload this page.
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
File Walkthrough
19 files
Expand coverage for formatting, colors, validationAdd full test suite for Sankey builderSkip unstable external-link test caseSkip flaky mount-time data fetch testAdd SQL data conversion unit testsAdd unit tests for dashboard panel composableAdd tests for add panel flowAdd unit tests for PromQL conversionAdd tests for dashboard query builderAdd tests for variable query selectorAdd tests for variables value selectorAdd tests for maps query builderAdd tests for dashboard view flowAdd tests for geomap query builderAdd tests for field list interactionsAdd tests for dashboard errors displayAdd tests for custom markdown editorAdd unit tests for geomap conversionAdd tests for tabs settings component1 files
Export chart-type series props helper83 files