From 641d88cb07562beb2c3d4ca0ae1e619a7a321a98 Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Mon, 23 Feb 2026 21:01:27 +0100 Subject: [PATCH] Address PR review comments - Remove cli-node/src/paths.ts compat layer, migrate all cli-node internal usage to import targetPaths from @backstage/cli-common - Use single-arg overrideTargetPaths('/root') where dir === rootDir - Scope mockDir to each describe block in bump.test.ts to avoid shared state issues with overrideTargetPaths - Remove unnecessary overrideTargetPaths from plugin-manager.test.ts - Remove stale findPaths mock from createApp.test.ts - Use overrideTargetPaths in getWorkspaceRoot.test.ts and cli-node tests Signed-off-by: Patrik Oldsberg Co-authored-by: Cursor --- .../src/manager/plugin-manager.test.ts | 5 --- packages/cli-node/src/git/GitUtils.ts | 6 +-- .../src/monorepo/PackageGraph.test.ts | 9 +---- .../cli-node/src/monorepo/PackageGraph.ts | 8 ++-- packages/cli-node/src/monorepo/isMonoRepo.ts | 4 +- .../cli-node/src/monorepo/isMonorepo.test.ts | 6 +-- .../src/pacman/PackageManager.test.ts | 7 +--- .../cli-node/src/pacman/PackageManager.ts | 6 +-- .../cli-node/src/pacman/yarn/Yarn.test.ts | 7 +--- packages/cli-node/src/pacman/yarn/Yarn.ts | 4 +- packages/cli-node/src/paths.ts | 40 ------------------- .../modules/build/commands/repo/start.test.ts | 2 +- .../migrate/commands/versions/bump.test.ts | 29 ++++++-------- packages/create-app/src/createApp.test.ts | 1 - .../src/util/getWorkspaceRoot.test.ts | 24 ++--------- 15 files changed, 40 insertions(+), 118 deletions(-) delete mode 100644 packages/cli-node/src/paths.ts diff --git a/packages/backend-dynamic-feature-service/src/manager/plugin-manager.test.ts b/packages/backend-dynamic-feature-service/src/manager/plugin-manager.test.ts index a9340d5962..aee492b1b7 100644 --- a/packages/backend-dynamic-feature-service/src/manager/plugin-manager.test.ts +++ b/packages/backend-dynamic-feature-service/src/manager/plugin-manager.test.ts @@ -39,7 +39,6 @@ import { ConfigSources } from '@backstage/config-loader'; import { Logs, MockedLogger, LogContent } from '../__testUtils__/testUtils'; import { PluginScanner } from '../scanner/plugin-scanner'; import { targetPaths } from '@backstage/cli-common'; -import { overrideTargetPaths } from '@backstage/cli-common/testUtils'; import { createMockDirectory } from '@backstage/backend-test-utils'; import { rootLifecycleServiceFactory } from '@backstage/backend-defaults/rootLifecycle'; import { BackstagePackageJson, PackageRole } from '@backstage/cli-node'; @@ -47,10 +46,6 @@ import { BackstagePackageJson, PackageRole } from '@backstage/cli-node'; describe('backend-dynamic-feature-service', () => { const mockDir = createMockDirectory(); - beforeAll(() => { - overrideTargetPaths(require('path').resolve(__dirname, '../../../..')); - }); - describe('loadPlugins', () => { afterEach(() => { jest.resetModules(); diff --git a/packages/cli-node/src/git/GitUtils.ts b/packages/cli-node/src/git/GitUtils.ts index 7edb3581d6..f2350be309 100644 --- a/packages/cli-node/src/git/GitUtils.ts +++ b/packages/cli-node/src/git/GitUtils.ts @@ -15,7 +15,7 @@ */ import { assertError, ForwardedError } from '@backstage/errors'; -import { paths } from '../paths'; +import { targetPaths } from '@backstage/cli-common'; import { runOutput } from '@backstage/cli-common'; /** @@ -24,7 +24,7 @@ import { runOutput } from '@backstage/cli-common'; export async function runGit(...args: string[]) { try { const stdout = await runOutput(['git', ...args], { - cwd: paths.targetRoot, + cwd: targetPaths.rootDir, }); return stdout.trim().split(/\r\n|\r|\n/); } catch (error) { @@ -88,7 +88,7 @@ export class GitUtils { } const stdout = await runOutput(['git', 'show', `${showRef}:${path}`], { - cwd: paths.targetRoot, + cwd: targetPaths.rootDir, }); return stdout; } diff --git a/packages/cli-node/src/monorepo/PackageGraph.test.ts b/packages/cli-node/src/monorepo/PackageGraph.test.ts index d1a0cce649..51d8f7b7be 100644 --- a/packages/cli-node/src/monorepo/PackageGraph.test.ts +++ b/packages/cli-node/src/monorepo/PackageGraph.test.ts @@ -14,21 +14,16 @@ * limitations under the License. */ -import { resolve as resolvePath } from 'node:path'; import { getPackages } from '@manypkg/get-packages'; import { PackageGraph } from './PackageGraph'; import { Lockfile } from './Lockfile'; import { GitUtils } from '../git'; +import { overrideTargetPaths } from '@backstage/cli-common/testUtils'; const mockListChangedFiles = jest.spyOn(GitUtils, 'listChangedFiles'); const mockReadFileAtRef = jest.spyOn(GitUtils, 'readFileAtRef'); -jest.mock('../paths', () => ({ - paths: { - targetRoot: '/', - resolveTargetRoot: (...paths: string[]) => resolvePath('/', ...paths), - }, -})); +overrideTargetPaths('/'); const testPackages = [ { diff --git a/packages/cli-node/src/monorepo/PackageGraph.ts b/packages/cli-node/src/monorepo/PackageGraph.ts index 6d3da7cf82..9d1745c7c5 100644 --- a/packages/cli-node/src/monorepo/PackageGraph.ts +++ b/packages/cli-node/src/monorepo/PackageGraph.ts @@ -16,7 +16,7 @@ import path from 'node:path'; import { getPackages, Package } from '@manypkg/get-packages'; -import { paths } from '../paths'; +import { targetPaths } from '@backstage/cli-common'; import { PackageRole } from '../roles'; import { GitUtils } from '../git'; import { Lockfile } from './Lockfile'; @@ -192,7 +192,7 @@ export class PackageGraph extends Map { * Lists all local packages in a monorepo. */ static async listTargetPackages(): Promise { - const { packages } = await getPackages(paths.targetDir); + const { packages } = await getPackages(targetPaths.dir); return packages as BackstagePackage[]; } @@ -332,7 +332,7 @@ export class PackageGraph extends Map { Array.from(this.values()).map(pkg => [ // relative from root, convert to posix, and add a / at the end path - .relative(paths.targetRoot, pkg.dir) + .relative(targetPaths.rootDir, pkg.dir) .split(path.sep) .join(path.posix.sep) + path.posix.sep, pkg, @@ -374,7 +374,7 @@ export class PackageGraph extends Map { let otherLockfile: Lockfile; try { thisLockfile = await Lockfile.load( - paths.resolveTargetRoot('yarn.lock'), + targetPaths.resolveRoot('yarn.lock'), ); otherLockfile = Lockfile.parse( await GitUtils.readFileAtRef('yarn.lock', options.ref), diff --git a/packages/cli-node/src/monorepo/isMonoRepo.ts b/packages/cli-node/src/monorepo/isMonoRepo.ts index da78c8dd53..93a5e48abb 100644 --- a/packages/cli-node/src/monorepo/isMonoRepo.ts +++ b/packages/cli-node/src/monorepo/isMonoRepo.ts @@ -14,7 +14,7 @@ * limitations under the License. */ -import { paths } from '../paths'; +import { targetPaths } from '@backstage/cli-common'; import fs from 'fs-extra'; /** @@ -23,7 +23,7 @@ import fs from 'fs-extra'; * @public */ export async function isMonoRepo(): Promise { - const rootPackageJsonPath = paths.resolveTargetRoot('package.json'); + const rootPackageJsonPath = targetPaths.resolveRoot('package.json'); try { const pkg = await fs.readJson(rootPackageJsonPath); return Boolean(pkg?.workspaces?.packages); diff --git a/packages/cli-node/src/monorepo/isMonorepo.test.ts b/packages/cli-node/src/monorepo/isMonorepo.test.ts index de0dae0893..e13f9a8784 100644 --- a/packages/cli-node/src/monorepo/isMonorepo.test.ts +++ b/packages/cli-node/src/monorepo/isMonorepo.test.ts @@ -16,12 +16,10 @@ import { isMonoRepo } from './isMonoRepo'; import { createMockDirectory } from '@backstage/backend-test-utils'; +import { overrideTargetPaths } from '@backstage/cli-common/testUtils'; const mockDir = createMockDirectory(); - -jest.mock('../paths', () => ({ - paths: { resolveTargetRoot: (...args: string[]) => mockDir.resolve(...args) }, -})); +overrideTargetPaths(mockDir.path); describe('isMonoRepo', () => { it('should detect a monorepo', async () => { diff --git a/packages/cli-node/src/pacman/PackageManager.test.ts b/packages/cli-node/src/pacman/PackageManager.test.ts index dd35e74902..ef169b1337 100644 --- a/packages/cli-node/src/pacman/PackageManager.test.ts +++ b/packages/cli-node/src/pacman/PackageManager.test.ts @@ -15,16 +15,13 @@ */ import { createMockDirectory } from '@backstage/backend-test-utils'; +import { overrideTargetPaths } from '@backstage/cli-common/testUtils'; import { detectPackageManager } from './PackageManager'; import { Yarn } from './yarn'; import { withLogCollector } from '@backstage/test-utils'; const mockDir = createMockDirectory(); - -jest.mock('../paths', () => ({ - ...jest.requireActual('../paths'), - paths: { resolveTargetRoot: (...args: string[]) => mockDir.resolve(...args) }, -})); +overrideTargetPaths(mockDir.path); const mockYarnCreate = jest.spyOn(Yarn, 'create'); diff --git a/packages/cli-node/src/pacman/PackageManager.ts b/packages/cli-node/src/pacman/PackageManager.ts index 44c0849705..b908e99c72 100644 --- a/packages/cli-node/src/pacman/PackageManager.ts +++ b/packages/cli-node/src/pacman/PackageManager.ts @@ -16,7 +16,7 @@ import { Yarn } from './yarn'; import { Lockfile } from './Lockfile'; -import { paths } from '../paths'; +import { targetPaths } from '@backstage/cli-common'; import { RunOptions } from '@backstage/cli-common'; import fs from 'fs-extra'; @@ -91,7 +91,7 @@ export interface PackageManager { */ export async function detectPackageManager(): Promise { const hasYarnLockfile = await fileExists( - paths.resolveTargetRoot('yarn.lock'), + targetPaths.resolveRoot('yarn.lock'), ); if (hasYarnLockfile) { return await Yarn.create(); @@ -99,7 +99,7 @@ export async function detectPackageManager(): Promise { try { const packageJson = await fs.readJson( - paths.resolveTargetRoot('package.json'), + targetPaths.resolveRoot('package.json'), ); if (packageJson.workspaces) { // technically this could be NPM as well diff --git a/packages/cli-node/src/pacman/yarn/Yarn.test.ts b/packages/cli-node/src/pacman/yarn/Yarn.test.ts index 9bf866b821..42b5caa6c5 100644 --- a/packages/cli-node/src/pacman/yarn/Yarn.test.ts +++ b/packages/cli-node/src/pacman/yarn/Yarn.test.ts @@ -15,14 +15,11 @@ */ import { createMockDirectory } from '@backstage/backend-test-utils'; +import { overrideTargetPaths } from '@backstage/cli-common/testUtils'; import { Yarn } from './Yarn'; const mockDir = createMockDirectory(); - -jest.mock('../../paths', () => ({ - ...jest.requireActual('../../paths'), - paths: { resolveTargetRoot: (...args: string[]) => mockDir.resolve(...args) }, -})); +overrideTargetPaths(mockDir.path); const yarnClassic = new Yarn({ version: '1.0.0', codename: 'classic' }); const yarnBerry = new Yarn({ version: '3.0.0', codename: 'berry' }); diff --git a/packages/cli-node/src/pacman/yarn/Yarn.ts b/packages/cli-node/src/pacman/yarn/Yarn.ts index a6544344ca..b153a6ef40 100644 --- a/packages/cli-node/src/pacman/yarn/Yarn.ts +++ b/packages/cli-node/src/pacman/yarn/Yarn.ts @@ -23,7 +23,7 @@ import { PackageInfo, PackageManager } from '../PackageManager'; import { Lockfile } from '../Lockfile'; import { YarnVersion } from './types'; import fs from 'fs-extra'; -import { paths } from '../../paths'; +import { targetPaths } from '@backstage/cli-common'; import { run, runOutput, RunOptions } from '@backstage/cli-common'; export class Yarn implements PackageManager { @@ -47,7 +47,7 @@ export class Yarn implements PackageManager { } async getMonorepoPackages() { - const rootPackageJsonPath = paths.resolveTargetRoot('package.json'); + const rootPackageJsonPath = targetPaths.resolveRoot('package.json'); try { const pkg = await fs.readJson(rootPackageJsonPath); return pkg?.workspaces?.packages || []; diff --git a/packages/cli-node/src/paths.ts b/packages/cli-node/src/paths.ts deleted file mode 100644 index 334ef9eaf3..0000000000 --- a/packages/cli-node/src/paths.ts +++ /dev/null @@ -1,40 +0,0 @@ -/* - * Copyright 2020 The Backstage Authors - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -import { targetPaths, findOwnPaths } from '@backstage/cli-common'; - -/* eslint-disable-next-line no-restricted-syntax */ -const ownPaths = findOwnPaths(__dirname); - -/* eslint-disable-next-line no-restricted-syntax */ -export const paths = { - get ownDir() { - return ownPaths.dir; - }, - get ownRoot() { - return ownPaths.rootDir; - }, - get targetDir() { - return targetPaths.dir; - }, - get targetRoot() { - return targetPaths.rootDir; - }, - resolveOwn: ownPaths.resolve, - resolveOwnRoot: ownPaths.resolveRoot, - resolveTarget: targetPaths.resolve, - resolveTargetRoot: targetPaths.resolveRoot, -}; diff --git a/packages/cli/src/modules/build/commands/repo/start.test.ts b/packages/cli/src/modules/build/commands/repo/start.test.ts index a9eb01b21b..328c8fd365 100644 --- a/packages/cli/src/modules/build/commands/repo/start.test.ts +++ b/packages/cli/src/modules/build/commands/repo/start.test.ts @@ -18,7 +18,7 @@ import { PackageGraph } from '@backstage/cli-node'; import { findTargetPackages } from './start'; import { overrideTargetPaths } from '@backstage/cli-common/testUtils'; -overrideTargetPaths({ dir: '/root', rootDir: '/root' }); +overrideTargetPaths('/root'); const mocks = { app: { diff --git a/packages/cli/src/modules/migrate/commands/versions/bump.test.ts b/packages/cli/src/modules/migrate/commands/versions/bump.test.ts index b44de6fc6b..cc662f9d5e 100644 --- a/packages/cli/src/modules/migrate/commands/versions/bump.test.ts +++ b/packages/cli/src/modules/migrate/commands/versions/bump.test.ts @@ -23,10 +23,7 @@ import { YarnInfoInspectData } from '../../../../lib/versioning/packages'; import { setupServer } from 'msw/node'; import { rest } from 'msw'; import { NotFoundError } from '@backstage/errors'; -import { - createMockDirectory, - MockDirectory, -} from '@backstage/backend-test-utils'; +import { createMockDirectory } from '@backstage/backend-test-utils'; // Avoid mutating the global agents used in other tests jest.mock('global-agent', () => ({ @@ -60,17 +57,10 @@ jest.mock('ora', () => ({ }, })); -let mockDir: MockDirectory; jest.mock('@backstage/cli-common', () => { const actual = jest.requireActual('@backstage/cli-common'); return { ...actual, - findPaths: () => ({ - resolveTargetRoot: (...args: string[]) => mockDir.resolve(...args), - get targetDir() { - return mockDir.path; - }, - }), run: jest.fn().mockReturnValue({ exitCode: null, waitForExit: jest.fn().mockResolvedValue(undefined), @@ -137,10 +127,10 @@ const expectLogsToMatch = ( }; describe('bump', () => { - mockDir = createMockDirectory(); - beforeAll(() => overrideTargetPaths(mockDir.path)); + const mockDir = createMockDirectory(); beforeEach(() => { + overrideTargetPaths(mockDir.path); mockFetchPackageInfo.mockImplementation(async name => ({ name: name, 'dist-tags': { @@ -944,8 +934,11 @@ describe('bump', () => { }); describe('bumpBackstageJsonVersion', () => { - mockDir = createMockDirectory(); - beforeAll(() => overrideTargetPaths(mockDir.path)); + const mockDir = createMockDirectory(); + + beforeEach(() => { + overrideTargetPaths(mockDir.path); + }); afterEach(() => { jest.resetAllMocks(); @@ -1079,10 +1072,14 @@ describe('createVersionFinder', () => { }); describe('environment variables', () => { + const mockDir = createMockDirectory(); + const worker = setupServer(); registerMswTestHooks(worker); - beforeAll(() => overrideTargetPaths(mockDir.path)); + beforeEach(() => { + overrideTargetPaths(mockDir.path); + }); beforeEach(() => { delete process.env.BACKSTAGE_MANIFEST_FILE; diff --git a/packages/create-app/src/createApp.test.ts b/packages/create-app/src/createApp.test.ts index 1f78c084c5..d49078887c 100644 --- a/packages/create-app/src/createApp.test.ts +++ b/packages/create-app/src/createApp.test.ts @@ -42,7 +42,6 @@ jest.mock('@backstage/cli-common', () => { }; return { ...actual, - findPaths: jest.fn(), findOwnPaths: () => mockOwnPaths, }; }); diff --git a/packages/yarn-plugin/src/util/getWorkspaceRoot.test.ts b/packages/yarn-plugin/src/util/getWorkspaceRoot.test.ts index 0f44429707..083baae907 100644 --- a/packages/yarn-plugin/src/util/getWorkspaceRoot.test.ts +++ b/packages/yarn-plugin/src/util/getWorkspaceRoot.test.ts @@ -14,8 +14,6 @@ * limitations under the License. */ -import { targetPaths } from '@backstage/cli-common'; - const setPlatform = (platform: string) => { Object.defineProperty(process, `platform`, { configurable: true, @@ -37,7 +35,6 @@ describe('getWorkspaceRoot', () => { `('platform: $platform', ({ platform, native, portable }) => { let realPlatform: string; let getWorkspaceRoot: () => string; - let mockResolveRoot: jest.MockedFunction; beforeEach(() => { realPlatform = process.platform; @@ -45,21 +42,10 @@ describe('getWorkspaceRoot', () => { jest.resetModules(); - mockResolveRoot = jest.fn(); - - jest.doMock('@backstage/cli-common', () => ({ - ...jest.requireActual('@backstage/cli-common'), - targetPaths: { - get dir() { - return mockResolveRoot(); - }, - get rootDir() { - return mockResolveRoot(); - }, - resolveRoot: mockResolveRoot, - }, - })); - + const { + overrideTargetPaths, + } = require('@backstage/cli-common/testUtils'); + overrideTargetPaths(native); getWorkspaceRoot = require('./getWorkspaceRoot').getWorkspaceRoot; }); @@ -68,8 +54,6 @@ describe('getWorkspaceRoot', () => { }); it('returns an appropriately-formatted workspace root path', () => { - mockResolveRoot.mockReturnValue(native); - expect(getWorkspaceRoot()).toEqual(portable); }); });