fix: dashboard unit tests #8239

ktx-vaidehi · 2025-09-01T12:54:47Z

PR Type

Tests, Enhancement

Description

Add comprehensive unit tests for data loaders
Add WebSocket/streaming values tests
Add annotations data workflow tests
Improve loader mocks and error handling

Diagram Walkthrough

flowchart LR
  specs["Add dashboard composables specs"] -- "usePanelDataLoader" --> panelLoader["Panel data loading tests"]
  specs -- "useValuesWebSocket" --> valuesWS["Field values WS/stream tests"]
  specs -- "useAnnotationsData" --> annotations["Annotations data workflow tests"]

File Walkthrough

Relevant files

Tests

usePanelDataLoader.spec.ts `Comprehensive tests for panel data loader` web/src/composables/dashboard/usePanelDataLoader.spec.ts Add extensive tests for loadData flow Mock services, WS, streaming, cache, annotations Cover variables, partitions, PromQL, histograms Validate error, abort, and cache behaviors	+2666/-0
useValuesWebSocket.spec.ts `Tests for values WebSocket and fetching paths` web/src/composables/dashboard/useValuesWebSocket.spec.ts Test trace ID management and handlers Verify WS/stream/REST field values paths Validate response merging and meta updates Cover error and edge-case handling	+719/-0
useAnnotationsData.spec.ts `Unit tests for annotations data composable` web/src/composables/dashboard/useAnnotationsData.spec.ts Test add/edit annotation UI state flows Verify notifications and watchers Test dashboard panel extraction logic Handle error and edge scenarios	+582/-0
usePanelCache.spec.ts `Tests for panel cache behavior` web/src/composables/dashboard/usePanelCache.spec.ts Add unit tests for panel cache composable Validate get/save interactions and edge cases	+524/-0
useCancelQuery.spec.ts `Tests for cancel query composable` web/src/composables/dashboard/useCancelQuery.spec.ts Add tests for canceling running queries Cover WS and streaming cancellation paths	+542/-0
useAnnotations.spec.ts `Tests for annotations composable` web/src/composables/dashboard/useAnnotations.spec.ts Add tests for annotations fetching logic Cover success and error flows	+310/-0

github-actions · 2025-09-01T12:56:10Z

PR Reviewer Guide 🔍

(Review updated until commit `ec5b089`)

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 5 🔵🔵🔵🔵🔵
🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review Flaky Async Mocking Overriding global setTimeout to execute immediately can mask real asynchronous behavior and cause timing-related flakiness or hide race conditions in production code paths. Consider using fake timers (vi.useFakeTimers) and advancing time deterministically instead of immediate Promise execution. // Mock setTimeout to work with promise-based async flows vi.spyOn(global, "setTimeout").mockImplementation((fn: any, ms: number) => { if (typeof fn === 'function') { // Execute immediately for tests Promise.resolve().then(() => fn()); } return 1 as any; }); Global Side Effects Tests mock global objects (IntersectionObserver, AbortController, window event listeners) and window.addEventListener twice; ensure restoration is complete between tests to avoid leakage across suites. Double-spying on addEventListener in beforeEach may shadow earlier spy references. // Mock IntersectionObserver for visibility detection global.IntersectionObserver = vi.fn().mockImplementation((callback) => ({ observe: vi.fn().mockImplementation(() => { // Immediately trigger visibility callback([{ isIntersecting: true }]); }), unobserve: vi.fn(), disconnect: vi.fn(), })); // Mock setTimeout to work with promise-based async flows vi.spyOn(global, "setTimeout").mockImplementation((fn: any, ms: number) => { if (typeof fn === 'function') { // Execute immediately for tests Promise.resolve().then(() => fn()); } return 1 as any; }); // Mock clearTimeout vi.spyOn(global, "clearTimeout").mockImplementation(() => {}); // Mock AbortController global.AbortController = vi.fn().mockImplementation(() => ({ signal: { addEventListener: vi.fn(), removeEventListener: vi.fn(), aborted: false, }, abort: vi.fn(), })); // Mock window.addEventListener and removeEventListener vi.spyOn(window, 'addEventListener').mockImplementation(() => {}); vi.spyOn(window, 'removeEventListener').mockImplementation(() => {}); }); Assertion Coupled To Console Tests assert on console.error calls for error paths; this couples tests to logging. Prefer asserting composable state changes or returned values instead of log output to reduce brittleness. expect(composable.panelsList.value).toEqual([]); expect(consoleErrorSpy).toHaveBeenCalledWith("Error fetching panels:", mockError); });

github-actions · 2025-09-01T12:56:44Z

PR Code Suggestions ✨

Latest suggestions up to ec5b089
Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Provide unique timer IDs Returning a constant handle for all timers can cause clearTimeout and subsequent logic relying on unique IDs to malfunction. Return a unique numeric handle per call to better emulate timer behavior in the code under test. web/src/composables/dashboard/usePanelDataLoader.spec.ts [310-316] +let __timerId = 0; vi.spyOn(global, "setTimeout").mockImplementation((fn: any, ms: number) => { - if (typeof fn === 'function') { - // Execute immediately for tests + const id = ++__timerId; + if (typeof fn === "function") { Promise.resolve().then(() => fn()); } - return 1 as any; + return id as any; }); Suggestion importance[1-10]: 7 __ Why: Returning a constant timeout handle can break code that relies on distinct IDs; generating unique IDs better simulates real timers and reduces flakiness. The change is correct and low-risk but not critical.	Medium
General	Simulate IndexedDB upgrade event The mock for `indexedDB.open` never triggers `onupgradeneeded`, so upgrade-path tests may not execute in environments expecting it when creating a new DB or store. Invoke `request.onupgradeneeded` before `onsuccess` when appropriate to reliably cover upgrade logic. web/src/composables/dashboard/usePanelCache.spec.ts [92-108] Object.defineProperty(global, 'indexedDB', { value: { open: () => { - const request = createMockRequest(mockDatabase); + const request = createMockRequest(mockDatabase) as any; queueMicrotask(() => { if (shouldThrowError && request.onerror) { request.onerror({ target: request }); - } else if (!shouldThrowError && request.onsuccess) { - request.onsuccess({ target: request }); + } else if (!shouldThrowError) { + // Trigger upgrade first to simulate initial DB/setup path + if (typeof request.onupgradeneeded === "function") { + request.onupgradeneeded({ target: request }); + } + if (request.onsuccess) { + request.onsuccess({ target: request }); + } } }); return request; }, }, writable: true, configurable: true }); Suggestion importance[1-10]: 7 __ Why: Triggering `onupgradeneeded` improves coverage of the upgrade path and better simulates real IndexedDB behavior. The change is consistent with the mock structure and benefits test reliability without altering production code.	Medium
	Avoid duplicate mock exports Providing both a default factory and top-level mocked functions with the same names can create ambiguity and mask calls, making assertions unreliable. Remove the unused top-level functions and expose only the default factory with the function spies you assert on. web/src/composables/dashboard/useValuesWebSocket.spec.ts [5-14] vi.mock("../useSearchWebSocket", () => ({ default: () => ({ fetchQueryDataWithWebSocket: vi.fn(), sendSearchMessageBasedOnRequestId: vi.fn(), cancelSearchQueryBasedOnRequestId: vi.fn(), }), - fetchQueryDataWithWebSocket: vi.fn(), - sendSearchMessageBasedOnRequestId: vi.fn(), - cancelSearchQueryBasedOnRequestId: vi.fn(), })); Suggestion importance[1-10]: 6 __ Why: Having both default-factory functions and top-level mocks with identical names can cause ambiguity during imports and assertions. Removing unused duplicates simplifies tests and prevents masking, offering a moderate maintainability improvement.	Low
	Avoid strict Event instance comparison The expectation relies on strict object identity for the Event instance, which can intermittently fail. Spy on `dispatchEvent` and assert the event type instead of comparing with `new Event(...)`. This makes the test robust across environments. web/src/composables/dashboard/useCancelQuery.spec.ts [150-156] it("should return early when no trace IDs are set", () => { const composable = useCancelQuery(); composable.cancelQuery(); - expect(windowDispatchEventSpy).toHaveBeenCalledWith(new Event("cancelQuery")); + expect(windowDispatchEventSpy).toHaveBeenCalled(); + const dispatchedEvent = windowDispatchEventSpy.mock.calls[0][0]; + expect(dispatchedEvent?.type).toBe("cancelQuery"); expect(mockQueryService.delete_running_queries).not.toHaveBeenCalled(); }); Suggestion importance[1-10]: 6 __ Why: Comparing with `new Event("cancelQuery")` is brittle; asserting on `dispatchEvent` call and event `type` is more robust and still validates behavior. The improved_code aligns with the existing test intent and the PR code.	Low
	Compare event type, not instance This test also compares Event objects by identity. Assert on the dispatched event’s `type` instead to prevent brittle failures and keep focus on the intended behavior. web/src/composables/dashboard/useCancelQuery.spec.ts [171-175] it("should return early when traceIdRef.value is not an array", () => { const composable = useCancelQuery(); // Manually set to non-array value composable.traceIdRef.value = "not-an-array"; composable.cancelQuery(); - expect(windowDispatchEventSpy).toHaveBeenCalledWith(new Event("cancelQuery")); + expect(windowDispatchEventSpy).toHaveBeenCalled(); + const dispatchedEvent = windowDispatchEventSpy.mock.calls[0][0]; + expect(dispatchedEvent?.type).toBe("cancelQuery"); expect(mockQueryService.delete_running_queries).not.toHaveBeenCalled(); }); Suggestion importance[1-10]: 6 __ Why: This mirrors the first case: asserting on an Event instance can fail due to identity; checking the dispatched event’s `type` is safer and keeps the test purpose intact.	Low
	Restore lifecycle mocks after tests Overriding lifecycle hooks globally in tests can affect other composables that rely on real mounting behavior, leading to false positives. Scope the stubs to only this spec by restoring the original implementations in afterEach. web/src/composables/dashboard/usePanelDataLoader.spec.ts [192-199] vi.mock("vue", async () => { const actual = await vi.importActual("vue"); - return { - ...actual, - onMounted: vi.fn(() => {}), // Don't execute callback to avoid loadData being called automatically - onUnmounted: vi.fn((cb) => cb?.()), - }; + const onMountedMock = vi.fn(() => {}); + const onUnmountedMock = vi.fn((cb) => cb?.()); + return { ...actual, onMounted: onMountedMock, onUnmounted: onUnmountedMock }; }); +// later in afterEach of this spec file: +// vi.restoreAllMocks(); // already present, ensure it runs after each to restore lifecycle mocks Suggestion importance[1-10]: 4 __ Why: The idea to ensure lifecycle mocks are restored is sensible for test isolation, but the spec already calls `vi.restoreAllMocks()` in `afterEach`, so the added note is marginal improvement and somewhat redundant.	Low

Previous suggestions

Suggestions up to commit d8f5f12

Category	Suggestion	Impact
General	Avoid conflicting duplicate mocks Remove duplicate exports to avoid masking the default factory’s functions and causing inconsistent mocks. Keep only the default factory or the named mocks consistent with how the composable imports them. web/src/composables/dashboard/useValuesWebSocket.spec.ts [5-14] vi.mock("../useSearchWebSocket", () => ({ default: () => ({ fetchQueryDataWithWebSocket: vi.fn(), sendSearchMessageBasedOnRequestId: vi.fn(), cancelSearchQueryBasedOnRequestId: vi.fn(), - }), - fetchQueryDataWithWebSocket: vi.fn(), - sendSearchMessageBasedOnRequestId: vi.fn(), - cancelSearchQueryBasedOnRequestId: vi.fn(), + }) })); Suggestion importance[1-10]: 7 __ Why: Removing duplicate named mocks prevents potential conflicts with the default factory and aligns with how the module is used; beneficial for test correctness.	Medium
Possible issue	Await async call in test Await the async operation before asserting, otherwise the test may pass before side-effects complete. Wrap `cancelQuery()` in `await` (or return the promise) and remove reliance on microtask timing to avoid flakiness. web/src/composables/dashboard/useCancelQuery.spec.ts [135-145] it("should dispatch cancelQuery event", async () => { const mockResponse = { data: [{ is_success: true }] }; mockQueryService.delete_running_queries.mockResolvedValue(mockResponse); const composable = useCancelQuery(); composable.searchRequestTraceIds(["trace1"]); - composable.cancelQuery(); + await composable.cancelQuery(); expect(windowDispatchEventSpy).toHaveBeenCalledWith(new Event("cancelQuery")); }); Suggestion importance[1-10]: 6 __ Why: Awaiting `cancelQuery()` makes the test more robust against async timing; the change is correct and low-risk but not critical.	Low
Possible issue	Await before verifying mock calls Ensure the assertion runs after the promise settles to avoid race conditions. Await `cancelQuery()` so the mock invocation is guaranteed before verifying calls. web/src/composables/dashboard/useCancelQuery.spec.ts [176-190] it("should call delete_running_queries with correct parameters", async () => { const mockResponse = { data: [{ is_success: true }] }; mockQueryService.delete_running_queries.mockResolvedValue(mockResponse); const composable = useCancelQuery(); composable.searchRequestTraceIds(["trace1", "trace2"]); - composable.cancelQuery(); + await composable.cancelQuery(); expect(mockQueryService.delete_running_queries).toHaveBeenCalledWith( "test-org", ["trace1", "trace2"] ); }); Suggestion importance[1-10]: 6 __ Why: Ensuring the promise settles before asserting avoids race conditions; it's a valid improvement in test reliability with moderate impact.	Low

web/src/composables/dashboard/usePanelDataLoader.spec.ts

github-actions · 2025-09-02T11:34:12Z

Persistent review updated to latest commit ec5b089

greptile-apps

Greptile Summary

This PR adds comprehensive unit test coverage for 6 critical dashboard composables in the Vue.js frontend, adding approximately 2,700+ lines of test code. The tests cover:

useAnnotations.spec.ts (310 lines): Tests the annotations fetching composable including parameter validation, error handling, API response processing, and edge cases with proper mocking of the annotation service
useAnnotationsData.spec.ts (582 lines): Tests the annotations data management composable covering UI state toggles, dialog visibility, annotation creation/editing, panel fetching via getDashboard, and time conversion logic including zero/negative value handling
useCancelQuery.spec.ts (542 lines): Tests the query cancellation workflows including trace ID management, concurrent operations, error scenarios, and integration with notification services and Vuex store
usePanelCache.spec.ts (524 lines): Tests the IndexedDB-based panel caching system with sophisticated mocking, covering save/retrieve operations, error handling, data integrity, and global cache management functions
useValuesWebSocket.spec.ts (719 lines): Tests the WebSocket/streaming/REST API communication for fetching field values, including response handling, filter merging, trace ID management, and fallback mechanisms
usePanelDataLoader.spec.ts (2,666 lines): The largest test file covering the core data loading composable with extensive testing of WebSocket connections, caching, variable processing, error handling, and various panel configurations

These composables are fundamental to the dashboard functionality in OpenObserve, handling data fetching, caching, real-time communication, annotations, and query management. The tests follow proper testing patterns with comprehensive mocking, edge case coverage, and proper cleanup procedures. The test suite significantly improves code coverage for critical dashboard components that were previously untested.

Confidence score: 4/5

This PR is generally safe to merge as it only adds test files without modifying production code
Score reflects the comprehensive nature of the tests but acknowledges that test-only changes have minimal production risk
Pay close attention to the useCancelQuery.spec.ts file for potential issues with undefined store state handling

_{6 files reviewed, 1 comment}

_{Edit Code Review Bot Settings | Greptile}

greptile-apps · 2025-09-02T11:33:41Z

web/src/composables/dashboard/usePanelCache.spec.ts

+        object: {
+          deep: {
+            value: "test",
+            array: [null, null, true, false],  // undefined becomes null after JSON serialization


syntax: Comment about JSON serialization behavior is incorrect - undefined values are removed, not converted to null in JSON

Suggested change

array: [null, null, true, false], // undefined becomes null after JSON serialization

array: [null, null, true, false], // undefined values are omitted during JSON serialization

github-actions bot added ☢️ Bug Something isn't working 🧹 Updates Review effort 3/5 labels Sep 1, 2025

github-advanced-security bot found potential problems Sep 1, 2025

View reviewed changes

web/src/composables/dashboard/usePanelDataLoader.spec.ts Fixed Show fixed Hide fixed

ktx-vaidehi force-pushed the fix/dashboard/pending-unit-tests branch from 614ff0d to e709fc5 Compare September 2, 2025 09:30

ktx-kirtan approved these changes Sep 2, 2025

View reviewed changes

ktx-vaidehi marked this pull request as ready for review September 2, 2025 11:33

ktx-vaidehi force-pushed the fix/dashboard/pending-unit-tests branch from 8c854cd to ec5b089 Compare September 2, 2025 11:33

github-actions bot added Review effort 5/5 and removed Review effort 3/5 labels Sep 2, 2025

greptile-apps bot reviewed Sep 2, 2025

View reviewed changes

ktx-vaidehi force-pushed the fix/dashboard/pending-unit-tests branch 2 times, most recently from 92376f5 to 7959cd3 Compare September 3, 2025 04:39

ktx-vaidehi added 7 commits September 3, 2025 11:26

useAnnotations unit tests

1e0560b

useAnnotationsData unit tests

226b438

useCancelQuery unit tests

fed1627

usePanelCache unit tests

51f3a9a

useValuesWebSocket unit tests

b6a3911

usePanelDataLoader unit tests

27f0b21

fix: loader file

9f71e8a

ktx-vaidehi force-pushed the fix/dashboard/pending-unit-tests branch from 7959cd3 to 9f71e8a Compare September 3, 2025 05:56

ktx-vaidehi merged commit d44db08 into main Sep 3, 2025
28 checks passed

ktx-vaidehi deleted the fix/dashboard/pending-unit-tests branch September 3, 2025 06:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix: dashboard unit tests #8239

fix: dashboard unit tests #8239

Uh oh!

ktx-vaidehi commented Sep 1, 2025 •

edited by github-actions bot

Loading

Uh oh!

github-actions bot commented Sep 1, 2025 •

edited

Loading

Uh oh!

github-actions bot commented Sep 1, 2025 •

edited

Loading

Uh oh!

Uh oh!

github-actions bot commented Sep 2, 2025

Uh oh!

greptile-apps bot left a comment

Uh oh!

greptile-apps bot Sep 2, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	array: [null, null, true, false], // undefined becomes null after JSON serialization
	array: [null, null, true, false], // undefined values are omitted during JSON serialization

fix: dashboard unit tests #8239

fix: dashboard unit tests #8239

Uh oh!

Conversation

ktx-vaidehi commented Sep 1, 2025 • edited by github-actions bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Type

Description

Diagram Walkthrough

File Walkthrough

Uh oh!

github-actions bot commented Sep 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Reviewer Guide 🔍

(Review updated until commit ec5b089)

Uh oh!

github-actions bot commented Sep 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Code Suggestions ✨

Previous suggestions

Uh oh!

Uh oh!

github-actions bot commented Sep 2, 2025

Uh oh!

greptile-apps bot left a comment

Choose a reason for hiding this comment

Greptile Summary

Confidence score: 4/5

Uh oh!

greptile-apps bot Sep 2, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ktx-vaidehi commented Sep 1, 2025 •

edited by github-actions bot

Loading

github-actions bot commented Sep 1, 2025 •

edited

Loading

(Review updated until commit `ec5b089`)

github-actions bot commented Sep 1, 2025 •

edited

Loading