Address review feedback - improve type safety, deduplicate code, add tests

Copilot · dmichon-msft · Copilot · commit 57fbb6bb35f0 · 2026-02-04T19:44:00.000Z
- Add proper jest.MockedFunction types for mock functions
- Add test for package.json + other file changes scenario
- Deduplicate version-only check logic into helper function
- Use Sort.sortBy for stable ordinal string comparison
- Extract isPackageJsonVersionOnlyChange as exported helper
- Add comprehensive unit tests for the helper function

Co-authored-by: dmichon-msft &lt;26827560+dmichon-msft@users.noreply.github.com&gt;
diff --git a/libraries/rush-lib/src/logic/ProjectChangeAnalyzer.ts b/libraries/rush-lib/src/logic/ProjectChangeAnalyzer.ts
@@ -6,7 +6,7 @@ import * as path from 'node:path';
 import ignore, { type Ignore } from 'ignore';
 
 import type { IReadonlyLookupByPath, LookupByPath } from '@rushstack/lookup-by-path';
-import { Path, FileSystem, Async, AlreadyReportedError } from '@rushstack/node-core-library';
+import { Path, FileSystem, Async, AlreadyReportedError, Sort } from '@rushstack/node-core-library';
 import {
   getRepoChanges,
   getRepoRoot,
@@ -118,6 +118,30 @@ export class ProjectChangeAnalyzer {
     > = this.getChangesByProject(lookup, changedFiles);
 
     const changedProjects: Set<RushConfigurationProject> = new Set();
+
+    // Helper function to check and add project to changedProjects
+    const checkAndAddProject = async (
+      project: RushConfigurationProject,
+      projectChanges: Map<string, IFileDiffStatus>
+    ): Promise<void> => {
+      if (projectChanges.size > 0) {
+        // Check for version-only changes if excludeVersionOnlyChanges is enabled
+        if (excludeVersionOnlyChanges && projectChanges.size === 1) {
+          const packageJsonPath: string = Path.convertToSlashes(
+            path.relative(repoRoot, path.join(project.projectFolder, 'package.json'))
+          );
+          const diffStatus: IFileDiffStatus | undefined = projectChanges.get(packageJsonPath);
+          if (diffStatus) {
+            const isVersionOnlyChange: boolean = await this._isVersionOnlyChangeAsync(diffStatus, repoRoot);
+            if (isVersionOnlyChange) {
+              return; // Skip adding this project
+            }
+          }
+        }
+        changedProjects.add(project);
+      }
+    };
+
     if (enableFiltering) {
       // Reading rush-project.json may be problematic if, e.g. rush install has not yet occurred and rigs are in use
       await Async.forEachAsync(
@@ -130,51 +154,15 @@ export class ProjectChangeAnalyzer {
             terminal
           );
 
-          if (filteredChanges.size > 0) {
-            // Check for version-only changes if excludeVersionOnlyChanges is enabled
-            if (excludeVersionOnlyChanges && filteredChanges.size === 1) {
-              const packageJsonPath: string = Path.convertToSlashes(
-                path.relative(repoRoot, path.join(project.projectFolder, 'package.json'))
-              );
-              const diffStatus: IFileDiffStatus | undefined = filteredChanges.get(packageJsonPath);
-              if (diffStatus) {
-                const isVersionOnlyChange: boolean = await this._isVersionOnlyChangeAsync(
-                  diffStatus,
-                  repoRoot
-                );
-                if (isVersionOnlyChange) {
-                  return; // Skip adding this project
-                }
-              }
-            }
-            changedProjects.add(project);
-          }
+          await checkAndAddProject(project, filteredChanges);
         },
         { concurrency: 10 }
       );
     } else {
       await Async.forEachAsync(
         changesByProject,
         async ([project, projectChanges]) => {
-          if (projectChanges.size > 0) {
-            // Check for version-only changes if excludeVersionOnlyChanges is enabled
-            if (excludeVersionOnlyChanges && projectChanges.size === 1) {
-              const packageJsonPath: string = Path.convertToSlashes(
-                path.relative(repoRoot, path.join(project.projectFolder, 'package.json'))
-              );
-              const diffStatus: IFileDiffStatus | undefined = projectChanges.get(packageJsonPath);
-              if (diffStatus) {
-                const isVersionOnlyChange: boolean = await this._isVersionOnlyChangeAsync(
-                  diffStatus,
-                  repoRoot
-                );
-                if (isVersionOnlyChange) {
-                  return; // Skip adding this project
-                }
-              }
-            }
-            changedProjects.add(project);
-          }
+          await checkAndAddProject(project, projectChanges);
         },
         { concurrency: 10 }
       );
@@ -263,9 +251,8 @@ export class ProjectChangeAnalyzer {
     }
 
     // Sort the set by projectRelativeFolder to avoid race conditions in the results
-    const sortedChangedProjects: RushConfigurationProject[] = Array.from(changedProjects).sort((a, b) =>
-      a.projectRelativeFolder.localeCompare(b.projectRelativeFolder)
-    );
+    const sortedChangedProjects: RushConfigurationProject[] = Array.from(changedProjects);
+    Sort.sortBy(sortedChangedProjects, (project) => project.projectRelativeFolder);
 
     return new Set(sortedChangedProjects);
   }
@@ -473,21 +460,7 @@ export class ProjectChangeAnalyzer {
         repositoryRoot: repoRoot
       });
 
-      // Parse both versions
-      const oldPackageJson: Record<string, unknown> = JSON.parse(oldPackageJsonContent);
-      const currentPackageJson: Record<string, unknown> = JSON.parse(currentPackageJsonContent);
-
-      // Ensure both have a version field
-      if (!oldPackageJson.version || !currentPackageJson.version) {
-        return false;
-      }
-
-      // Remove the version field from both (no need to clone, these are fresh objects from JSON.parse)
-      oldPackageJson.version = undefined;
-      currentPackageJson.version = undefined;
-
-      // Compare the objects without the version field
-      return JSON.stringify(oldPackageJson) === JSON.stringify(currentPackageJson);
+      return isPackageJsonVersionOnlyChange(oldPackageJsonContent, currentPackageJsonContent);
     } catch (error) {
       // If we can't read the file or parse it, assume it's not a version-only change
       return false;
@@ -612,3 +585,36 @@ async function getAdditionalFilesFromRushProjectConfigurationAsync(
 
   return additionalFilesFromRushProjectConfiguration;
 }
+
+/**
+ * Compares two package.json file contents and determines if the only difference is the "version" field.
+ * @param oldPackageJsonContent - The old package.json content as a string
+ * @param newPackageJsonContent - The new package.json content as a string
+ * @returns true if the only difference is the version field, false otherwise
+ * @public
+ */
+export function isPackageJsonVersionOnlyChange(
+  oldPackageJsonContent: string,
+  newPackageJsonContent: string
+): boolean {
+  try {
+    // Parse both versions
+    const oldPackageJson: Record<string, unknown> = JSON.parse(oldPackageJsonContent);
+    const newPackageJson: Record<string, unknown> = JSON.parse(newPackageJsonContent);
+
+    // Ensure both have a version field
+    if (!oldPackageJson.version || !newPackageJson.version) {
+      return false;
+    }
+
+    // Remove the version field from both (no need to clone, these are fresh objects from JSON.parse)
+    oldPackageJson.version = undefined;
+    newPackageJson.version = undefined;
+
+    // Compare the objects without the version field
+    return JSON.stringify(oldPackageJson) === JSON.stringify(newPackageJson);
+  } catch (error) {
+    // If we can't parse the JSON, assume it's not a version-only change
+    return false;
+  }
+}
diff --git a/libraries/rush-lib/src/logic/test/ProjectChangeAnalyzer.test.ts b/libraries/rush-lib/src/logic/test/ProjectChangeAnalyzer.test.ts
@@ -23,7 +23,8 @@ const mockHashes: Map<string, string> = new Map([
 ]);
 
 // Mock function for customizing repo changes in each test
-const mockGetRepoChanges = jest.fn<Map<string, IFileDiffStatus>, []>();
+const mockGetRepoChanges: jest.MockedFunction<typeof import('@rushstack/package-deps-hash').getRepoChanges> =
+  jest.fn();
 
 jest.mock(`@rushstack/package-deps-hash`, () => {
   return {
@@ -47,16 +48,22 @@ jest.mock(`@rushstack/package-deps-hash`, () => {
     hashFilesAsync(rootDirectory: string, filePaths: Iterable<string>): ReadonlyMap<string, string> {
       return new Map(Array.from(filePaths, (filePath: string) => [filePath, filePath]));
     },
-    getRepoChanges(): Map<string, IFileDiffStatus> {
-      return mockGetRepoChanges();
+    getRepoChanges(
+      currentWorkingDirectory: string,
+      revision?: string,
+      gitPath?: string
+    ): Map<string, IFileDiffStatus> {
+      return mockGetRepoChanges(currentWorkingDirectory, revision, gitPath);
     }
   };
 });
 
 const { Git: OriginalGit } = jest.requireActual('../Git');
 
 // Mock function for getBlobContentAsync to be customized in each test
-const mockGetBlobContentAsync = jest.fn<Promise<string>, [{ blobSpec: string; repositoryRoot: string }]>();
+const mockGetBlobContentAsync: jest.MockedFunction<
+  typeof import('../Git').Git.prototype.getBlobContentAsync
+> = jest.fn();
 
 /** Mock Git to test `getChangedProjectsAsync` */
 jest.mock('../Git', () => {
@@ -110,7 +117,7 @@ import { resolve } from 'node:path';
 import type { IDetailedRepoState, IFileDiffStatus } from '@rushstack/package-deps-hash';
 import { StringBufferTerminalProvider, Terminal } from '@rushstack/terminal';
 
-import { ProjectChangeAnalyzer } from '../ProjectChangeAnalyzer';
+import { ProjectChangeAnalyzer, isPackageJsonVersionOnlyChange } from '../ProjectChangeAnalyzer';
 import { RushConfiguration } from '../../api/RushConfiguration';
 import type {
   IInputsSnapshot,
@@ -370,6 +377,178 @@ describe(ProjectChangeAnalyzer.name, () => {
       });
       expect(changedProjects.has(rushConfiguration.getProjectByName('b')!)).toBe(true);
     });
+
+    it('excludeVersionOnlyChanges does not exclude projects when package.json and other files changed', async () => {
+      const rootDir: string = resolve(__dirname, 'repo');
+      const rushConfiguration: RushConfiguration = RushConfiguration.loadFromConfigurationFile(
+        resolve(rootDir, 'rush.json')
+      );
+
+      // Mock package.json with only version change
+      const oldPackageJsonContent = JSON.stringify(
+        {
+          name: 'c',
+          version: '1.0.0',
+          description: 'Test package',
+          dependencies: {
+            a: '1.0.0'
+          }
+        },
+        null,
+        2
+      );
+
+      const newPackageJsonContent = JSON.stringify(
+        {
+          name: 'c',
+          version: '1.0.1',
+          description: 'Test package',
+          dependencies: {
+            a: '1.0.0'
+          }
+        },
+        null,
+        2
+      );
+
+      // Set up mock repo changes - package.json AND another file changed for project 'c'
+      mockGetRepoChanges.mockReturnValue(
+        new Map<string, IFileDiffStatus>([
+          [
+            'c/package.json',
+            {
+              mode: 'modified',
+              newhash: 'newhash3',
+              oldhash: 'oldhash3',
+              status: 'M'
+            }
+          ],
+          [
+            'c/src/index.ts',
+            {
+              mode: 'modified',
+              newhash: 'newhash4',
+              oldhash: 'oldhash4',
+              status: 'M'
+            }
+          ]
+        ])
+      );
+
+      // Mock the blob content to return different versions based on the hash
+      mockGetBlobContentAsync.mockImplementation((opts: { blobSpec: string; repositoryRoot: string }) => {
+        if (opts.blobSpec === 'oldhash3') {
+          return Promise.resolve(oldPackageJsonContent);
+        } else if (opts.blobSpec === 'newhash3') {
+          return Promise.resolve(newPackageJsonContent);
+        }
+        return Promise.resolve('');
+      });
+
+      const projectChangeAnalyzer: ProjectChangeAnalyzer = new ProjectChangeAnalyzer(rushConfiguration);
+      const terminalProvider: StringBufferTerminalProvider = new StringBufferTerminalProvider(true);
+      const terminal: Terminal = new Terminal(terminalProvider);
+
+      // Test with excludeVersionOnlyChanges - project should still be detected as changed because multiple files changed
+      const changedProjects = await projectChangeAnalyzer.getChangedProjectsAsync({
+        enableFiltering: false,
+        includeExternalDependencies: false,
+        targetBranchName: 'main',
+        terminal,
+        excludeVersionOnlyChanges: true
+      });
+      expect(changedProjects.has(rushConfiguration.getProjectByName('c')!)).toBe(true);
+    });
+  });
+
+  describe('isPackageJsonVersionOnlyChange', () => {
+    it('returns true when only version field changed', () => {
+      const oldContent = JSON.stringify({
+        name: 'test-package',
+        version: '1.0.0',
+        description: 'Test package',
+        dependencies: { foo: '1.0.0' }
+      });
+      const newContent = JSON.stringify({
+        name: 'test-package',
+        version: '1.0.1',
+        description: 'Test package',
+        dependencies: { foo: '1.0.0' }
+      });
+
+      expect(isPackageJsonVersionOnlyChange(oldContent, newContent)).toBe(true);
+    });
+
+    it('returns false when other fields changed', () => {
+      const oldContent = JSON.stringify({
+        name: 'test-package',
+        version: '1.0.0',
+        description: 'Test package',
+        dependencies: { foo: '1.0.0' }
+      });
+      const newContent = JSON.stringify({
+        name: 'test-package',
+        version: '1.0.1',
+        description: 'Test package',
+        dependencies: { foo: '1.0.1' }
+      });
+
+      expect(isPackageJsonVersionOnlyChange(oldContent, newContent)).toBe(false);
+    });
+
+    it('returns false when version field is missing in old content', () => {
+      const oldContent = JSON.stringify({
+        name: 'test-package',
+        description: 'Test package'
+      });
+      const newContent = JSON.stringify({
+        name: 'test-package',
+        version: '1.0.1',
+        description: 'Test package'
+      });
+
+      expect(isPackageJsonVersionOnlyChange(oldContent, newContent)).toBe(false);
+    });
+
+    it('returns false when version field is missing in new content', () => {
+      const oldContent = JSON.stringify({
+        name: 'test-package',
+        version: '1.0.0',
+        description: 'Test package'
+      });
+      const newContent = JSON.stringify({
+        name: 'test-package',
+        description: 'Test package'
+      });
+
+      expect(isPackageJsonVersionOnlyChange(oldContent, newContent)).toBe(false);
+    });
+
+    it('returns false when JSON is invalid', () => {
+      const oldContent = 'invalid json';
+      const newContent = '{ "name": "test" }';
+
+      expect(isPackageJsonVersionOnlyChange(oldContent, newContent)).toBe(false);
+    });
+
+    it('returns true even with whitespace differences', () => {
+      const oldContent = JSON.stringify(
+        {
+          name: 'test-package',
+          version: '1.0.0',
+          description: 'Test package'
+        },
+        null,
+        2
+      );
+      const newContent = JSON.stringify({
+        name: 'test-package',
+        version: '1.0.1',
+        description: 'Test package'
+      });
+
+      expect(isPackageJsonVersionOnlyChange(oldContent, newContent)).toBe(true);
+    });
   });
 });
 
