From 70a0b620981092a5e705c5886975b15ebacdb7a6 Mon Sep 17 00:00:00 2001 From: jamlen Date: Wed, 12 Aug 2026 22:05:48 +0100 Subject: [PATCH] feat!: constrain stub method names to keyof T MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #127. stub(methodNames) looked like it constrained names to keyof T but did not. Two overloads were reachable and the second accepted any string[], so a typo compiled and produced a mock silently missing that method: stub(['get', 'sve']) // compiled; no save() on the stub The name-list overload is now the single checked form, and is declared before the object overload rather than after it. Three defects fixed: 1. Names are checked against keyof T, so a typo is a compile error and TypeScript suggests the intended name. 2. Overload order. An array is also an object, so stub(['query','close']) with no type argument bound T to string[] and keyed the facades off Array members — setup.length, setup.push. The names now infer into the mock's shape, so setup.query and expect.close type-check. 3. readonly / as const name lists are accepted; they failed before. Uses overload reordering rather than excluding arrays from the object overload with a conditional type. Both constrain correctly, but the conditional type breaks generic helpers that wrap stub(): function makeMock(o: U) { return stub(o) } Verified against tsc 6.0.3. A regression guard lives in test/type-check.ts, which is the only test file tsconfig compiles — a guard in test/types.test.ts would never run in CI. Exhaustiveness is deliberately not enforced. The check is that supplied names exist on T, not that all of T is covered; partial stubs are a supported pattern used by test/type-check.ts itself. Runtime behaviour is unchanged. BREAKING CHANGE: stub(methodNames) no longer accepts a value typed string[]. Correct literal lists are unaffected. Lists built at runtime must opt out explicitly with stub(names as (keyof T)[]). Code relying on the old stub(['a','b']) inference of Wrapped will surface type errors, since the mock is now typed from the names. Co-Authored-By: Claude Opus 5 (1M context) --- docs/next/ai/common-mistakes.md | 58 +++++++++++++++++++++++++++++++ docs/next/ai/decision-tree.md | 7 +++- docs/next/api/stub.md | 26 +++++++++++--- docs/next/guide/creating-mocks.md | 39 ++++++++++++++++++++- docs/next/guide/migrating.md | 47 +++++++++++++++++++++++++ docs/next/guide/typescript.md | 21 +++++++++++ src/stub.ts | 31 +++++++++++------ test/stub.test.ts | 37 ++++++++++++++++++++ test/type-check.ts | 56 +++++++++++++++++++++++++++++ 9 files changed, 305 insertions(+), 17 deletions(-) diff --git a/docs/next/ai/common-mistakes.md b/docs/next/ai/common-mistakes.md index c40135d..379dc70 100644 --- a/docs/next/ai/common-mistakes.md +++ b/docs/next/ai/common-mistakes.md @@ -328,3 +328,61 @@ mock.expect.greet.called.withArg('alice') // separate arg check ``` **Why:** Negated count methods (`once()`, `twice()`, `times()`, `lt()`, `gt()`, etc.) are terminal — they return `void`. If chaining were allowed, each link would be negated independently (De Morgan), producing surprising results: `not.once()` passes but `not.withArg('alice')` fails even though the user intended "was not called exactly once with alice". Use two separate assertions instead. + +## 15. Casting a `string[]` to silence the `stub` name check + +**❌ Wrong** + +```typescript +stub(['query', 'findByID'] as (keyof Database)[]) +// Compiles — and the mock has no findById. The cast suppressed the typo. +``` + +**✅ Right** + +```typescript +stub(['query', 'findById']) +// Literal array — every name is checked against keyof Database +``` + +**Why:** `stub(methodNames)` constrains names to `keyof T`, so a typo or a +renamed method is a compile error. Casting the literal array throws that away +and reinstates the exact bug the check exists to catch — the stub is missing +the method and you find out at runtime, if at all. + +The cast is only correct when the list genuinely is not known statically: + +```typescript +const names: string[] = readMethodsFromConfig() +stub(names as (keyof Database)[]) // nothing to check against +``` + +If you find yourself casting a literal, the name is wrong — fix the name. + +## 16. Assuming a name list keeps up with the interface + +**❌ Wrong** + +```typescript +interface Gateway { + get(): Promise + save(patch: Patch): Promise // added later +} +const gw = stub(['get']) +await gw.save(patch) // typed fine — undefined at runtime +``` + +**✅ Right** + +```typescript +const gw = stub(GatewayImpl) // surface derived from the class +``` + +**Why:** The name check verifies that the names you supplied *exist* on `T`. It +does not verify that you supplied *all* of them — partial stubs are a +deliberate, supported pattern. So a method added to the interface later is +still missing from the mock, while `Wrapped` continues to claim it exists. + +Deriving the surface from a class or a real instance removes the failure mode +entirely. Reach for a name list only when there is no class or instance to +derive from. diff --git a/docs/next/ai/decision-tree.md b/docs/next/ai/decision-tree.md index 4a2c734..bad99a3 100644 --- a/docs/next/ai/decision-tree.md +++ b/docs/next/ai/decision-tree.md @@ -6,7 +6,8 @@ Which API to reach for, by task. Tables, not prose. If your question isn't answe | What you have | Factory | Example | |---------------|---------|---------| -| A TypeScript interface or type, no instance | `stub(['method1', 'method2'])` | `stub(['query'])` | +| A TypeScript interface or type, no instance | `stub(['method1', 'method2'])` — names are checked against `keyof T` | `stub(['query'])` | +| A method name list built at runtime | `stub(names as (keyof T)[])` | `stub(names as (keyof Database)[])` | | An existing object instance | `stub(obj)` | `stub(new Logger())` | | A class (want prototype methods auto-discovered) | `stub(MyClass)` | `stub(Greeter)` | | A class (want static methods instead) | `stub(MyClass, undefined, { debug:{prefix:'deride',suffix:'stub'}, static:true })` | `stub(Greeter, undefined, {…, static:true})` | @@ -16,6 +17,10 @@ Which API to reach for, by task. Tables, not prose. If your question isn't answe | A brand-new standalone function from scratch | `func()` | `func<(x: number) => number>()` | **Rules of thumb:** +- Prefer `stub(MyClass)` or `stub(obj)` over a name list whenever a class or + instance is available — the surface is derived, so it cannot drift out of + sync when a method is added or renamed. A name list only checks the names you + wrote, not that you wrote all of them. - `stub` replaces, `wrap` preserves real behaviour until overridden. - `stub.class` is only for `new`-call interception. If you control the call site, inject a `stub(...)` instance instead. - Use `func()` when the dependency is **itself** a function (callback, handler, fetcher). diff --git a/docs/next/api/stub.md b/docs/next/api/stub.md index ee8dc93..4c04661 100644 --- a/docs/next/api/stub.md +++ b/docs/next/api/stub.md @@ -6,25 +6,41 @@ Build a test double from method names, an object, or a class. ```typescript stub(cls: new (...args: never[]) => I): Wrapped -stub(obj: T): Wrapped -stub(methodNames: (keyof T)[]): Wrapped -stub( - methodNames: string[], +stub>( + methodNames: readonly (keyof T)[], properties?: { name: PropertyKey; options: PropertyDescriptor }[], options?: StubOptions ): Wrapped +stub(obj: T): Wrapped ``` +::: warning Changed in v3 +The method-name list is now constrained to `keyof T`. A typo or a stale name is +a compile error instead of a mock that is silently missing the method. See +[Migrating](../guide/migrating#method-name-lists-are-checked-against-keyof-t). +::: + ## Parameters ### `target` (first arg) Can be: -- **Array of method names** — TypeScript inference drives typing via `stub([...])`. +- **Array of method names** — every name must be a key of `T`. With no explicit + `T`, the names are inferred into the mock's shape, so `stub(['query'])` gives + you `setup.query` rather than a mock typed as `string[]`. `readonly` and + `as const` arrays are accepted. - **Existing object** — all own + inherited function-typed keys become stubbed methods. - **Class constructor** — walks the prototype chain (not statics, unless `{ static: true }`). +Names assembled at runtime widen to `string[]` and cannot be checked against +`keyof T`. Assert the intent explicitly at the call site: + +```typescript +const names: string[] = readMethodsFromSomewhere() +stub(names as (keyof Database)[]) +``` + ### `properties` (optional second arg) Array of `{ name, options }` descriptors to attach non-method fields. Useful for interfaces that mix methods and data: diff --git a/docs/next/guide/creating-mocks.md b/docs/next/guide/creating-mocks.md index 4ac3e28..8571414 100644 --- a/docs/next/guide/creating-mocks.md +++ b/docs/next/guide/creating-mocks.md @@ -27,7 +27,44 @@ const mockDb = stub(['query', 'findById']) mockDb.setup.query.toResolveWith([]) ``` -Methods default to returning `undefined` until configured. Every method on `T` must appear in the array; TypeScript checks it. +Methods default to returning `undefined` until configured. + +Every name is checked against `keyof T`, so a typo or a renamed method is a +compile error: + +```typescript +stub(['query', 'findByID']) +// Type '"findByID"' is not assignable to type 'keyof Database'. +// Did you mean '"findById"'? +``` + +The check is that each name you supply *exists* on `T` — not that you supplied +all of them. Listing a subset is fine and often what you want: + +```typescript +const mockDb = stub(['query']) // findById intentionally omitted +``` + +The flip side is that a method added to `Database` later is silently missing +from the mock, while the type still claims it exists. Where a class or a real +instance is available, prefer [`stub(MyClass)`](#stub-from-a-class) or +[`stub(obj)`](#stub-from-an-existing-object) — those derive the surface, so it +cannot drift. + +A list built at runtime widens to `string[]` and cannot be checked. Assert it +explicitly: + +```typescript +const names: string[] = readMethodsFromConfig() +stub(names as (keyof Database)[]) +``` + +With no type argument at all, the names are inferred into the mock's shape: + +```typescript +const mock = stub(['query', 'close']) +mock.setup.query.toReturn('rows') // type-checks +``` ### With extra properties diff --git a/docs/next/guide/migrating.md b/docs/next/guide/migrating.md index 37576e2..45efb09 100644 --- a/docs/next/guide/migrating.md +++ b/docs/next/guide/migrating.md @@ -2,6 +2,53 @@ Common patterns from other mocking libraries, translated to deride. +## Upgrading from v2 to v3 + +### Method name lists are checked against `keyof T` + +`stub(methodNames)` now constrains every name to `keyof T`. Previously a +permissive `string[]` overload accepted anything, so a typo compiled silently +and produced a mock missing that method — the failure only surfaced at runtime, +when something called it. + +```typescript +interface AccountGateway { + get(): Promise + save(patch: AccountPatch): Promise +} + +stub(['get', 'sve']) +// v2: compiles. The stub has no save(). +// v3: Type '"sve"' is not assignable to type 'keyof AccountGateway'. +// Did you mean '"save"'? +``` + +Correct name lists are unaffected. Two things changed that may need action: + +**Name lists built at runtime.** A value typed `string[]` can no longer be +checked, so it is rejected. Assert the intent at the call site: + +```typescript +const names: string[] = readMethodsFromConfig() + +stub(names) // v3: compile error +stub(names as (keyof Database)[]) // explicit opt-out +``` + +Only do this when the list genuinely is not known statically. Casting a literal +array reinstates the bug this change exists to catch. + +**Mocks built without a type argument.** `stub(['query', 'close'])` used to +resolve to `Wrapped`, so the facades keyed off `Array` members — +`setup.length`, `setup.push` — instead of your method names. It now infers the +names into the mock's shape, so `setup.query` and `expect.close` type-check. +This is a fix, but it will surface real type errors in code that was relying on +the old, wrong inference. + +`readonly` and `as const` name lists are now accepted, which previously failed. + +Nothing about runtime behaviour changed — this is a type-level change only. + ## From sinon ### `sinon.stub` diff --git a/docs/next/guide/typescript.md b/docs/next/guide/typescript.md index da940ee..c90c1ae 100644 --- a/docs/next/guide/typescript.md +++ b/docs/next/guide/typescript.md @@ -23,6 +23,27 @@ The type `Wrapped` is inferred automatically. You can annotate explicit const mock: Wrapped = stub(['fetch', 'process']) ``` +The name list is constrained to `keyof Service`, so the compiler catches a +mistyped or stale method name — including suggesting the intended one: + +```typescript +stub(['fetch', 'proccess']) +// Type '"proccess"' is not assignable to type 'keyof Service'. +// Did you mean '"process"'? +``` + +Names produced at runtime widen to `string[]` and cannot be checked against +`keyof Service`; `stub(names as (keyof Service)[])` opts out +explicitly. See [Common mistakes](../ai/common-mistakes) for when that cast is +and isn't appropriate. + +Omitting the type argument infers the mock's shape from the names themselves: + +```typescript +const mock = stub(['fetch', 'process']) +mock.setup.fetch.toReturn('response') // ✓ — keys come from the name list +``` + ## `toResolveWith` unwraps `Promise` For async methods, `toResolveWith(v)` expects the resolved type — not the Promise itself: diff --git a/src/stub.ts b/src/stub.ts index 5bcef5e..4cef187 100644 --- a/src/stub.ts +++ b/src/stub.ts @@ -72,7 +72,7 @@ function staticMethods(ctor: AnyCtor): string[] { return out } -function isMethodNameArray(target: unknown): target is (string | PropertyKey)[] { +function isMethodNameArray(target: unknown): target is readonly PropertyKey[] { return Array.isArray(target) && target.every((v) => typeof v === 'string' || typeof v === 'symbol' || typeof v === 'number') } @@ -81,24 +81,35 @@ function isMethodNameArray(target: unknown): target is (string | PropertyKey)[] * * Overloads: * - `stub(MyClass)` — auto-discovers methods from the class prototype chain. + * - `stub(['method1', 'method2'], properties?, options?)` — explicit list of + * method names, constrained to `keyof T` when `T` is supplied. `properties` + * adds property descriptors; `options` takes e.g. `{ static: true }` to mock + * a class's static side. * - `stub(existingObject)` — auto-discovers methods from an instance. - * - `stub(['method1', 'method2'])` — explicit list of method names. - * - `stub(methodNames, properties, options)` — with property descriptors and - * additional options (e.g. `{ static: true }` to mock a class's static side). * * Pass `{ static: true }` alongside a class target to stub the static methods * instead of the prototype methods. + * + * @remarks + * The name list is checked against `keyof T`, so a typo or a renamed method is + * a compile error rather than a mock that is silently missing the method. Names + * built at runtime are widened to `string[]` and cannot be checked — assert the + * intent explicitly with `stub(names as (keyof T)[])`. + * + * The method-name overload deliberately precedes the object overload: an array + * is also an `object`, so were the order reversed `stub(['a', 'b'])` would bind + * `T` to `string[]` and key the facades off `Array` members instead of the + * supplied names. */ export function stub(cls: new (...args: never[]) => I): Wrapped -export function stub(obj: T): Wrapped -export function stub(methodNames: (keyof T)[]): Wrapped -export function stub( - methodNames: string[], +export function stub>( + methodNames: readonly (keyof T)[], properties?: StubPropertyDescriptor[], options?: StubOptions ): Wrapped +export function stub(obj: T): Wrapped export function stub( - target: T | (keyof T)[] | string[] | AnyCtor, + target: T | readonly (keyof T)[] | readonly string[] | AnyCtor, properties?: StubPropertyDescriptor[], options: StubOptions = { debug: { prefix: PREFIX, suffix: 'stub' } } ): Wrapped { @@ -155,7 +166,7 @@ stub.class = function stubClass( { construct(_target, args) { constructorMock.invoke(args, undefined) - const instance = stub>(methodNames as string[]) as Wrapped> + const instance = stub>(methodNames as (keyof InstanceType)[]) as Wrapped> instances.push(instance) for (const s of setups) s(instance) return instance as unknown as object diff --git a/test/stub.test.ts b/test/stub.test.ts index 11ea2f2..3dd497d 100644 --- a/test/stub.test.ts +++ b/test/stub.test.ts @@ -104,3 +104,40 @@ describe('stub.class()', () => { expect(MockedGreeter.instances).toEqual([a, b]) }) }) + +describe('stub(methodNames) — name list forms (issue #127)', () => { + interface Db { + query(sql: string): string + close(): void + } + + it('stubs every listed method', () => { + const db = stub(['query', 'close']) + db.setup.query.toReturn('rows') + expect(db.query('select 1')).toBe('rows') + db.close() + db.expect.close.called.once() + }) + + it('accepts a readonly / as const name list', () => { + const names = ['query', 'close'] as const + const db = stub(names) + db.setup.query.toReturn('rows') + expect(db.query('select 1')).toBe('rows') + }) + + it('only stubs the names listed — omitted methods are absent', () => { + const db = stub(['query']) + expect(typeof db.query).toBe('function') + expect((db as unknown as Record).close).toBeUndefined() + }) + + it('infers the mock shape from the names when no type argument is given', () => { + const mock = stub(['query', 'close']) + mock.setup.query.toReturn('rows') + expect(mock.query()).toBe('rows') + // The array overload must win over the object overload, otherwise the + // facades key off Array members rather than the supplied names. + expect((mock.setup as unknown as Record).push).toBeUndefined() + }) +}) diff --git a/test/type-check.ts b/test/type-check.ts index 8f28638..29975f5 100644 --- a/test/type-check.ts +++ b/test/type-check.ts @@ -102,3 +102,59 @@ expectSvc.expect.greet.not.called.withArg('nobody').withReturn('nope') // VALID: deprecated called.not path still type-checks (issue #109 review §4) expectSvc.expect.greet.called.not.withArg('nobody') expectSvc.expect.greet.called.not.twice() + +// ── stub(methodNames) constrains names to keyof T (issue #127) ── + +// VALID: every name is a method of MyService +deride.stub(['greet', 'fetchData']) + +// INVALID: 'fetchDat' is a typo — must not silently fall through to a +// permissive string[] overload. This is the whole point of issue #127. +// @ts-expect-error: "fetchDat" is not assignable to keyof MyService +deride.stub(['greet', 'fetchDat']) + +// VALID: a subset is still allowed — partial stubs are a supported pattern +deride.stub(['greet']) + +// VALID: readonly / `as const` arrays are accepted +deride.stub(['greet', 'fetchData'] as const) + +// INVALID: a widened string[] cannot be checked against keyof T +const dynamicNames: string[] = ['greet', 'fetchData'] +// @ts-expect-error: string is not assignable to keyof MyService +deride.stub(dynamicNames) + +// VALID: documented escape hatch for dynamically built name lists +deride.stub(dynamicNames as (keyof MyService)[]) + +// VALID: with no explicit T, names are inferred into the mock's shape — +// the array overload must win over the object overload, or T infers as +// string[] and the facades key off Array members instead of the names. +const inferred = deride.stub(['query', 'close']) +inferred.setup.query.toReturn(1) +inferred.expect.query.called.once() +inferred.spy.close.callCount satisfies number + +// INVALID: a name that was never listed is not on the inferred mock +// @ts-expect-error: 'missing' was not in the method name list +inferred.setup.missing.toReturn(1) + +// VALID: class and object forms are unaffected by the array overload order +class RealService { + greet(_name: string): string { + return '' + } + fetchData(_url: string): Promise<{ id: number }> { + return Promise.resolve({ id: 1 }) + } +} +deride.stub(RealService).setup.greet.toReturn('hi') +const realObj = { greet: (_n: string): string => '' } +deride.stub(realObj).setup.greet.toReturn('hi') + +// VALID: generic helpers wrapping stub() still infer (regression guard — +// a conditional-type object overload breaks this case) +function makeMock(target: U) { + return deride.stub(target) +} +makeMock(new RealService()).setup.greet.toReturn('hi')