Merge pull request #912 from chromaui/tom/ap-4194-ts-lockfile-checkin…

…g-can-take-10s-even-when-no-changes-are Be smarter about comparing lock files
chromaui · Feb 7, 2024 · 36bae9b · 36bae9b
2 parents c1ddf2b + b8db435
commit 36bae9b
Show file tree

Hide file tree

Showing 9 changed files with 213 additions and 143 deletions.
diff --git a/node-src/lib/findChangedDependencies.test.ts b/node-src/lib/findChangedDependencies.test.ts
@@ -4,6 +4,7 @@ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest';
 import * as git from '../git/git';
 import { findChangedDependencies } from './findChangedDependencies';
 import TestLogger from './testLogger';
+import { Context } from '..';
 
 vi.mock('snyk-nodejs-lockfile-parser');
 vi.mock('yarn-or-npm');
@@ -25,25 +26,63 @@ afterEach(() => {
   buildDepTree.mockReset();
 });
 
-const getContext: any = (baselineCommits: string[], options = {}) => ({
-  log: new TestLogger(),
-  git: { baselineCommits },
-  options,
-});
+const getContext = (
+  input: Partial<Omit<Context, 'git' | 'options'>> & {
+    git: Partial<Context['git']>;
+    options?: Partial<Context['options']>;
+  }
+) =>
+  ({
+    log: new TestLogger(),
+    options: {},
+    ...input,
+  } as Context);
+
+const AMetadataChanges = [{ changedFiles: ['package.json'], commit: 'A' }];
 
 describe('findChangedDependencies', () => {
-  it('returns nothing given no baselines', async () => {
-    buildDepTree.mockResolvedValue({ dependencies: {} });
+  it('returns nothing given no changes', async () => {
+    const context = getContext({ git: { packageMetadataChanges: [] } });
+
+    await expect(findChangedDependencies(context)).resolves.toEqual([]);
+    expect(checkoutFile).not.toHaveBeenCalled();
+    expect(buildDepTreeFromFiles).not.toHaveBeenCalled();
+  });
+
+  it('returns nothing given no changes to found package metadata', async () => {
+    const context = getContext({
+      // Only detected metadata change is to a file that's not on disk
+      git: { packageMetadataChanges: [{ changedFiles: ['foo/package.json'], commit: 'A' }] },
+    });
+
+    await expect(findChangedDependencies(context)).resolves.toEqual([]);
+
+    expect(checkoutFile).not.toHaveBeenCalled();
+    expect(buildDepTreeFromFiles).not.toHaveBeenCalled();
+  });
+
+  it('returns nothing when dependency tree is unchanged', async () => {
+    checkoutFile.mockResolvedValueOnce('A.package.json');
+    checkoutFile.mockResolvedValueOnce('A.yarn.lock');
+    buildDepTree.mockResolvedValue({
+      dependencies: {
+        react: { name: 'react', version: '18.2.0', dependencies: {} },
+      },
+    });
+
+    const context = getContext({ git: { packageMetadataChanges: AMetadataChanges } });
 
-    await expect(findChangedDependencies(getContext([]))).resolves.toEqual([]);
+    await expect(findChangedDependencies(context)).resolves.toEqual([]);
   });
 
   it('returns nothing when dependency tree is empty', async () => {
     checkoutFile.mockResolvedValueOnce('A.package.json');
     checkoutFile.mockResolvedValueOnce('A.yarn.lock');
     buildDepTree.mockResolvedValue({ dependencies: {} });
 
-    await expect(findChangedDependencies(getContext(['A']))).resolves.toEqual([]);
+    const context = getContext({ git: { packageMetadataChanges: AMetadataChanges } });
+
+    await expect(findChangedDependencies(context)).resolves.toEqual([]);
   });
 
   it('returns nothing when dependency tree is unchanged', async () => {
@@ -55,7 +94,9 @@ describe('findChangedDependencies', () => {
       },
     });
 
-    await expect(findChangedDependencies(getContext(['A']))).resolves.toEqual([]);
+    const context = getContext({ git: { packageMetadataChanges: AMetadataChanges } });
+
+    await expect(findChangedDependencies(context)).resolves.toEqual([]);
   });
 
   it('returns updated dependencies', async () => {
@@ -71,7 +112,9 @@ describe('findChangedDependencies', () => {
       dependencies: { react: { name: 'react', version: '18.3.0', dependencies: {} } },
     });
 
-    await expect(findChangedDependencies(getContext(['A']))).resolves.toEqual(['react']);
+    const context = getContext({ git: { packageMetadataChanges: AMetadataChanges } });
+
+    await expect(findChangedDependencies(context)).resolves.toEqual(['react']);
   });
 
   it('returns added/removed dependencies', async () => {
@@ -87,7 +130,9 @@ describe('findChangedDependencies', () => {
       dependencies: { vue: { name: 'vue', version: '3.2.0', dependencies: {} } },
     });
 
-    await expect(findChangedDependencies(getContext(['A']))).resolves.toEqual(['vue', 'react']);
+    const context = getContext({ git: { packageMetadataChanges: AMetadataChanges } });
+
+    await expect(findChangedDependencies(context)).resolves.toEqual(['vue', 'react']);
   });
 
   it('finds updated transient dependencies', async () => {
@@ -119,7 +164,9 @@ describe('findChangedDependencies', () => {
       },
     });
 
-    await expect(findChangedDependencies(getContext(['A']))).resolves.toEqual(['loose-envify']);
+    const context = getContext({ git: { packageMetadataChanges: AMetadataChanges } });
+
+    await expect(findChangedDependencies(context)).resolves.toEqual(['loose-envify']);
   });
 
   it('combines and dedupes changes for multiple baselines', async () => {
@@ -151,7 +198,16 @@ describe('findChangedDependencies', () => {
       },
     });
 
-    await expect(findChangedDependencies(getContext(['A', 'B']))).resolves.toEqual([
+    const context = getContext({
+      git: {
+        packageMetadataChanges: [
+          { changedFiles: ['package.json'], commit: 'A' },
+          { changedFiles: ['package.json'], commit: 'B' },
+        ],
+      },
+    });
+
+    await expect(findChangedDependencies(getContext(context))).resolves.toEqual([
       'react',
       'lodash',
     ]);
@@ -180,7 +236,15 @@ describe('findChangedDependencies', () => {
       dependencies: { lodash: { name: 'lodash', version: '4.18.0', dependencies: {} } },
     });
 
-    await expect(findChangedDependencies(getContext(['A']))).resolves.toEqual(['react', 'lodash']);
+    const context = getContext({
+      git: {
+        packageMetadataChanges: [
+          { changedFiles: ['package.json', 'subdir/package.json'], commit: 'A' },
+        ],
+      },
+    });
+
+    await expect(findChangedDependencies(context)).resolves.toEqual(['react', 'lodash']);
 
     // Root manifest and lock files are checked
     expect(buildDepTree).toHaveBeenCalledWith('/root', 'package.json', 'yarn.lock', true);
@@ -210,7 +274,13 @@ describe('findChangedDependencies', () => {
     checkoutFile.mockImplementation((ctx, commit, file) => Promise.resolve(`${commit}.${file}`));
     buildDepTree.mockResolvedValue({ dependencies: {} });
 
-    await expect(findChangedDependencies(getContext(['A']))).resolves.toEqual([]);
+    const context = getContext({
+      git: {
+        packageMetadataChanges: [{ changedFiles: ['yarn.lock'], commit: 'A' }],
+      },
+    });
+
+    await expect(findChangedDependencies(context)).resolves.toEqual([]);
 
     expect(buildDepTree).toHaveBeenCalledWith(
       '/root',
@@ -220,6 +290,28 @@ describe('findChangedDependencies', () => {
     );
   });
 
+  it('ignores lockfile changes if metadata file is untraced', async () => {
+    findFiles.mockImplementation((file) => {
+      if (file === 'subdir/yarn.lock') return Promise.resolve([]);
+      return Promise.resolve(file.startsWith('**') ? [file.replace('**', 'subdir')] : [file]);
+    });
+
+    const context = getContext({
+      git: {
+        // The metadata changes are filtered by untraced, but lockfile changes could still get in here
+        packageMetadataChanges: [{ changedFiles: ['yarn.lock'], commit: 'A' }],
+      },
+      options: {
+        untraced: ['package.json', 'subdir/package.json'],
+      },
+    });
+
+    await expect(findChangedDependencies(context)).resolves.toEqual([]);
+
+    expect(checkoutFile).not.to.toHaveBeenCalled();
+    expect(buildDepTree).not.toHaveBeenCalled();
+  });
+
   it('uses package-lock.json if yarn.lock is missing', async () => {
     findFiles.mockImplementation((file) => {
       if (file.endsWith('yarn.lock'))
@@ -230,7 +322,13 @@ describe('findChangedDependencies', () => {
     checkoutFile.mockImplementation((ctx, commit, file) => Promise.resolve(`${commit}.${file}`));
     buildDepTree.mockResolvedValue({ dependencies: {} });
 
-    await expect(findChangedDependencies(getContext(['A']))).resolves.toEqual([]);
+    const context = getContext({
+      git: {
+        packageMetadataChanges: [{ changedFiles: ['subdir/package-lock.json'], commit: 'A' }],
+      },
+    });
+
+    await expect(findChangedDependencies(context)).resolves.toEqual([]);
 
     expect(buildDepTree).toHaveBeenCalledWith(
       '/root',
@@ -239,38 +337,4 @@ describe('findChangedDependencies', () => {
       true
     );
   });
-
-  it('ignores manifest files matching --untraced', async () => {
-    // HEAD
-    buildDepTree.mockResolvedValueOnce({
-      dependencies: { react: { name: 'react', version: '18.2.0', dependencies: {} } },
-    });
-
-    // Baseline A
-    checkoutFile.mockResolvedValueOnce('A.package.json');
-    checkoutFile.mockResolvedValueOnce('A.yarn.lock');
-    buildDepTree.mockResolvedValueOnce({
-      dependencies: { react: { name: 'react', version: '18.3.0', dependencies: {} } },
-    });
-
-    const ctx = getContext(['A'], { untraced: ['package.json'] });
-    await expect(findChangedDependencies(ctx)).resolves.toEqual([]);
-  });
-
-  it('ignores lockfiles matching --untraced', async () => {
-    // HEAD
-    buildDepTree.mockResolvedValueOnce({
-      dependencies: { react: { name: 'react', version: '18.2.0', dependencies: {} } },
-    });
-
-    // Baseline A
-    checkoutFile.mockResolvedValueOnce('A.package.json');
-    checkoutFile.mockResolvedValueOnce('A.yarn.lock');
-    buildDepTree.mockResolvedValueOnce({
-      dependencies: { react: { name: 'react', version: '18.3.0', dependencies: {} } },
-    });
-
-    const ctx = getContext(['A'], { untraced: ['yarn.lock'] });
-    await expect(findChangedDependencies(ctx)).resolves.toEqual([]);
-  });
 });
diff --git a/node-src/lib/findChangedDependencies.ts b/node-src/lib/findChangedDependencies.ts
@@ -13,17 +13,17 @@ const YARN_LOCK = 'yarn.lock';
 // Yields a list of dependency names which have changed since the baseline.
 // E.g. ['react', 'react-dom', '@storybook/react']
 export const findChangedDependencies = async (ctx: Context) => {
-  const { baselineCommits } = ctx.git;
+  const { packageMetadataChanges } = ctx.git;
   const { untraced = [] } = ctx.options;
 
-  if (!baselineCommits.length) {
-    ctx.log.debug('No baseline commits found');
+  if (!packageMetadataChanges.length) {
+    ctx.log.debug('No package metadata changed found');
     return [];
   }
 
   ctx.log.debug(
-    { baselineCommits },
-    `Finding changed dependencies for ${baselineCommits.length} baselines`
+    { packageMetadataChanges },
+    `Finding changed dependencies for ${packageMetadataChanges.length} baselines`
   );
 
   const rootPath = await getRepositoryRoot();
@@ -40,7 +40,7 @@ export const findChangedDependencies = async (ctx: Context) => {
 
   // Handle monorepos with (multiple) nested package.json files.
   const nestedManifestPaths = await findFiles(`**/${PACKAGE_JSON}`);
-  const pathPairs = await Promise.all(
+  const metadataPathPairs = await Promise.all(
     nestedManifestPaths.map(async (manifestPath) => {
       const dirname = path.dirname(manifestPath);
       const [lockfilePath] = await findFiles(
@@ -53,36 +53,55 @@ export const findChangedDependencies = async (ctx: Context) => {
   );
 
   if (rootManifestPath && rootLockfilePath) {
-    pathPairs.unshift([rootManifestPath, rootLockfilePath]);
-  } else if (!pathPairs.length) {
+    metadataPathPairs.unshift([rootManifestPath, rootLockfilePath]);
+  } else if (!metadataPathPairs.length) {
     throw new Error(`Could not find any pairs of ${PACKAGE_JSON} + ${PACKAGE_LOCK} / ${YARN_LOCK}`);
   }
 
-  ctx.log.debug({ pathPairs }, `Found ${pathPairs.length} manifest/lockfile pairs to check`);
+  ctx.log.debug(
+    { pathPairs: metadataPathPairs },
+    `Found ${metadataPathPairs.length} manifest/lockfile pairs to check`
+  );
+
+  // Now filter out any pairs that don't have git changes, or for which the manifest is untraced
+  const filteredPathPairs = metadataPathPairs
+    .map(([manifestPath, lockfilePath]) => {
+      const commits = packageMetadataChanges
+        .filter(({ changedFiles }) =>
+          changedFiles.some((file) => file === lockfilePath || file === manifestPath)
+        )
+        .map(({ commit }) => commit);
+
+      return [manifestPath, lockfilePath, [...new Set(commits)]] as const;
+    })
+    .filter(
+      ([manifestPath, , commits]) =>
+        !untraced.some((glob) => matchesFile(glob, manifestPath)) && commits.length > 0
+    );
 
-  const tracedPairs = pathPairs.filter(([manifestPath, lockfilePath]) => {
-    if (untraced.some((glob) => matchesFile(glob, manifestPath))) return false;
-    if (untraced.some((glob) => matchesFile(glob, lockfilePath))) return false;
-    return true;
-  });
-  const untracedCount = pathPairs.length - tracedPairs.length;
-  if (untracedCount) {
-    ctx.log.debug(`Skipping ${untracedCount} manifest/lockfile pairs due to --untraced`);
+  ctx.log.debug(
+    { filteredPathPairs },
+    `Found ${filteredPathPairs.length} manifest/lockfile pairs to diff`
+  );
+
+  // Short circuit
+  if (filteredPathPairs.length === 0) {
+    return [];
   }
 
   // Use a Set so we only keep distinct package names.
   const changedDependencyNames = new Set<string>();
 
   await Promise.all(
-    tracedPairs.map(async ([manifestPath, lockfilePath]) => {
+    filteredPathPairs.map(async ([manifestPath, lockfilePath, commits]) => {
       const headDependencies = await getDependencies(ctx, { rootPath, manifestPath, lockfilePath });
       ctx.log.debug({ manifestPath, lockfilePath, headDependencies }, `Found HEAD dependencies`);
 
       // Retrieve the union of dependencies which changed compared to each baseline.
       // A change means either the version number is different or the dependency was added/removed.
       // If a manifest or lockfile is missing on the baseline, this throws and we'll end up bailing.
       await Promise.all(
-        baselineCommits.map(async (ref) => {
+        commits.map(async (ref) => {
           const baselineChanges = await compareBaseline(ctx, headDependencies, {
             ref,
             rootPath,

diff --git a/node-src/lib/findChangedPackageFiles.ts b/node-src/lib/findChangedPackageFiles.ts
@@ -1,4 +1,5 @@
 import { execGitCommand } from '../git/git';
+import { isPackageMetadataFile } from './utils';
 
 const isEqual = (left: unknown = {}, right: unknown = {}) => {
   if (typeof left !== typeof right) {
@@ -77,12 +78,14 @@ const getManifestFilesWithChangedDependencies = async (manifestFiles: string[],
 
 // Yields a list of package.json files with dependency-related changes compared to the baseline.
 export const findChangedPackageFiles = async (
-  packageManifestChanges: { changedFiles: string[]; commit: string }[]
+  packageMetadataChanges: { changedFiles: string[]; commit: string }[]
 ) => {
   const changedManifestFiles = await Promise.all(
-    packageManifestChanges.map(({ changedFiles, commit }) =>
-      getManifestFilesWithChangedDependencies(changedFiles, commit)
-    )
+    packageMetadataChanges.map(({ changedFiles, commit }) => {
+      const changedManifestFiles = changedFiles.filter(isPackageMetadataFile);
+      if (!changedManifestFiles) return [];
+      return getManifestFilesWithChangedDependencies(changedManifestFiles, commit);
+    })
   );
   // Remove duplicate entries (in case multiple ancestor builds changed the same package.json)
   return Array.from(new Set(changedManifestFiles.flat()));

diff --git a/node-src/lib/utils.ts b/node-src/lib/utils.ts
@@ -53,7 +53,13 @@ export const matchesFile = (glob: string, filepath: string) => {
 };
 
 export const isPackageManifestFile = (filePath: string) =>
-  [/^package\.json$/, /\/package\.json$/].some((re) => re.test(filePath));
+  [/(^|\/)package\.json$/].some((re) => re.test(filePath));
+
+export const isPackageLockFile = (filePath: string) =>
+  [/(^|\/)package-lock\.json$/, /(^|\/)yarn\.lock$/].some((re) => re.test(filePath));
+
+export const isPackageMetadataFile = (filePath: string) =>
+  isPackageManifestFile(filePath) || isPackageLockFile(filePath);
 
 export const redact = <T>(value: T, ...fields: string[]): T => {
   if (value === null || typeof value !== 'object') return value;