Code review feedback

Author: Ben Keen

libraries/rush-lib/src/cli/test/RushPnpmCommandLineParser.test.ts

@@ -5,26 +5,19 @@ import * as path from 'node:path';
 import { FileSystem, JsonFile } from '@rushstack/node-core-library';
 import { TestUtilities } from '@rushstack/heft-config-file';
 import { RushConfiguration } from '../../api/RushConfiguration';
+import { PnpmWorkspaceFile } from '../../logic/pnpm/PnpmWorkspaceFile';
 
-const MONOREPO_ROOT: string = path.dirname(
-  RushConfiguration.tryFindRushJsonLocation({ startingFolder: __dirname })!
-);
+const PACKAGE_ROOT: string = path.resolve(__dirname, '../../..');
 const CATALOG_SYNC_REPO_PATH: string = `${__dirname}/catalogSyncTestRepo`;
 
 describe('RushPnpmCommandLineParser', () => {
   describe('catalog syncing', () => {
-    const testRepoPath: string = `${MONOREPO_ROOT}/temp/catalog-sync-test-repo`;
+    const testRepoPath: string = `${PACKAGE_ROOT}/temp/catalog-sync-test-repo`;
     const pnpmConfigPath: string = `${testRepoPath}/common/config/rush/pnpm-config.json`;
     const pnpmWorkspacePath: string = `${testRepoPath}/common/temp/pnpm-workspace.yaml`;
 
     beforeEach(async () => {
       await FileSystem.copyFilesAsync({ sourcePath: CATALOG_SYNC_REPO_PATH, destinationPath: testRepoPath });
-
-      // common/temp is gitignored so it is not part of the static repo; copy the initial workspace file here
-      await FileSystem.copyFilesAsync({
-        sourcePath: `${CATALOG_SYNC_REPO_PATH}/pnpm-workspace.yaml`,
-        destinationPath: pnpmWorkspacePath
-      });
     });
 
     afterEach(async () => {
@@ -58,7 +51,6 @@ catalogs:
         }
       });
 
-      const { PnpmWorkspaceFile } = require('../../logic/pnpm/PnpmWorkspaceFile');
       const newCatalogs = await PnpmWorkspaceFile.loadCatalogsFromFileAsync(pnpmWorkspacePath);
 
       await pnpmOptions?.updateGlobalCatalogsAsync(newCatalogs);
@@ -82,7 +74,6 @@ catalogs:
     });
 
     it('does not update pnpm-config.json when catalogs are unchanged', async () => {
-      const { PnpmWorkspaceFile } = require('../../logic/pnpm/PnpmWorkspaceFile');
       const newCatalogs = await PnpmWorkspaceFile.loadCatalogsFromFileAsync(pnpmWorkspacePath);
 
       const rushConfiguration: RushConfiguration = RushConfiguration.loadFromConfigurationFile(
@@ -108,7 +99,6 @@ catalogs:
 `;
       await FileSystem.writeFileAsync(pnpmWorkspacePath, workspaceWithoutCatalogs);
 
-      const { PnpmWorkspaceFile } = require('../../logic/pnpm/PnpmWorkspaceFile');
       const newCatalogs = await PnpmWorkspaceFile.loadCatalogsFromFileAsync(pnpmWorkspacePath);
 
       expect(newCatalogs).toBeUndefined();

libraries/rush-lib/src/cli/test/catalogSyncTestRepo/common/.gitignore

@@ -0,0 +1 @@
+!temp

…/catalogSyncTestRepo/pnpm-workspace.yaml …TestRepo/common/temp/pnpm-workspace.yamllibraries/rush-lib/src/cli/test/catalogSyncTestRepo/pnpm-workspace.yaml renamed to libraries/rush-lib/src/cli/test/catalogSyncTestRepo/common/temp/pnpm-workspace.yaml libraries/rush-lib/src/cli/test/catalogSyncTestRepo/pnpm-workspace.yaml renamed to libraries/rush-lib/src/cli/test/catalogSyncTestRepo/common/temp/pnpm-workspace.yaml

@@ -5,12 +5,9 @@ import * as path from 'node:path';
 import { FileSystem, JsonFile } from '@rushstack/node-core-library';
 import { PnpmOptionsConfiguration } from '../PnpmOptionsConfiguration';
 import { TestUtilities } from '@rushstack/heft-config-file';
-import { RushConfiguration } from '../../../api/RushConfiguration';
 
-const MONOREPO_ROOT: string = path.dirname(
-  RushConfiguration.tryFindRushJsonLocation({ startingFolder: __dirname })!
-);
-const TEST_TEMP_FOLDER: string = `${MONOREPO_ROOT}/temp/pnpm-config-update-test`;
+const PACKAGE_ROOT: string = path.resolve(__dirname, '../../../..');
+const TEST_TEMP_FOLDER: string = `${PACKAGE_ROOT}/temp/pnpm-config-update-test`;
 
 const fakeCommonTempFolder: string = `${__dirname}/common/temp`;
 

libraries/rush-lib/src/logic/pnpm/test/PnpmOptionsConfiguration.test.ts

@@ -14,7 +14,6 @@ describe(PnpmWorkspaceFile.name, () => {
   let mockReadFile: jest.SpyInstance;
   let mockReadFileAsync: jest.SpyInstance;
   let mockExists: jest.SpyInstance;
-  let mockExistsAsync: jest.SpyInstance;
   let writtenContent: string | undefined;
 
   beforeEach(() => {
@@ -31,15 +30,23 @@ describe(PnpmWorkspaceFile.name, () => {
     // Mock FileSystem.readFile to return the written content
     mockReadFile = jest.spyOn(FileSystem, 'readFile').mockImplementation(() => {
       if (writtenContent === undefined) {
-        throw Object.assign(new Error('ENOENT: no such file or directory'), { code: 'ENOENT', errno: -2, syscall: 'open' });
+        throw Object.assign(new Error('ENOENT: no such file or directory'), {
+          code: 'ENOENT',
+          errno: -2,
+          syscall: 'open'
+        });
       }
       return writtenContent;
     });
 
     // Mock async version for loadCatalogsFromFileAsync
     mockReadFileAsync = jest.spyOn(FileSystem, 'readFileAsync').mockImplementation(async () => {
       if (writtenContent === undefined) {
-        throw Object.assign(new Error('ENOENT: no such file or directory'), { code: 'ENOENT', errno: -2, syscall: 'open' });
+        throw Object.assign(new Error('ENOENT: no such file or directory'), {
+          code: 'ENOENT',
+          errno: -2,
+          syscall: 'open'
+        });
       }
       return writtenContent;
     });
@@ -48,19 +55,13 @@ describe(PnpmWorkspaceFile.name, () => {
     mockExists = jest.spyOn(FileSystem, 'exists').mockImplementation(() => {
       return writtenContent !== undefined;
     });
-
-    // Mock async version for loadCatalogsFromFileAsync
-    mockExistsAsync = jest.spyOn(FileSystem, 'existsAsync').mockImplementation(async () => {
-      return writtenContent !== undefined;
-    });
   });
 
   afterEach(() => {
     mockWriteFile.mockRestore();
     mockReadFile.mockRestore();
     mockReadFileAsync.mockRestore();
     mockExists.mockRestore();
-    mockExistsAsync.mockRestore();
   });
 
   describe('basic functionality', () => {