Repository navigation
Conversation
| glean: DeepMocked<GleanMetricsType>, | ||
| routes: ReturnType<typeof passkeyRoutes>, | ||
| route: ReturnType<typeof passkeyRoutes>[number] | undefined, | ||
| request: AuthRequest & |
There was a problem hiding this comment.
I think this intersection doesn't buy anything: these are already typed any on AuthRequest, and any & jest.Mock collapses back to any. request: AuthRequest should compile identically and reads easier. (The real fix is typing those four fields in types.ts, but that's a different PR.)
|
|
||
| await expect( | ||
| route.options.response.schema.validateAsync(result) | ||
| ( |
There was a problem hiding this comment.
These inline option casts repeat the same shape seven times. Would one typed accessor near runTest do?
type TestRouteOptions = {
auth: { strategies: string[] };
validate: { payload: Schema; params: Schema };
response: { schema: Schema };
};
const optionsOf = (r: typeof route) => {
if (!r) throw new Error('route not found');
return r.options as TestRouteOptions;
};
// optionsOf(route).validate.payloadThat also removes the route?. optional chains since the helper narrows. I looked at whether annotating passkeyRoutes as ServerRoute[] would make the cast unnecessary and it wouldn't. Hapi types options as RouteOptions | (server) => RouteOptions and auth as false | string | RouteOptionsAccess, so you'd still narrow.
| }, | ||
| }; | ||
| return route.handler(request); | ||
| return route.handler(request as unknown as AuthRequest); |
There was a problem hiding this comment.
The as unknown as AuthRequest accepts any object, so a typo like credentails passes type-check and surfaces as a confusing runtime failure. Could typing the literal against golevelup's deep partial first do the job here and elsewhere?
import type { PartialFuncReturn } from '@golevelup/ts-jest';
const asRequest = (r: PartialFuncReturn<AuthRequest>) => r as AuthRequest;Because: - The passkey specs used `any` for mocks, requests and route options, so tsc did not check them. This commit: - Builds the passkeys.spec.ts mocks as DeepMocked<T> with createMock<T>. - Types found routes with an optionsOf helper and requests with asRequest. - Passes the missing PasskeyHandler config argument. - Uses createMock<StatsD>() and createMock<LoggerService>() in the remote specs. - Uses `as unknown as` casts in passkey.challenge.manager.spec.ts. Closes FXA-13338
c3e349d to
9331133
Compare
|
🤖 This review round pushed a new commit:
|
Because
anyfor mocks, routes, and requests, sotsccould not check them.This pull request
passkeys.spec.ts: the mocks are nowDeepMocked<T>built withcreateMock<T>. Every existing return value stays.passkey-wraps.spec.ts: aRoutealias types the found routes.as Schematypes the validate schemas.passkeys.in.spec.tsandpasskey_wraps.in.spec.ts:createMock<StatsD>()andcreateMock<LoggerService>()replace the hand-built mocks. Event callbacks use(e: { name: string }).passkey.challenge.manager.spec.ts: usesas unknown as Redisandas unknown as StatsD. This adds no dependency tolibs/accounts.runTestinpasskeys.spec.tsnow throws when a route is not found. It has a generic return type.PasskeyHandlerconstructor call now passes the missing 10thconfigargument. This was atscerror on main.as jest.Mock. Their test values break the declared type on purpose: an unknown transport and two partial accounts.Issue that this pull request solves
Closes: FXA-13338
Checklist
Put an
xin the boxes that applyHow to review (Optional)
passkeys.spec.ts, where the mocks move tocreateMock.passkeys.spec.ts,passkey-wraps.spec.ts, the two remote specs, thenpasskey.challenge.manager.spec.ts.createMockauto-mocks any method that a test does not stub. A new unstubbed call no longer throws aTypeError. Every existing test passes unchanged. No test failed after the move tocreateMock.Screenshots (Optional)
None. There is no user-visible change, so there is no functional test or screenshot.
Other information (Optional)
anyremains only on the JS test client holders in the two remote specs. This includes thegetMfaAccessTokenForPasskey(clientInstance: any)parameter, which holds the same client..preis read inpasskey-wraps.spec.ts. The file compiles without it, so I added none./fxa-verify --run --plan:passkeys.spec.tsandpasskey-wraps.spec.ts: 212 passed.passkey.challenge.manager.spec.ts: 14 passed.test/remote/passkeys.in.spec.tsandpasskey_wraps.in.spec.ts: 25 passed. MySQL and Redis run in the sandbox.tsc --noEmit -p packages/fxa-auth-server/tsconfig.jsonand-p libs/accounts/passkey/tsconfig.spec.json: no errors in the five files.