Skip to content

Migrate cast-based class mocks in specs to createMockInstance #492

Description

@vincent-psarga

Context

The mockInterface migration (#483, then #487 / #488 / #491) is removing as unknown as jest.Mocked<IPort> casts from spec files, package by package. Those PRs deliberately leave class mocks alone, because they belong to a different utility — createMockInstance — and mixing the two would have made each diff much harder to review.

This issue tracks that second track so it doesn't get lost.

The anti-pattern

Identical in shape to the interface one, but the target is a class:

mockStandardService = {
  getStandardById: jest.fn(),
} as unknown as jest.Mocked<StandardService>;   // 1 of N methods

The as unknown as double cast switches off the structural check that the spec typecheck pass (tsc --noEmit --project <pkg>/tsconfig.spec.json) relies on, so an object literal implementing one method still claims to be the whole class.

createMockInstance (packages/test-utils/src/createMockInstance.ts) replaces it:

mockStandardService = createMockInstance(StandardService);
mockStandardService.getStandardById.mockResolvedValue(standard);

It walks the class prototype at runtime and backs every method with a jest.fn(), so the mock is complete by construction and needs no cast.

Inventory

190 non-interface as unknown as jest.Mocked<…> casts across 47 distinct targets (counted on main at a514337). Roughly 182 are genuine class targets; the other ~8 are TypeORM/DOM types (see "Out of scope" below).

Project Casts Spec typecheck enforced?
packages/deployments 65 ✅
packages/accounts 42 ✅
packages/standards 21 ✅
packages/skills 19 ✅
apps/cli 11 ❌
apps/api 11 ❌
packages/commands 8 ✅
packages/spaces 7 ✅
packages/node-utils 3 ✅
packages/coding-agent 2 ✅
packages/test-utils 1 ✅

Most-mocked targets:

Target Sites
PackmindEventEmitterService 29
UserService 18
PackageService 14
RenderModeConfigurationService 12
TargetService 9
SkillService 8
PackmindCliHexa 8
StandardService 7
OrganizationService 7
DeploymentsServices 6

The remaining 37 targets have 1–5 sites each.

createMockInstance is currently used in exactly one spec repo-wide (packages/skills/src/application/useCases/uploadSkill/UploadSkillUseCase.spec.ts), so in raw volume this is the larger untapped win of the two tracks.

How we should handle them

Convention

Same one settled in #487 and used since: construct, then stub.

const service = createMockInstance(StandardService);
service.getStandardById.mockResolvedValue(standard);

Not the pre-seeded-literal form. Both are type-checked equally; post-construction stubbing is what the per-test overrides (mockResolvedValueOnce, mockRejectedValue) already use, and it keeps evaluation timing identical to the old jest.fn().mockResolvedValue(x).

Per package, the loop that worked for mockInterface

  1. Swap the casts mechanically.
  2. Run nx typecheck <pkg> — this is where the value is. Every package migrated so far surfaced real fixture drift the cast had been hiding (♻️ refactor(deployments): migrate port mocks to mockInterface #488 found a stub for IAccountsPort.isMemberOf, a method that exists nowhere in the codebase; ♻️ refactor(standards): migrate port mocks to mockInterface #491 found every findMembership stub in standards was missing required UserSpaceMembership fields). Expect the same here and budget review time for it.
  3. nx test <pkg> and nx lint <pkg>.
  4. One commit per package, one PR per package — the diffs are large and uniform, and reviewers need the drift fixes separated from the mechanical swap.

Suggested order, largest payoff first: deployments → accounts → standards → skills → commands / spaces / node-utils / coding-agent.

Known constraints of createMockInstance

Worth knowing before starting; none of them block the current targets, all of which were checked:

  • It walks Object.getOwnPropertyNames(cls.prototype) — own properties only, so methods inherited from a base class are not mocked. PackmindEventEmitterService extends BaseService, but BaseService declares only abstract members and a constructor, so it contributes nothing at runtime and this is a non-issue today. It would bite if a base class ever gained a concrete method.
  • It only mocks members whose descriptor value is a function, so getters and arrow-function class fields (foo = () => {}) are skipped. Checked across the 13 most-mocked targets: all use plain prototype methods.
  • It takes the class itself as an argument, so the spec must import the real class — fine for these, but it does mean a spec can't mock a class it only knows by type.

If a future target trips one of the first two, the mock will be silently incomplete rather than failing loudly — a strictness option (like mockInterface's { strict: true }) would be the fix.

Out of scope

~8 of the 190 are not classes and should be left as hand-written mocks:

  • Repository<Distribution>, Repository<TestEntity>, SelectQueryBuilder<…> — TypeORM types. mockInterface's own docstring says a mostly-data type is better mocked by hand, and the same reasoning applies.
  • Response, and one bare T in a generic helper.

apps/cli and apps/api (22 casts) do not run the spec typecheck pass, so migrating them buys nothing until that's added to their project.json — worth doing, but as its own change.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions