Skip to content

🧪 QA: Add missing test for ComparativeProgressService #350

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.

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Open
daggerstuff wants to merge 1 commit into
base: staging
Choose a base branch
from
+29 −0
Open

🧪 QA: Add missing test for ComparativeProgressService #350

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 29 additions & 0 deletions src/lib/services/analytics/__tests__/ComparativeProgressService.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
import { describe, it, expect, vi, beforeEach } from "vitest"
import { ComparativeProgressService } from "../ComparativeProgressService"

describe("ComparativeProgressService", () => {
Comment on lines +1 to +4
Copy link

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test file uses double quotes, which is inconsistent with the repo’s standard single-quote formatting in TS/Vitest tests (e.g., AnalyticsService.test.ts). Running the repo formatter (oxfmt) / aligning to single quotes will keep formatting consistent and avoid churn in future formatting-only diffs.

Copilot uses AI. Check for mistakes.
let service: ComparativeProgressService
let mockLogger: any
Copy link
Copy Markdown

@charliecreates charliecreates bot Apr 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using any for mockLogger removes useful constraints and can hide real test mistakes (e.g., misspelling error/info or passing the wrong callable shape). Even in tests, prefer a minimal explicit type or satisfies so the mock stays aligned with what ComparativeProgressService expects.

Suggestion

Replace any with a small, explicit logger shape (or satisfies the service's logger type if it’s exported).

let mockLogger: { info: ReturnType<typeof vi.fn>; error: ReturnType<typeof vi.fn> }

beforeEach(() => {
  mockLogger = {
    info: vi.fn(),
    error: vi.fn()
  }
  service = new ComparativeProgressService(mockLogger)
})

If the real logger type is available, prefer:

const mockLogger = {
  info: vi.fn(),
  error: vi.fn()
} satisfies Logger

Reply with "@CharlieHelps yes please" if you’d like me to add a commit with this change.

Copy link

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mockLogger is typed as any, which makes refactors harder (e.g., changing the logger interface won’t surface type errors in tests). Consider typing it via the service constructor parameter type (e.g., ConstructorParameters<typeof ComparativeProgressService>[0]) or a locally declared logger type matching the constructor.

Suggested change
let mockLogger: any
let mockLogger: ConstructorParameters<typeof ComparativeProgressService>[0]

Copilot uses AI. Check for mistakes.

beforeEach(() => {
mockLogger = {
info: vi.fn(),
error: vi.fn()
}
service = new ComparativeProgressService(mockLogger)
})

it("should return insufficient data when no user progress snapshots exist", async () => {
vi.spyOn(service as any, "fetchUserProgressData").mockResolvedValue([])
Copy link
Copy Markdown

@charliecreates charliecreates bot Apr 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Spying on an internal method via (service as any) makes the test brittle and weakens refactoring safety (renames/visibility changes won’t be caught cleanly). This also introduces additional any usage, which can mask incorrect stubbing.

Suggestion

Prefer stubbing at an injected dependency boundary (ideal), or make the data-fetch method protected and override it in a test subclass.

Example (if you can change visibility to protected):

class TestComparativeProgressService extends ComparativeProgressService {
  protected override fetchUserProgressData() {
    return Promise.resolve([])
  }
}

beforeEach(() => {
  service = new TestComparativeProgressService(mockLogger)
})

At minimum, also ensure spies don’t leak across tests:

afterEach(() => vi.restoreAllMocks())

Reply with "@CharlieHelps yes please" if you’d like me to add a commit with a safer stubbing approach.


const result = await service.analyzeProgress({
anonymizedUserId: "test-user",
metricName: "test-metric",
cohortId: "test-cohort",
dateRange: { startDate: "2023-01-01", endDate: "2023-01-31" }
Comment on lines +21 to +23
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot Apr 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Validate no-snapshot path with a non-null benchmark

Using metricName: "test-metric" makes fetchBenchmarkData return null in ComparativeProgressService, so the current assertions can still pass even if the early-return branch for empty snapshots is removed: generateInsights would still produce insufficient_data. This means the test does not actually protect the intended regression scenario and gives false confidence about the no-user-data shortcut path.

Useful? React with 👍 / 👎.

})
Comment on lines +17 to +24
Copy link

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test uses service as any to spy on a private method. A narrower cast (e.g., to an object type that only exposes fetchUserProgressData) keeps TypeScript checking in place and reduces the chance of the test silently breaking if the method name/signature changes.

Suggested change
vi.spyOn(service as any, "fetchUserProgressData").mockResolvedValue([])
const result = await service.analyzeProgress({
anonymizedUserId: "test-user",
metricName: "test-metric",
cohortId: "test-cohort",
dateRange: { startDate: "2023-01-01", endDate: "2023-01-31" }
})
const serviceWithFetch = service as Pick<ComparativeProgressService, "fetchUserProgressData">
vi.spyOn(serviceWithFetch, "fetchUserProgressData").mockResolvedValue([])
const result = await service.analyzeProgress({
anonymizedUserId: "test-user",
metricName: "test-metric",
cohortId: "test-cohort",
dateRange: { startDate: "2023-01-01", endDate: "2023-01-31" }

Copilot uses AI. Check for mistakes.

expect(result.userProgressSnapshots).toHaveLength(0)
expect(result.comparisonInsights.trend).toBe("insufficient_data")
})
})
Loading