migrate vitest to v4 #322
Draft
migrate vitest to v4 #322
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
Reviewer's GuideMigrates the monorepo’s testing stack to Vitest 4, introduces a shared constructor-mocking helper, tightens Storybook/browser Vitest configuration for CI stability, aligns GraphQL stories and repository tests with new schemas and mocking patterns, and splits coverage into separate Node vs UI pipelines wired through package.json and Azure Pipelines. Sequence diagram for MongoUnitOfWork transaction and integration events with Vitest 4 mockssequenceDiagram
participant Test as TestScenario
participant UnitOfWork as MongoUnitOfWork
participant Mongoose as mongoose.connection
participant RepoClass as TestRepoClass
participant Repo as RepoMock
participant IntegrationBus as IntegrationEventBus
Test->>Mongoose: transaction(callback)
activate Mongoose
Mongoose->>UnitOfWork: callback(session)
activate UnitOfWork
UnitOfWork->>RepoClass: new RepoClass(Passport, model, typeConverter, bus, session)
activate RepoClass
RepoClass-->>UnitOfWork: Repo instance
deactivate RepoClass
UnitOfWork->>Repo: get(aggId)
activate Repo
Repo-->>UnitOfWork: AggregateRootMock
UnitOfWork->>Repo: domainOperation(repo)
Repo-->>UnitOfWork: domain changes and addIntegrationEvent
UnitOfWork->>Repo: save(aggregate)
Repo-->>UnitOfWork: saved
UnitOfWork->>Repo: getIntegrationEvents()
Repo-->>UnitOfWork: [event1, event2]
loop for each event
UnitOfWork->>IntegrationBus: dispatch(EventClass, payload)
activate IntegrationBus
IntegrationBus-->>UnitOfWork: resolved
deactivate IntegrationBus
end
UnitOfWork-->>Mongoose: transaction completed
deactivate UnitOfWork
Mongoose-->>Test: resolved
deactivate Mongoose
rect rgb(255,220,220)
Test->>Mongoose: transaction(callback)
Mongoose->>UnitOfWork: callback(session)
UnitOfWork->>RepoClass: new RepoClass(...)
UnitOfWork->>Repo: domainOperation(repo)
UnitOfWork->>Repo: getIntegrationEvents()
Repo-->>UnitOfWork: [event1, event2]
UnitOfWork->>IntegrationBus: dispatch(event1)
IntegrationBus-->>UnitOfWork: rejected Error
UnitOfWork-->>Test: propagate Error
end
Class diagram for shared Vitest constructor mocking and related testsclassDiagram
class TestUtils {
+makeNewableMock~TArgs, TResult~(impl TArgs... TResult) MockFunction
}
class MockFunction {
<<function>>
}
class OtelBuilderTests {
+MockExporter
+traceExporterMock
+metricExporterMock
+logExporterMock
}
class TwilioTests {
+mockConversationInstance
+mockClient
+TwilioConstructor
}
class AccountPlanRepositoryTests {
+makeAccountPlanDoc()
+setupAccountPlanRepo()
}
class ReservationRequestRepositoryTests {
+makeReservationRequestDoc()
+setupReservationRequestRepo()
}
class ConversationRepositoryTests {
+makeConversationDoc()
+setupConversationRepo()
}
class AdminUserRepositoryTests {
+makeAdminUserDoc()
+setupAdminUserRepo()
}
class PersonalUserRepositoryTests {
+makePersonalUserDoc()
+setupPersonalUserRepo()
}
class ServiceMessagingTwilio {
+constructor(accountSid string, authToken string)
}
class OtelBuilder {
+buildExporters(enable bool) Exporters
}
class Exporters {
+traceExporter any
+metricExporter any
+logExporter any
}
TestUtils ..> MockFunction : returns
OtelBuilderTests ..> TestUtils : uses makeNewableMock
TwilioTests ..> TestUtils : uses makeNewableMock
AccountPlanRepositoryTests ..> TestUtils : uses makeNewableMock
ReservationRequestRepositoryTests ..> TestUtils : uses makeNewableMock
ConversationRepositoryTests ..> TestUtils : uses makeNewableMock
AdminUserRepositoryTests ..> TestUtils : uses makeNewableMock
PersonalUserRepositoryTests ..> TestUtils : uses makeNewableMock
OtelBuilderTests ..> OtelBuilder : builds exporters
TwilioTests ..> ServiceMessagingTwilio : constructs service
OtelBuilder ..> Exporters : returns
TwilioTests ..> Exporters : verifies exporters via mocks
Class diagram for Storybook Vitest configuration with Vitest 4 browser PlaywrightclassDiagram
class StorybookVitestConfigOptions {
+storybookDirRelativeToPackage string
+setupFiles string[]
+browsers BrowserInstanceConfig[]
+additionalCoverageExclude string[]
}
class BrowserInstanceConfig {
+browser string
}
class VitestConfigModule {
+createStorybookVitestConfig(pkgDirname string, opts StorybookVitestConfigOptions) UserConfig
}
class UserConfig {
+resolve any
+server any
+test VitestTestConfig
}
class VitestTestConfig {
+exclude string[]
+globals bool
+retry number
+testTimeout number
+fileParallelism bool
+projects any[]
+coverage CoverageConfig
+watch bool
+isolate bool
}
class CoverageConfig {
+exclude string[]
}
class BaseConfigModule {
+baseConfig UserConfig
}
class StorybookAddonVitest {
+storybookTest(config any) any
}
class VitestBrowserPlaywright {
+playwright() any
}
VitestConfigModule ..> StorybookVitestConfigOptions : uses
VitestConfigModule ..> UserConfig : returns
VitestConfigModule ..> VitestTestConfig : configures
VitestTestConfig ..> CoverageConfig : contains
VitestConfigModule ..> BaseConfigModule : mergeConfig
VitestConfigModule ..> StorybookAddonVitest : uses storybookTest
VitestConfigModule ..> VitestBrowserPlaywright : uses playwright
StorybookVitestConfigOptions "1" *-- "*" BrowserInstanceConfig
Flow diagram for split Node vs UI coverage in CI with Vitest 4flowchart TD
A_Developer[Developer]
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- The MongoUnitOfWork tests contain several repeated patterns for building custom domain operations that create aggregates and add integration events; consider extracting small helper functions (e.g.
makeOpWithEvents(...)) to reduce duplication and make each scenario easier to read and maintain. - There are a number of
as unknown as ...andas MockInstancecasts around mocked event buses and handlers (e.g. in the MongoUnitOfWork and event bus tests); introducing small typed factory helpers for these mocks would tighten type-safety and avoid the need for broad casts in the test bodies.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The MongoUnitOfWork tests contain several repeated patterns for building custom domain operations that create aggregates and add integration events; consider extracting small helper functions (e.g. `makeOpWithEvents(...)`) to reduce duplication and make each scenario easier to read and maintain.
- There are a number of `as unknown as ...` and `as MockInstance` casts around mocked event buses and handlers (e.g. in the MongoUnitOfWork and event bus tests); introducing small typed factory helpers for these mocks would tighten type-safety and avoid the need for broad casts in the test bodies.
## Individual Comments
### Comment 1
<location> `packages/cellix/mongoose-seedwork/src/mongoose-seedwork/mongo-unit-of-work.test.ts:146` </location>
<code_context>
});
});
Scenario('Domain operation with no events, completes successfully', ({ Given, When, Then }) => {
Given('a domain operation that emits no domain or integration events', () => {
- repoInstance.getIntegrationEvents = vi.fn(() => []);
</code_context>
<issue_to_address>
**issue (complexity):** Consider extracting shared helpers for domain operations, repo/model setup, and a typed dispatch mock to remove duplication and clarify the tests’ intent while keeping existing behavior.
You can keep all the new behaviors while substantially reducing complexity and duplication by extracting a few small helpers and tightening the typing of your mocks.
### 1. Factor out domain operation factories
All `domainOperation`, `customDomainOp`, `customDomainOp2`, and `customDomainOp3` share the same structure: load aggregate → mutate → (optionally) add events → save.
You can encapsulate that in a couple of helpers:
```ts
type RepoOp = (repo: RepoMock) => Promise<void>;
type RepoOpMock = RepoOp & MockInstance;
function makeDomainOpWithoutEvents(): RepoOpMock {
const op: RepoOp = async (repo) => {
const aggregate = await repo.get('agg-1');
aggregate.foo = 'new-foo';
await repo.save(aggregate);
};
return vi.fn(op) as RepoOpMock;
}
function makeDomainOpWithEvents(events: TestEvent[]): RepoOpMock {
const op: RepoOp = async (repo) => {
const aggregate = await repo.get('agg-1');
aggregate.foo = 'new-foo';
for (const e of events) {
aggregate.addIntegrationEvent(TestEvent, e.payload);
}
await repo.save(aggregate);
};
return vi.fn(op) as RepoOpMock;
}
```
Then in scenarios:
```ts
let domainOperation: RepoOpMock;
BeforeEachScenario(() => {
// ...
domainOperation = makeDomainOpWithoutEvents();
});
Scenario('Domain operation emits integration events, all dispatch succeed', ({ Given, When, Then }) => {
let event1: TestEvent;
let event2: TestEvent;
Given('integration events are emitted during the domain operation', () => {
event1 = new TestEvent('id'); event1.payload = { foo: 'bar1' };
event2 = new TestEvent('id'); event2.payload = { foo: 'bar2' };
domainOperation = makeDomainOpWithEvents([event1, event2]);
});
// ...
});
```
This removes `customDomainOp`, `customDomainOp2`, `customDomainOp3` and the repeated inline async functions while preserving behavior.
---
### 2. Strongly-type the dispatch mock once
Instead of repeatedly casting `integrationEventBus.dispatch` to `MockInstance`, define a mock type alias and use it consistently:
```ts
type DispatchMock = MockInstance<
[new (...args: any[]) => DomainSeedwork.DomainEvent<any>, any],
Promise<void>
>;
let dispatchMock: DispatchMock;
BeforeEachScenario(() => {
dispatchMock = vi.fn() as DispatchMock;
integrationEventBus = {
dispatch: dispatchMock,
register: vi.fn(),
} as unknown as DomainSeedwork.EventBus;
// ...
});
```
Then in scenarios you can avoid the `unknown` cast gymnastics:
```ts
When('the transaction completes successfully', async () => {
domainOperation.mockClear();
dispatchMock.mockClear();
await unitOfWork.withTransaction(Passport, domainOperation);
});
Then('all integration events are dispatched after the transaction commits', () => {
expect(dispatchMock).toHaveBeenCalledTimes(2);
expect(dispatchMock).toHaveBeenNthCalledWith(1, event1.constructor, event1.payload);
expect(dispatchMock).toHaveBeenNthCalledWith(2, event2.constructor, event2.payload);
});
```
This keeps the same assertions but centralizes the casting and improves readability.
---
### 3. Extract repo & model setup helpers instead of reconfiguring per scenario
You’re repeating:
- `repoInstance.getIntegrationEvents = vi.fn(() => []) as ...;`
- `repoInstance.get = vi.fn().mockResolvedValue(new AggregateRootMock(...));`
- `repoInstance.save = vi.fn().mockResolvedValue(undefined);`
- Re-setup of `mockModel.findById` after `vi.clearAllMocks()`.
Create small helpers:
```ts
function setupMockModelReturningAggregate() {
(mockModel.findById as MockInstance).mockReturnValue({
exec: vi.fn().mockResolvedValue({
_id: 'agg-1',
foo: 'old-foo',
}),
});
}
function setupRepoNoEvents() {
repoInstance.getIntegrationEvents = vi.fn(() => []) as typeof repoInstance.getIntegrationEvents;
repoInstance.get = vi.fn().mockResolvedValue(
new AggregateRootMock(
{
id: 'agg-1',
foo: 'old-foo',
createdAt: new Date(),
updatedAt: new Date(),
schemaVersion: '1',
} as PropType,
{},
),
);
repoInstance.save = vi.fn().mockResolvedValue(undefined);
}
```
Use them in `BeforeEachScenario` and scenarios:
```ts
BeforeEachScenario(() => {
// ... create mockModel, repoInstance, unitOfWork, etc.
vi.clearAllMocks();
vi.spyOn(mongoose.connection, 'transaction').mockImplementation(async cb =>
cb({} as ClientSession),
);
setupMockModelReturningAggregate();
});
Scenario('Domain operation with no events, completes successfully', ({ Given, When, Then }) => {
Given('a domain operation that emits no domain or integration events', () => {
setupRepoNoEvents();
});
// ...
});
```
This removes duplicated plumbing and keeps each scenario’s `Given` focused on behavior.
---
### 4. Simplify repo wiring assertions
You currently assert individual internal properties of the repo instance. You can keep the safety of checking that `withTransaction` passes a correctly-constructed repo without hard-coding internal structure:
```ts
Then('the transaction is committed and no events are dispatched', () => {
expect(domainOperation).toHaveBeenCalled();
const repoArg = domainOperation.mock.calls[0]?.[0];
expect(repoArg).toBeInstanceOf(TestRepoClass);
expect((repoArg as TestRepoClass).model).toBe(mockModel);
expect((repoArg as TestRepoClass).typeConverter).toBe(typeConverter);
expect((repoArg as TestRepoClass).bus).toBe(eventBus);
expect((repoArg as TestRepoClass).session).toStrictEqual(session);
expect(dispatchMock).not.toHaveBeenCalled();
});
```
Or, if you want to reduce coupling further:
```ts
expect(domainOperation).toHaveBeenCalledWith(expect.any(TestRepoClass));
```
The key is you don’t need to inspect every internal field of `TestRepoClass` in every scenario; one focused scenario can check wiring, and others can just assert `expect.any(TestRepoClass)` plus event behavior.
</issue_to_address>
### Comment 2
<location> `packages/sthrift/service-otel/src/otel-builder.test.ts:22` </location>
<code_context>
- const metricExporterMock = vi.fn((args) => Object.assign(new MetricExporter(), { __args: args }));
- const logExporterMock = vi.fn((args) => Object.assign(new LogExporter(), { __args: args }));
- // Expose mocks and classes for test access
+ // Class-based mocks that record constructor args
+ type ExporterArgs = Record<string, unknown>;
+ class TraceExporter {
</code_context>
<issue_to_address>
**issue (complexity):** Consider reverting to `vi.fn`-based constructor mocks with inner classes so you can keep `instanceof` and arg assertions while avoiding custom `mock.calls` state and helpers.
The new class/static `mock.calls` setup duplicates Vitest’s call tracking and makes the mock module harder to reason about. You can keep `instanceof` semantics and constructor arg assertions while simplifying back to `vi.fn`-based constructor mocks.
A possible simplification inside the `vi.mock` factory:
```ts
vi.mock('@azure/monitor-opentelemetry-exporter', () => {
class TraceExporter {
constructor(public __args: Record<string, unknown>) {}
}
class MetricExporter {
constructor(public __args: Record<string, unknown>) {}
}
class LogExporter {
constructor(public __args: Record<string, unknown>) {}
}
const AzureMonitorTraceExporter = vi
.fn<[], InstanceType<typeof TraceExporter>>()
.mockImplementation((args: Record<string, unknown>) => new TraceExporter(args));
const AzureMonitorMetricExporter = vi
.fn<[], InstanceType<typeof MetricExporter>>()
.mockImplementation((args: Record<string, unknown>) => new MetricExporter(args));
const AzureMonitorLogExporter = vi
.fn<[], InstanceType<typeof LogExporter>>()
.mockImplementation((args: Record<string, unknown>) => new LogExporter(args));
return {
AzureMonitorTraceExporter,
AzureMonitorMetricExporter,
AzureMonitorLogExporter,
__test: {
traceExporterMock: AzureMonitorTraceExporter,
metricExporterMock: AzureMonitorMetricExporter,
logExporterMock: AzureMonitorLogExporter,
TraceExporter,
MetricExporter,
LogExporter,
},
};
});
```
Then the test setup and assertions become simpler and rely on Vitest’s built-in tracking:
```ts
let traceExporterMock: ReturnType<typeof vi.fn>;
let metricExporterMock: ReturnType<typeof vi.fn>;
let logExporterMock: ReturnType<typeof vi.fn>;
BeforeEachScenario(async () => {
// ...
const azureModule = await import('@azure/monitor-opentelemetry-exporter') as any;
traceExporterMock = azureModule.__test.traceExporterMock;
metricExporterMock = azureModule.__test.metricExporterMock;
logExporterMock = azureModule.__test.logExporterMock;
traceExporterMock.mockClear();
metricExporterMock.mockClear();
logExporterMock.mockClear();
});
```
And the expectations:
```ts
expect(exporters.traceExporter).toBeInstanceOf(TraceExporter);
expect(traceExporterMock).toHaveBeenCalledWith({ connectionString: connStr });
expect((exporters.traceExporter as any).__args.connectionString).toBe(connStr);
```
This keeps:
- `instanceof` checks via the inner classes,
- per-instance `__args` for extra safety,
- constructor-argument assertions via `toHaveBeenCalledWith`,
while removing custom `mock.calls` arrays, manual resets, and the extra `clearMocks` helper.
</issue_to_address>
### Comment 3
<location> `packages/sthrift/persistence/src/datasources/readonly/account-plan/account-plan/account-plan.read-repository.test.ts:9` </location>
<code_context>
- AccountPlanConverter: vi.fn().mockImplementation(() => ({
- toDomain: vi.fn((doc) => ({ id: doc._id, name: doc.name })),
- })),
+ // biome-ignore lint/complexity/useArrowFunction: Constructor function must be a regular function
+ AccountPlanConverter: vi.fn(function() {
+ return {
</code_context>
<issue_to_address>
**issue (complexity):** Consider simplifying the mocks and model context so you can remove lint ignores, heavy casting, and unused nested factories while keeping test behavior unchanged.
You can keep all behavior while simplifying the mocks and removing lint ignores / heavy casting.
### 1. Simplify constructor mocks and drop lint suppression
If `AccountPlanConverter` / `AccountPlanDataSourceImpl` don’t need to be used with `new` in these tests, you can revert to a simpler arrow-based implementation:
```ts
vi.mock('../../../domain/account-plan/account-plan/account-plan.domain-adapter.ts', () => ({
AccountPlanConverter: vi.fn().mockImplementation(() => ({
toDomain: vi.fn((doc) => ({ id: doc._id, name: doc.name })),
})),
}));
vi.mock('../account-plan/account-plan.data.ts', () => ({
AccountPlanDataSourceImpl: vi.fn().mockImplementation(() => ({
find: vi.fn(),
findById: vi.fn(),
findOne: vi.fn(),
})),
}));
```
If production code requires calling them with `new`, you can still avoid `biome-ignore` by using a regular function in `mockImplementation` (not as the top-level callback):
```ts
vi.mock('../../../domain/account-plan/account-plan/account-plan.domain-adapter.ts', () => ({
AccountPlanConverter: vi.fn().mockImplementation(function () {
return {
toDomain: vi.fn((doc) => ({ id: doc._id, name: doc.name })),
};
}),
}));
```
Same shape, no lint ignore needed.
### 2. Slim down `makeModelsContext` to what the test actually uses
Since `AccountPlanDataSourceImpl` is fully mocked and other models are unused, you can keep the context minimal and avoid the `unknown` cast:
```ts
function makeModelsContext(): ModelsContext {
return {
AccountPlan: {
// Still a function so it’s “model-like”, but simple
AccountPlanModel: vi.fn(() => ({})),
},
} as ModelsContext;
}
```
If `ModelsContext` requires other properties at type level but they’re unused in behavior, you can stub them as `{} as any` without adding nested factories:
```ts
function makeModelsContext(): ModelsContext {
return {
User: {} as any,
Listing: {} as any,
Conversation: {} as any,
ReservationRequest: {} as any,
Role: {} as any,
AppealRequest: {} as any,
AccountPlan: {
AccountPlanModel: vi.fn(() => ({})),
},
} as ModelsContext;
}
```
This keeps all functionality intact while reducing verbosity, nested mocks, and the need for `as unknown as ModelsContext`.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
…r tests (addressed AI suggestions)
…-4x-and-fix-all-related-testing
…ile limit in CI pipeline
…-4x-and-fix-all-related-testing
…0000 in build pipeline
Contributor
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- In
createStorybookVitestConfig,test.watchis hard-disabled (watch: false), which may make local Storybook/Vitest runs less ergonomic; consider toggling this based on the sameisCIflag you use for retries andfileParallelismso watch mode remains available during local development. - In the AccountPlan read repository tests you repeatedly cast
repositoryto{ mongoDataSource: ... }to reachmongoDataSource; extracting a small typed helper (e.g.getMongoDataSource(repository)) would remove the duplicated casts and make the intent clearer. - Several tests now rely on explicit
as any/unknowncasts for mocks (e.g. OTEL exporter mocks, Mongo unit-of-work helpers); where possible, tightening these with shared mock types or helper factories would reduce the surface area for type regressions while keeping the Vitest v4 constructor patterns.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `createStorybookVitestConfig`, `test.watch` is hard-disabled (`watch: false`), which may make local Storybook/Vitest runs less ergonomic; consider toggling this based on the same `isCI` flag you use for retries and `fileParallelism` so watch mode remains available during local development.
- In the AccountPlan read repository tests you repeatedly cast `repository` to `{ mongoDataSource: ... }` to reach `mongoDataSource`; extracting a small typed helper (e.g. `getMongoDataSource(repository)`) would remove the duplicated casts and make the intent clearer.
- Several tests now rely on explicit `as any`/`unknown` casts for mocks (e.g. OTEL exporter mocks, Mongo unit-of-work helpers); where possible, tightening these with shared mock types or helper factories would reduce the surface area for type regressions while keeping the Vitest v4 constructor patterns.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- In
storybook.config.ts, the CI detection was narrowed fromprocess.env['CI'] === 'true' || process.env['TF_BUILD'] === 'True'to only checkingTF_BUILD; if these configs are ever run outside Azure Pipelines (e.g., localCI=trueor other CI providers), you may want to keep the genericCIflag to avoid changing retry/parallelism behavior unexpectedly. - The new
test:coverage:*scripts split node and UI coverage nicely, buttest:coverage:mergestill callstest:coverage(which runs both phases) rather than the more granulartest:coverage:node/ui; consider aligning:mergeand:merge:affectedto use the same split pattern to avoid accidental duplication or divergence between full and affected coverage workflows.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `storybook.config.ts`, the CI detection was narrowed from `process.env['CI'] === 'true' || process.env['TF_BUILD'] === 'True'` to only checking `TF_BUILD`; if these configs are ever run outside Azure Pipelines (e.g., local `CI=true` or other CI providers), you may want to keep the generic `CI` flag to avoid changing retry/parallelism behavior unexpectedly.
- The new `test:coverage:*` scripts split node and UI coverage nicely, but `test:coverage:merge` still calls `test:coverage` (which runs both phases) rather than the more granular `test:coverage:node/ui`; consider aligning `:merge` and `:merge:affected` to use the same split pattern to avoid accidental duplication or divergence between full and affected coverage workflows.
## Individual Comments
### Comment 1
<location> `packages/cellix/vitest-config/src/configs/storybook.config.ts:23` </location>
<code_context>
- },
- },
- }));
+ const isCI = process.env['TF_BUILD'] === 'True';
- return mergeConfig(base, defineConfig({}));
</code_context>
<issue_to_address>
**question (bug_risk):** The CI detection now only checks TF_BUILD, which may miss CI runs outside Azure DevOps.
This change means Storybook/Vitest will no longer detect CI in environments like GitHub Actions (where `CI='true'`), causing them to run with local-style settings (lower retries, shorter timeouts, parallel files, watch mode). If you want CI behaviour in any CI system, consider something like:
```ts
const isCI = process.env['TF_BUILD'] === 'True' || process.env['CI'] === 'true';
```
and let each pipeline decide which env vars to set.
</issue_to_address>
### Comment 2
<location> `packages/cellix/vitest-config/src/configs/storybook.config.ts:37-38` </location>
<code_context>
+ ignored: ['**/dist/**', '**/coverage/**', '**/node_modules/**'],
+ },
+ },
+ test: {
+ // Prevent Vite/Vitest from scanning/transpiling build artifacts and coverage temp files.
+ // This greatly reduces the number of open files during coverage runs in CI.
+ exclude: ['**/dist/**', '**/coverage/**', '**/coverage/.tmp/**'],
</code_context>
<issue_to_address>
**suggestion (performance):** Overriding `test.exclude` without including Vitest defaults may unintentionally re-include some files (e.g. node_modules) in test scanning.
Vitest’s `test.exclude` has a non-empty default (node_modules, dist, etc.). Setting it directly replaces that list instead of extending it, which can increase test scan I/O and counteract the goal of reducing open files. Consider merging with `configDefaults.exclude`, for example:
```ts
import { defineConfig, configDefaults } from 'vitest/config';
export default defineConfig({
test: {
exclude: [
...configDefaults.exclude,
'**/dist/**',
'**/coverage/**',
'**/coverage/.tmp/**',
],
},
});
```
This preserves Vitest’s defaults while adding your extra exclusions.
Suggested implementation:
```typescript
import { defineConfig, configDefaults } from 'vitest/config';
```
```typescript
test: {
// Prevent Vite/Vitest from scanning/transpiling build artifacts and coverage temp files.
// This greatly reduces the number of open files during coverage runs in CI.
// Extend Vitest's default excludes instead of replacing them outright.
exclude: [
...configDefaults.exclude,
'**/dist/**',
'**/coverage/**',
'**/coverage/.tmp/**',
],
```
If this file currently re-exports or merges other shared Vitest config pieces, ensure they do not also override `test.exclude` in a conflicting way. If the import from `'vitest/config'` is structured differently (e.g. `import * as vitestConfig`), adjust the added `configDefaults` import accordingly.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- In the new Storybook Vitest config,
isCInow only checksTF_BUILD; if you ever run these tests outside Azure DevOps, consider centralizing CI detection (e.g., includingCIor a shared helper) so behaviour like retries, timeouts, and fileParallelism stays consistent across environments.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In the new Storybook Vitest config, `isCI` now only checks `TF_BUILD`; if you ever run these tests outside Azure DevOps, consider centralizing CI detection (e.g., including `CI` or a shared helper) so behaviour like retries, timeouts, and fileParallelism stays consistent across environments.
## Individual Comments
### Comment 1
<location> `packages/cellix/mongoose-seedwork/src/mongoose-seedwork/mongo-unit-of-work.test.ts:35` </location>
<code_context>
class TestEvent extends DomainSeedwork.CustomDomainEventImpl<{ foo: string }> {}
+// Helper types and factories for domain operations
+type RepoOp = (repo: RepoMock) => Promise<void>;
+type RepoOpMock = RepoOp & MockInstance;
</code_context>
<issue_to_address>
**issue (complexity):** Consider simplifying the new test helpers and mocks by inlining scenario-specific setup and using the existing classes/mocks directly instead of extra wrapper types and aliases.
You can keep the new behavior while trimming a lot of the added machinery. The main win is to remove indirection that’s only used in one or two places.
### 1. Drop `RepoOp` / `RepoOpMock` / `makeDomainOp*` indirection
The test-specific domain operations are simple and only used per-scenario. You can inline them or use a tiny local helper without type aliases:
```ts
// Before
type RepoOp = (repo: RepoMock) => Promise<void>;
type RepoOpMock = RepoOp & MockInstance;
function makeDomainOpWithoutEvents(): RepoOpMock {
const op: RepoOp = async (repo) => {
const aggregate = await repo.get('agg-1');
aggregate.foo = 'new-foo';
await repo.save(aggregate);
};
return vi.fn(op) as RepoOpMock;
}
// Scenario
let customDomainOp: RepoOpMock;
Given(..., () => {
customDomainOp = makeDomainOpWithEvents([event1, event2]);
});
```
```ts
// After – inline per scenario
Given('integration events are emitted during the domain operation', () => {
event1 = new TestEvent('id');
event1.payload = { foo: 'bar1' };
event2 = new TestEvent('id');
event2.payload = { foo: 'bar2' };
customDomainOp = vi.fn(async (repo: RepoMock) => {
const aggregate = await repo.get('agg-1');
aggregate.foo = 'new-foo';
aggregate.addIntegrationEvent(TestEvent, event1.payload);
aggregate.addIntegrationEvent(TestEvent, event2.payload);
await repo.save(aggregate);
});
});
```
For the “no events” case, you can similarly inline the operation in the `Given` instead of using `makeDomainOpWithoutEvents`.
### 2. Remove `TestRepoClass` and assert on the actual instance
`TestRepoClass` is a pure alias of `RepoMock` only to allow `expect.any(TestRepoClass)`. You can keep the assertion focused but simpler:
```ts
// Before
class TestRepoClass extends RepoMock {}
unitOfWork = new MongoUnitOfWork(
eventBus,
integrationEventBus,
mockModel,
typeConverter,
TestRepoClass,
);
Then('...', () => {
expect(unitOfWork.repoClass).toBe(TestRepoClass);
});
// In scenario
expect(domainOperation).toHaveBeenCalledWith(expect.any(TestRepoClass));
```
```ts
// After – use the real class / instance
unitOfWork = new MongoUnitOfWork(
eventBus,
integrationEventBus,
mockModel,
typeConverter,
RepoMock,
);
Then('...', () => {
expect(unitOfWork.repoClass).toBe(RepoMock);
});
// Scenario
Then('the transaction is committed and no events are dispatched', () => {
expect(domainOperation).toHaveBeenCalledWith(repoInstance);
});
```
If you only care that a repo-like object is passed, you can also assert on shape:
```ts
expect(domainOperation).toHaveBeenCalledWith(
expect.objectContaining({ get: expect.any(Function), save: expect.any(Function) }),
);
```
### 3. Inline `setupMockModelReturningAggregate` / `setupRepoNoEvents` where used
These helpers hide straightforward setup that’s only relevant to a couple of scenarios.
```ts
// Before
function setupRepoNoEvents() {
repoInstance.getIntegrationEvents = vi.fn(() => []);
repoInstance.get = vi.fn().mockResolvedValue(
new AggregateRootMock(
{ id: 'agg-1', foo: 'old-foo', createdAt: new Date(), updatedAt: new Date(), schemaVersion: '1' } as PropType,
{},
),
);
repoInstance.save = vi.fn().mockResolvedValue(undefined);
}
Given('a domain operation that emits no domain or integration events', () => {
setupRepoNoEvents();
});
```
```ts
// After – explicit in the scenario
Given('a domain operation that emits no domain or integration events', () => {
repoInstance.getIntegrationEvents = vi.fn(() => []);
repoInstance.get = vi.fn().mockResolvedValue(
new AggregateRootMock(
{
id: 'agg-1',
foo: 'old-foo',
createdAt: new Date(),
updatedAt: new Date(),
schemaVersion: '1',
} as PropType,
{},
),
);
repoInstance.save = vi.fn().mockResolvedValue(undefined);
});
```
Same for `setupMockModelReturningAggregate` – the inline `findById` mocking is short and makes the test self-contained.
### 4. Use `integrationEventBus` directly instead of `dispatchMock`
You can keep the bus wiring simple and avoid an extra alias:
```ts
// Before
type DispatchMock = ReturnType<typeof vi.fn>;
let dispatchMock: DispatchMock;
dispatchMock = vi.fn() as DispatchMock;
integrationEventBus = {
dispatch: dispatchMock,
register: vi.fn(),
} as unknown as DomainSeedwork.EventBus;
When(..., async () => {
dispatchMock.mockClear();
dispatchMock.mockResolvedValueOnce(undefined);
});
// Then
expect(dispatchMock).toHaveBeenCalledTimes(2);
```
```ts
// After – mock the bus and assert on `integrationEventBus.dispatch`
integrationEventBus = vi.mocked({
dispatch: vi.fn(),
register: vi.fn(),
}) as DomainSeedwork.EventBus;
When(..., async () => {
(integrationEventBus.dispatch as MockInstance).mockClear();
(integrationEventBus.dispatch as MockInstance).mockResolvedValueOnce(undefined);
(integrationEventBus.dispatch as MockInstance).mockResolvedValueOnce(undefined);
});
// Then
expect(integrationEventBus.dispatch).toHaveBeenCalledTimes(2);
expect(integrationEventBus.dispatch).toHaveBeenNthCalledWith(
1,
event1.constructor,
event1.payload,
);
```
This keeps the tests close to the actual behavior (unit of work calling the event bus) without an extra mock layer.
---
Applying these changes retains all new behaviors (domain op mutating aggregate, adding events, dispatch semantics) but removes most of the type-level and helper indirection that makes the file harder to follow.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
Author
CI is not a predefined env var on Azure DevOps, so I removed it. |
Contributor
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- In the Storybook Vitest config,
isCIis now derived only fromTF_BUILD, which will skip CI-specific behavior (retries, timeouts, watch/fileParallelism) in other CI providers that rely on theCIenv var; consider keeping the previousCI || TF_BUILDcheck to avoid regressions. - Some of the new Vitest 4 constructor mocks (e.g. twilio, mongoose model ctors, exporter mocks) are wrapped in
vi.fn(function () { ... })with repeated inline biome ignores; you may want to factor out small helper factories for these to keep the tests less noisy and reduce duplication.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In the Storybook Vitest config, `isCI` is now derived only from `TF_BUILD`, which will skip CI-specific behavior (retries, timeouts, watch/fileParallelism) in other CI providers that rely on the `CI` env var; consider keeping the previous `CI || TF_BUILD` check to avoid regressions.
- Some of the new Vitest 4 constructor mocks (e.g. twilio, mongoose model ctors, exporter mocks) are wrapped in `vi.fn(function () { ... })` with repeated inline biome ignores; you may want to factor out small helper factories for these to keep the tests less noisy and reduce duplication.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
…-4x-and-fix-all-related-testing
Contributor
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The Storybook Vitest config now infers CI solely from
TF_BUILD === 'True', whereas it previously also respectedCI === 'true'; if any non-Azure environments run these tests, consider keepingCIin the check to avoid accidentally enabling watch/parallelism there. - Several repository tests (account plan, reservation request, conversation, user repositories) now repeat the
vi.fn(function() { return mockNewDoc; })constructor pattern; extracting a small shared helper for creating Vitest-4-compatible model constructors would reduce duplication and make the intent clearer.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The Storybook Vitest config now infers CI solely from `TF_BUILD === 'True'`, whereas it previously also respected `CI === 'true'`; if any non-Azure environments run these tests, consider keeping `CI` in the check to avoid accidentally enabling watch/parallelism there.
- Several repository tests (account plan, reservation request, conversation, user repositories) now repeat the `vi.fn(function() { return mockNewDoc; })` constructor pattern; extracting a small shared helper for creating Vitest-4-compatible model constructors would reduce duplication and make the intent clearer.
## Individual Comments
### Comment 1
<location> `packages/cellix/vitest-config/src/configs/storybook.config.ts:23-24` </location>
<code_context>
- },
- },
- }));
+ // https://learn.microsoft.com/en-us/azure/devops/pipelines/build/variables?view=azure-devops&tabs=yaml
+ const isCI = process.env['TF_BUILD'] === 'True';
- return mergeConfig(base, defineConfig({}));
</code_context>
<issue_to_address>
**question (bug_risk):** CI detection now ignores the standard `CI` env var, which may change behavior outside Azure DevOps.
The previous `isCI` logic also honored `process.env['CI']`, but the new version only checks `TF_BUILD`. In non-Azure environments (GitHub Actions, local `CI=true`, etc.), this will now behave as non-CI for retries, timeouts, fileParallelism, and watch. If that’s not intentional, please restore `CI` support, e.g.:
```ts
const isCI = process.env['TF_BUILD'] === 'True' || process.env['CI'] === 'true';
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- The new
@cellix/test-utilspackage is only used from test files, but it's added as a runtime dependency in@sthrift/service-oteland@sthrift/persistence(and even duplicated in bothdependenciesanddevDependenciesforservice-otel); consider moving it todevDependenciesonly wherever it’s used to avoid bloating production installs. - In
packages/cellix/test-utils/src/index.ts,makeNewableMockreturns a plain function; if you intend consumers to be able to inspect calls like a Vitest mock, you may want to wrap the implementation withvi.fn(or document that this helper does not provide spy/mocking behavior itself).
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `@cellix/test-utils` package is only used from test files, but it's added as a runtime dependency in `@sthrift/service-otel` and `@sthrift/persistence` (and even duplicated in both `dependencies` and `devDependencies` for `service-otel`); consider moving it to `devDependencies` only wherever it’s used to avoid bloating production installs.
- In `packages/cellix/test-utils/src/index.ts`, `makeNewableMock` returns a plain function; if you intend consumers to be able to inspect calls like a Vitest mock, you may want to wrap the implementation with `vi.fn` (or document that this helper does not provide spy/mocking behavior itself).
## Individual Comments
### Comment 1
<location> `packages/cellix/vitest-config/src/configs/storybook.config.ts:23-24` </location>
<code_context>
- },
- },
- }));
+ // https://learn.microsoft.com/en-us/azure/devops/pipelines/build/variables?view=azure-devops&tabs=yaml
+ const isCI = process.env['TF_BUILD'] === 'True';
- return mergeConfig(base, defineConfig({}));
</code_context>
<issue_to_address>
**suggestion:** CI detection now only checks TF_BUILD and may miss other CI environments setting CI=true.
The previous config handled both `CI` and `TF_BUILD` when adjusting retry/timeout/fileParallelism for CI runs. With `isCI` now only checking `TF_BUILD`, other CI providers (or local runs with `CI=true`) will be treated as non‑CI, dropping the stricter CI defaults. Please either include the `CI` env var in the check (e.g. `process.env.CI === 'true' || process.env.TF_BUILD === 'True'`) or clearly document that only Azure Pipelines are supported here.
```suggestion
// Generic CI detection (CI=true) and Azure Pipelines (TF_BUILD=True)
// https://learn.microsoft.com/en-us/azure/devops/pipelines/build/variables?view=azure-devops&tabs=yaml
const isCI =
process.env.CI === 'true' ||
process.env['TF_BUILD'] === 'True';
```
</issue_to_address>
### Comment 2
<location> `packages/sthrift/service-otel/package.json:38-39` </location>
<code_context>
"@opentelemetry/sdk-node": "0.57.2",
"@opentelemetry/sdk-trace-node": "1.30.1",
- "@opentelemetry/semantic-conventions": "1.28.0"
+ "@opentelemetry/semantic-conventions": "1.28.0",
+ "@cellix/test-utils": "workspace:*"
},
"devDependencies": {
</code_context>
<issue_to_address>
**issue:** The @cellix/test-utils dependency is declared in both dependencies and devDependencies.
`@cellix/test-utils` is only used in tests, so it should live solely in `devDependencies` rather than being duplicated in both sections, unless there’s a concrete runtime use case.
</issue_to_address>
### Comment 3
<location> `packages/cellix/mongoose-seedwork/src/mongoose-seedwork/mongo-unit-of-work.test.ts:35` </location>
<code_context>
class TestEvent extends DomainSeedwork.CustomDomainEventImpl<{ foo: string }> {}
+// Helper types and factories for domain operations
+type RepoOp = (repo: RepoMock) => Promise<void>;
+type RepoOpMock = RepoOp & MockInstance;
</code_context>
<issue_to_address>
**issue (complexity):** Consider simplifying the tests by inlining the new helper types/functions and mocks so each scenario’s flow is explicit without extra indirection layers.
You can keep the new behaviors (e.g. `repoClass` wiring and event assertions) but reduce indirection by inlining some of the abstractions you introduced.
### 1. Drop `RepoOp` / `RepoOpMock` and factories
The `RepoOp`/`RepoOpMock` types plus `makeDomainOpWithoutEvents`/`makeDomainOpWithEvents` add a lot of plumbing for what are essentially 2–3 inline operations.
You can keep the same behavior and assertions while simplifying to per-scenario inline functions:
```ts
// BeforeEachScenario
let domainOperation: ReturnType<typeof vi.fn>;
BeforeEachScenario(() => {
// ...
unitOfWork = new MongoUnitOfWork(
eventBus,
integrationEventBus,
mockModel,
typeConverter,
TestRepoClass,
);
domainOperation = vi.fn(async (repo: RepoMock) => {
const aggregate = await repo.get('agg-1');
aggregate.foo = 'new-foo';
await repo.save(aggregate);
});
vi.spyOn(mongoose.connection, 'transaction').mockImplementation(
async (cb: (session: ClientSession) => Promise<unknown>) =>
cb({} as ClientSession),
);
});
```
Then for scenarios that need events, define the domain op inline in the `Given`:
```ts
Scenario('Domain operation emits integration events, all dispatch succeed', ({ Given, When, Then }) => {
let event1: TestEvent;
let event2: TestEvent;
let customDomainOp: ReturnType<typeof vi.fn>;
Given('integration events are emitted during the domain operation', () => {
event1 = new TestEvent('id');
event1.payload = { foo: 'bar1' };
event2 = new TestEvent('id');
event2.payload = { foo: 'bar2' };
customDomainOp = vi.fn(async (repo: RepoMock) => {
const aggregate = await repo.get('agg-1');
aggregate.foo = 'new-foo';
aggregate.addIntegrationEvent(TestEvent, event1.payload);
aggregate.addIntegrationEvent(TestEvent, event2.payload);
await repo.save(aggregate);
});
});
When('the transaction completes successfully', async () => {
(integrationEventBus.dispatch as MockInstance).mockClear();
(integrationEventBus.dispatch as MockInstance).mockResolvedValueOnce(undefined);
(integrationEventBus.dispatch as MockInstance).mockResolvedValueOnce(undefined);
await unitOfWork.withTransaction(Passport, customDomainOp);
});
Then('all integration events are dispatched after the transaction commits', () => {
expect(integrationEventBus.dispatch).toHaveBeenCalledTimes(2);
// ...
});
});
```
This preserves the richer coverage and `TestRepoClass` usage while making the flow of each scenario explicit.
### 2. Inline `setupRepoNoEvents`
`setupRepoNoEvents` is only used in a couple of places and just wraps a short mock setup. Inlining keeps the behavior but removes one level of jumping:
```ts
Scenario('Domain operation with no events, completes successfully', ({ Given, When, Then }) => {
Given('a domain operation that emits no domain or integration events', () => {
repoInstance.getIntegrationEvents = vi.fn(() => []) as typeof repoInstance.getIntegrationEvents;
repoInstance.get = vi.fn().mockResolvedValue(
new AggregateRootMock(
{
id: 'agg-1',
foo: 'old-foo',
createdAt: new Date(),
updatedAt: new Date(),
schemaVersion: '1',
} as PropType,
{},
),
);
repoInstance.save = vi.fn().mockResolvedValue(undefined);
});
When('the operation completes successfully', async () => {
domainOperation.mockClear();
(integrationEventBus.dispatch as MockInstance).mockClear();
await unitOfWork.withTransaction(Passport, domainOperation);
});
Then('the transaction is committed and no events are dispatched', () => {
expect(domainOperation).toHaveBeenCalledWith(expect.any(TestRepoClass));
expect(integrationEventBus.dispatch).not.toHaveBeenCalled();
});
});
```
You keep the same repo behavior and `TestRepoClass` assertion, but readers don’t have to track a separate helper.
### 3. Assert directly on `integrationEventBus.dispatch`
The dedicated `DispatchMock` alias and extra variable aren’t strictly needed; you can still have a strongly-typed mock by working directly with `integrationEventBus.dispatch`:
```ts
let integrationEventBus: DomainSeedwork.EventBus;
BeforeEachScenario(() => {
integrationEventBus = vi.mocked({
dispatch: vi.fn(),
register: vi.fn(),
}) as DomainSeedwork.EventBus;
// ...
});
// In scenarios
When('...', async () => {
const dispatch = integrationEventBus.dispatch as MockInstance;
dispatch.mockClear();
dispatch.mockResolvedValueOnce(undefined);
// ...
});
Then('...', () => {
const dispatch = integrationEventBus.dispatch as MockInstance;
expect(dispatch).toHaveBeenCalledTimes(2);
});
```
This keeps the mock control localized but removes the extra alias and type alias.
### 4. Consider removing `setupMockModelReturningAggregate` if default
If the `BeforeEachScenario` already sets `mockModel.findById` to return the same aggregate shape, the extra `setupMockModelReturningAggregate` call can be removed entirely. If you do need to override it for a specific scenario, keep that override next to the scenario instead of in a shared helper.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
…-4x-and-fix-all-related-testing
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Migrate the monorepo test stack to Vitest 4 and stabilize browser/Storybook testing and coverage collection across services and UI packages.
Enhancements:
Build:
CI:
Tests:
Chores: