From 719f62f102edcfdf1e2ebfea2f47713f1f0218fe Mon Sep 17 00:00:00 2001 From: Zoltan Kochan Date: Mon, 17 Aug 2026 02:19:30 +0200 Subject: [PATCH] test: type the install option helpers so a stale key cannot pass (#13951) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A test's options object is the entire input to the subject under test, so a key nothing reads does not fail: it quietly runs the default instead of the case the test is named for. Both install helpers accepted anything — the headless one took `opts?: any`, and the install one takes `opts?: T & {...}`, where an unknown key is absorbed into the inferred `T`. That is how the `registries` -> `registriesByScope` rename left seven suites pointed at the wrong registry with a green type-check, and only CI to say so. The headless helper now takes a `Partial`. Everything that surfaced was real: - Seven fields `HeadlessOptions` requires and reads — `configByUri`, `globalVirtualStoreDir`, `pruneStore`, `sideEffectsCacheRead`/`Write`, `userAgent`, `virtualStoreDirMaxLength` — were never set, so every headless test ran with them `undefined`. They now carry the values the install behaved as. - `verifyStoreIntegrity` is a package-store option, not a headless one. The three tests that "disable" it were setting a key nobody read; it is now forwarded to the store. - `hoistPattern: '*'` and `publicHoistPattern: '*'` are `string[]`. A one-character string iterates like a one-element array, which is the only reason they worked. - `development`, `optional`, `production` predate `include`, which the same calls already set, and `sideEffectsCache` predates the read/write split. Constraining the install helper's `T` is a bigger job — 162 errors, mostly unrelated to options keys — so it keeps its generic and gains a narrow guard instead: the three renamed keys are declared with a literal type that names the replacement. It caught one more live case on the way in. --- .changeset/typed-test-option-helpers.md | 5 ++ .../deps-installer/test/lockfile.ts | 2 +- .../deps-installer/test/utils/testDefaults.ts | 15 +++++ pnpm11/installing/deps-restorer/test/index.ts | 15 ++--- .../deps-restorer/test/utils/testDefaults.ts | 56 ++++++++++++++----- pnpm11/testing/temp-store/src/index.ts | 2 +- 6 files changed, 71 insertions(+), 24 deletions(-) create mode 100644 .changeset/typed-test-option-helpers.md diff --git a/.changeset/typed-test-option-helpers.md b/.changeset/typed-test-option-helpers.md new file mode 100644 index 0000000000..7fdf6352e5 --- /dev/null +++ b/.changeset/typed-test-option-helpers.md @@ -0,0 +1,5 @@ +--- +"@pnpm/testing.temp-store": patch +--- + +`createTempStore`'s `storeOptions` are typed as a partial, which is how they are used: they are spread over the store's own defaults, so a caller that overrides one knob no longer has to restate the other five. diff --git a/pnpm11/installing/deps-installer/test/lockfile.ts b/pnpm11/installing/deps-installer/test/lockfile.ts index 010de8a569..b43c75bff5 100644 --- a/pnpm11/installing/deps-installer/test/lockfile.ts +++ b/pnpm11/installing/deps-installer/test/lockfile.ts @@ -466,7 +466,7 @@ test('scoped module from different registry', async () => { '@zkochan': `http://localhost:${REGISTRY_MOCK_PORT}`, '@foo': `http://localhost:${REGISTRY_MOCK_PORT}`, } - await addDependenciesToPackage({}, ['@zkochan/foo', '@foo/has-dep-from-same-scope', 'is-positive'], testDefaults({ registries }, { registries })) + await addDependenciesToPackage({}, ['@zkochan/foo', '@foo/has-dep-from-same-scope', 'is-positive'], testDefaults({ registriesByScope: registries }, { registriesByScope: registries })) project.has('@zkochan/foo') diff --git a/pnpm11/installing/deps-installer/test/utils/testDefaults.ts b/pnpm11/installing/deps-installer/test/utils/testDefaults.ts index 9d5cb3cc57..261ceb1582 100644 --- a/pnpm11/installing/deps-installer/test/utils/testDefaults.ts +++ b/pnpm11/installing/deps-installer/test/utils/testDefaults.ts @@ -19,6 +19,21 @@ export function testDefaults ( minimumReleaseAge?: number minimumReleaseAgeStrict?: boolean minimumReleaseAgeExclude?: string[] + /** + * Renamed to `registriesByScope`, and kept here so an options object that + * still carries the old key fails to compile: `T` is inferred from the + * argument, so an unknown key is otherwise absorbed into it and the test + * silently exercises the default registry instead. + * + * Typed as the replacement's name rather than `never` so the compiler + * prints the fix — `not assignable to type '… & "renamed: use + * registriesByScope"'`. + */ + registries?: 'renamed: use registriesByScope' + /** Renamed to `registriesByPrefix`. See `registries` above. */ + namedRegistries?: 'renamed: use registriesByPrefix' + /** Renamed to `registryOptionsByUrl`. See `registries` above. */ + registryOptions?: 'renamed: use registryOptionsByUrl' }, resolveOpts?: any, // eslint-disable-line fetchOpts?: any, // eslint-disable-line diff --git a/pnpm11/installing/deps-restorer/test/index.ts b/pnpm11/installing/deps-restorer/test/index.ts index 22bbfb7a21..bdaa345522 100644 --- a/pnpm11/installing/deps-restorer/test/index.ts +++ b/pnpm11/installing/deps-restorer/test/index.ts @@ -301,15 +301,12 @@ test('installing only optional deps', async () => { const prefix = f.prepare('simple') await headlessInstall(await testDefaults({ - development: false, include: { dependencies: false, devDependencies: false, optionalDependencies: true, }, lockfileDir: prefix, - optional: true, - production: false, })) const project = assertProject(prefix) @@ -536,7 +533,7 @@ test('installing using passed in lockfile files', async () => { await headlessInstall(await testDefaults({ lockfileDir: prefix, - wantedLockfile, + wantedLockfile: wantedLockfile ?? undefined, })) const project = assertProject(prefix) @@ -570,7 +567,7 @@ test('installing with hoistPattern=*', async () => { const prefix = prepareFixtureWithIntegrity('simple-shamefully-flatten') const reporter = jest.fn() - await headlessInstall(await testDefaults({ lockfileDir: prefix, reporter, hoistPattern: '*' })) + await headlessInstall(await testDefaults({ lockfileDir: prefix, reporter, hoistPattern: ['*'] })) const project = assertProject(prefix) expect(project.requireModule('is-positive')).toBeTruthy() @@ -629,7 +626,7 @@ test('installing with publicHoistPattern=*', async () => { const prefix = prepareFixtureWithIntegrity('simple-shamefully-flatten') const reporter = jest.fn() - await headlessInstall(await testDefaults({ lockfileDir: prefix, reporter, publicHoistPattern: '*' })) + await headlessInstall(await testDefaults({ lockfileDir: prefix, reporter, publicHoistPattern: ['*'] })) const project = assertProject(prefix) expect(project.requireModule('is-positive')).toBeTruthy() @@ -694,7 +691,7 @@ test('installing with publicHoistPattern=* in a project with external lockfile', await headlessInstall(await testDefaults({ lockfileDir, projects: [prefix], - publicHoistPattern: '*', + publicHoistPattern: ['*'], })) const project = assertProject(lockfileDir) @@ -703,7 +700,7 @@ test('installing with publicHoistPattern=* in a project with external lockfile', const ENGINE_DIR = `${process.platform}-${process.arch}-node-${process.version.split('.')[0]}` -test.each([['isolated'], ['hoisted']])('using side effects cache with nodeLinker=%s', async (nodeLinker) => { +test.each([['isolated'], ['hoisted']] as const)('using side effects cache with nodeLinker=%s', async (nodeLinker) => { let prefix = prepareFixtureWithIntegrity('side-effects') // Right now, hardlink does not work with side effects, so we specify copy as the packageImportMethod @@ -764,7 +761,7 @@ test.skip('using side effects cache and hoistPattern=*', async () => { // Right now, hardlink does not work with side effects, so we specify copy as the packageImportMethod // We disable verifyStoreIntegrity because we are going to change the cache const opts = await testDefaults({ - hoistPattern: '*', + hoistPattern: ['*'], lockfileDir, sideEffectsCacheRead: true, sideEffectsCacheWrite: true, diff --git a/pnpm11/installing/deps-restorer/test/utils/testDefaults.ts b/pnpm11/installing/deps-restorer/test/utils/testDefaults.ts index 62759e5f2f..b326174e4d 100644 --- a/pnpm11/installing/deps-restorer/test/utils/testDefaults.ts +++ b/pnpm11/installing/deps-restorer/test/utils/testDefaults.ts @@ -6,25 +6,44 @@ import { safeReadPackageJsonFromDir } from '@pnpm/pkg-manifest.reader' import { getStorePath } from '@pnpm/store.path' import { REGISTRY_MOCK_PORT } from '@pnpm/testing.registry-mock' import { createTempStore } from '@pnpm/testing.temp-store' +import type { DepPath, ProjectRootDir } from '@pnpm/types' import { temporaryDirectory } from 'tempy' const registry = `http://localhost:${REGISTRY_MOCK_PORT}/` +/** + * The options a test may override, on top of what this helper fills in. + * + * Typed rather than `any` so a key the headless install does not read is a + * compile error here: an options object is the whole input to the subject + * under test, and a misspelled or renamed key in one silently exercises the + * default instead of the case the test is named for. + */ +export type TestHeadlessOptions = Partial & { + /** Project directories, expanded into `HeadlessOptions.projects`. */ + projects?: string[] + /** + * Forwarded to the package store, which is what reads it — the headless + * install itself has no such option. + */ + verifyStoreIntegrity?: boolean +} + export async function testDefaults ( - opts?: any, // eslint-disable-line - resolveOpts?: any, // eslint-disable-line - fetchOpts?: any, // eslint-disable-line - storeOpts?: any, // eslint-disable-line + opts?: TestHeadlessOptions, + resolveOpts?: Record, + fetchOpts?: Record, + storeOpts?: Record ): Promise { const tmp = temporaryDirectory() let storeDir = opts?.storeDir ?? path.join(tmp, 'store') const lockfileDir = opts?.lockfileDir ?? process.cwd() const { include, pendingBuilds, projects } = await readProjectsContext( - opts.projects - ? opts.projects.map((rootDir: string) => ({ rootDir })) + opts?.projects + ? opts.projects.map((rootDir) => ({ rootDir: rootDir as ProjectRootDir })) : [ { - rootDir: lockfileDir, + rootDir: lockfileDir as ProjectRootDir, }, ], { lockfileDir } @@ -41,7 +60,13 @@ export async function testDefaults ( ...resolveOpts, ...fetchOpts, }, - storeOptions: storeOpts, + storeOptions: { + // The package store reads this, not the headless install. + ...(opts?.verifyStoreIntegrity != null + ? { verifyStoreIntegrity: opts.verifyStoreIntegrity } + : {}), + ...storeOpts, + }, } ) return { @@ -60,20 +85,25 @@ export async function testDefaults ( version: '1.0.0', }, pendingBuilds, - selectedProjectDirs: opts.selectedProjectDirs ?? projects.map((project) => project.rootDir), + selectedProjectDirs: opts?.selectedProjectDirs ?? projects.map((project) => project.rootDir), allProjects: Object.fromEntries( await Promise.all(projects.map(async (project) => [project.rootDir, { ...project, manifest: await safeReadPackageJsonFromDir(project.rootDir) }])) ), - authConfig: {}, registriesByScope: { default: registry, }, - sideEffectsCache: true, - skipped: new Set(), + skipped: new Set(), storeController, storeDir, + configByUri: {}, + globalVirtualStoreDir: path.join(storeDir, 'links'), + ignoreScripts: false, + pruneStore: false, + sideEffectsCacheRead: false, + sideEffectsCacheWrite: false, + userAgent: 'pnpm/0.0.0 npm/? node/0.0.0 test test', + virtualStoreDirMaxLength: process.platform === 'win32' ? 60 : 120, unsafePerm: true, - verifyStoreIntegrity: true, ...opts, } } diff --git a/pnpm11/testing/temp-store/src/index.ts b/pnpm11/testing/temp-store/src/index.ts index 8967bc5769..0b39fce269 100644 --- a/pnpm11/testing/temp-store/src/index.ts +++ b/pnpm11/testing/temp-store/src/index.ts @@ -20,7 +20,7 @@ export function createTempStore (opts?: { fastUnpack?: boolean storeDir?: string clientOptions?: Partial - storeOptions?: CreatePackageStoreOptions + storeOptions?: Partial }): CreateTempStoreResult { const configByUri: ClientOptions['configByUri'] = {} const cacheDir = path.resolve('cache')