test: migrate to vitest. add deal.service.spec.ts and unit test CI #83
Conversation
There was a problem hiding this comment.
Pull request overview
This PR migrates the testing framework from Jest to Vitest, adds comprehensive unit tests for deal.service.ts, and implements a GitHub Action for running unit tests in CI. The migration improves developer experience with better mocking support and faster test execution, while maintaining full test coverage.
Key changes:
- Complete migration from Jest to Vitest testing framework with appropriate configuration
- New comprehensive unit test suite for
DealServicecovering initialization, deal creation, and error handling scenarios - Updated test scripts in package.json and removed Jest-specific configuration files
Reviewed changes
Copilot reviewed 9 out of 11 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| vitest.config.ts | New Vitest configuration for unit tests with coverage reporting via v8 provider |
| test/vitest-e2e.config.ts | New Vitest configuration specifically for e2e tests |
| src/deal/deal.service.spec.ts | Comprehensive unit tests for DealService with 100+ lines covering all major code paths |
| src/metrics/utils/time-window-parser.test.ts | Migrated existing tests to Vitest with UTC date handling fixes |
| package.json | Updated test scripts and dependencies, replacing Jest with Vitest |
| jest.config.js | Removed Jest configuration (no longer needed) |
| test/jest-e2e.json | Removed Jest e2e configuration (replaced by vitest-e2e.config.ts) |
| tsconfig.test.json | Removed (no longer needed with Vitest) |
| biome.json | Removed Jest global variables from linter configuration |
| pnpm-lock.yaml | Updated dependencies reflecting Jest → Vitest migration |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
silent-cipher
left a comment
silent-cipher
left a comment
There was a problem hiding this comment.
Everything looks good to me overall. The only concern is the migration of test/app.e2e-spec.ts to vitest.
github-project-automation
bot
moved this from 📌 Triage
to ✔️ Approved by reviewer
in FS
.github/workflows/biome.yml
Outdated
|
|
||
| - name: Run Biome | ||
| run: biome ci . | ||
| run: pnpm lint:check |
There was a problem hiding this comment.
I just noticed this - switching from biome ci . to pnpm lint:check seems to skip formatting checks. It only runs linting, so formatting issues won’t get caught.
I tried running pnpm format locally and it updated three unformatted files.
There was a problem hiding this comment.
i'm thinking we should just use biome check since that runs both format and lint
There was a problem hiding this comment.
Yes, we can do that but biome check feels more suited for local development, whereas biome ci is intended specifically for CI/CD workflows.
@silent-cipher the e2e test isn't being ran anywhere today that I can see, and fails for me locally.. e2e seems broken anyway until we get #81 or similar local setup so e2e tests can be ran easily |
migrates from jest to vitest (better mocking support, and better DX all around)
adds deal.service.spec.ts (unit tests for deal.service.ts)
adds github action to run unit tests.