Merge pull request #988 from chromaui/231-turbosnap-should-treat-unco…

…mmitted-builds-like-missing-commits

Use replacement build for baseline build with uncommitted changes

node-src/git/findAncestorBuildWithCommit.test.ts

@@ -14,6 +14,7 @@ const makeBuild = (build: Partial<Build> = {}): Build => ({
   id: 'id',
   number: 1,
   commit: 'missing',
+  uncommittedHash: '',
   ...build,
 });
 const makeResult = (ancestorBuilds: Build[]): AncestorBuildsQueryResult => ({
@@ -35,6 +36,14 @@ describe('findAncestorBuildWithCommit', () => {
     expect(client.runQuery.mock.calls[0][1]).toMatchObject({ buildNumber: 1 });
   });
 
+  it('does not return build with uncommitted changes', async () => {
+    client.runQuery.mockReturnValue(
+      makeResult([makeBuild({ commit: 'exists', uncommittedHash: 'abc123' })])
+    );
+
+    expect(await findAncestorBuildWithCommit({ client }, 1, { page: 1, limit: 1 })).toBeNull();
+  });
+
   it('passes skip and limit and recurse', async () => {
     const toFind = makeBuild({ number: 3, commit: 'exists' });
     client.runQuery

node-src/git/findAncestorBuildWithCommit.ts

@@ -11,6 +11,7 @@ const AncestorBuildsQuery = gql`
           id
           number
           commit
+          uncommittedHash
         }
       }
     }
@@ -24,20 +25,21 @@ export interface AncestorBuildsQueryResult {
         id: string;
         number: number;
         commit: string;
+        uncommittedHash: string;
       }[];
     };
   };
 }
 
 /**
  * If we have a build who's commit no longer exists in the repository (likely a rebase/force-pushed
- * commit), search for an ancestor build which *does* have one.
+ * commit) or it had uncommitted changes, search for an ancestor build which has a clean commit.
  *
  * To do this we use the `Build.ancestorBuilds` API on the index, which will give us a set of builds
  * in reverse "git-chronological" order. That is, if we pick the first build that the API gives us
  * that has a commit, it is guaranteed to be the min number of builds "back" in Chromatic's history.
  *
- * The purpose here is to allow us to substitute a build with a known commit when doing TurboSnap.
+ * The purpose here is to allow us to substitute a build with a known clean commit for TurboSnap.
  *
  * @param {Context} context
  * @param {int} number The build number to start searching from
@@ -66,7 +68,7 @@ export async function findAncestorBuildWithCommit(
         return [build, exists] as const;
       })
     );
-    const result = results.find(([_, exists]) => exists);
+    const result = results.find(([build, exists]) => !build.uncommittedHash && exists);
 
     if (result) return result[0];
 

node-src/git/getBaselineBuilds.ts

@@ -16,6 +16,7 @@ const BaselineCommitsQuery = gql`
         status(legacy: false)
         commit
         committedAt
+        uncommittedHash
         changeCount
       }
     }
@@ -29,6 +30,7 @@ interface BaselineCommitsQueryResult {
       status: string;
       commit: string;
       committedAt: number;
+      uncommittedHash: string;
       changeCount: number;
     }[];
   };

node-src/git/getChangedFilesWithReplacement.test.ts

@@ -20,20 +20,59 @@ describe('getChangedFilesWithReplacements', () => {
 
   it('passes changedFiles on through on the happy path', async () => {
     expect(
-      await getChangedFilesWithReplacement(context, { id: 'id', number: 3, commit: 'exists' })
+      await getChangedFilesWithReplacement(context, {
+        id: 'id',
+        number: 3,
+        commit: 'exists',
+        uncommittedHash: '',
+      })
     ).toEqual({
       changedFiles: ['changed', 'files'],
     });
 
     expect(client.runQuery).not.toHaveBeenCalled();
   });
 
-  it('uses a replacement when there is one', async () => {
-    const replacementBuild = { id: 'replacement', number: 2, commit: 'exists' };
+  it('uses a replacement when build has missing commit', async () => {
+    const replacementBuild = {
+      id: 'replacement',
+      number: 2,
+      commit: 'exists',
+      uncommittedHash: '',
+    };
     client.runQuery.mockReturnValue({ app: { build: { ancestorBuilds: [replacementBuild] } } });
 
     expect(
-      await getChangedFilesWithReplacement(context, { id: 'id', number: 3, commit: 'missing' })
+      await getChangedFilesWithReplacement(context, {
+        id: 'id',
+        number: 3,
+        commit: 'missing',
+        uncommittedHash: '',
+      })
+    ).toEqual({
+      changedFiles: ['changed', 'files'],
+      replacementBuild,
+    });
+
+    expect(client.runQuery).toHaveBeenCalled();
+  });
+
+  it('uses a replacement when build has uncommitted changes', async () => {
+    const replacementBuild = {
+      id: 'replacement',
+      number: 2,
+      commit: 'exists',
+      uncommittedHash: '',
+    };
+    client.runQuery.mockReturnValue({ app: { build: { ancestorBuilds: [replacementBuild] } } });
+
+    expect(
+      await getChangedFilesWithReplacement(context, {
+        id: 'id',
+        number: 3,
+        commit: 'exists',
+        uncommittedHash: 'abcdef',
+      })
     ).toEqual({
       changedFiles: ['changed', 'files'],
       replacementBuild,
@@ -43,11 +82,21 @@ describe('getChangedFilesWithReplacements', () => {
   });
 
   it('throws if there is no replacement', async () => {
-    const replacementBuild = { id: 'replacement', number: 2, commit: 'also-missing' };
+    const replacementBuild = {
+      id: 'replacement',
+      number: 2,
+      commit: 'also-missing',
+      uncommittedHash: '',
+    };
     client.runQuery.mockReturnValue({ app: { build: { ancestorBuilds: [replacementBuild] } } });
 
     await expect(
-      getChangedFilesWithReplacement(context, { id: 'id', number: 3, commit: 'missing' })
+      getChangedFilesWithReplacement(context, {
+        id: 'id',
+        number: 3,
+        commit: 'missing',
+        uncommittedHash: '',
+      })
     ).rejects.toThrow(/fatal: bad object missing/);
   });
 });

node-src/git/getChangedFilesWithReplacement.ts

@@ -2,10 +2,11 @@ import { getChangedFiles } from './git';
 import { findAncestorBuildWithCommit } from './findAncestorBuildWithCommit';
 import { Context } from '../types';
 
-type BuildWithCommit = {
+type BuildWithCommitInfo = {
   id: string;
   number: number;
   commit: string;
+  uncommittedHash: string;
 };
 
 /**
@@ -18,17 +19,18 @@ type BuildWithCommit = {
  */
 export async function getChangedFilesWithReplacement(
   context: Context,
-  build: BuildWithCommit
-): Promise<{ changedFiles: string[]; replacementBuild?: BuildWithCommit }> {
+  build: BuildWithCommitInfo
+): Promise<{ changedFiles: string[]; replacementBuild?: BuildWithCommitInfo }> {
   try {
+    if (build.uncommittedHash) throw new Error('Build had uncommitted changes');
     const changedFiles = await getChangedFiles(build.commit);
     return { changedFiles };
   } catch (err) {
     context.log.debug(
       `Got error fetching commit for #${build.number}(${build.commit}): ${err.message}`
     );
 
-    if (err.message.match(/bad object/)) {
+    if (err.message.match(/(bad object|uncommitted changes)/)) {
       const replacementBuild = await findAncestorBuildWithCommit(context, build.number);
 
       if (replacementBuild) {

node-src/tasks/gitInfo.test.ts

@@ -111,6 +111,7 @@ describe('setGitInfo', () => {
         id: 'parent',
         number: 1,
         commit: '987bca',
+        uncommittedHash: '',
       },
     });
     const ctx = { log, options: { onlyChanged: true }, client } as any;

node-src/tasks/gitInfo.ts

@@ -178,7 +178,7 @@ export const setGitInfo = async (ctx: Context, task: Task) => {
 
     try {
       // Map the baseline builds to their changed files, falling back to an earlier "replacement"
-      // build if the baseline commit no longer exists (e.g. due to force-pushing).
+      // build if the baseline commit no longer exists or the baseline had uncommitted changes.
       const changedFilesWithInfo = await Promise.all(
         baselineBuilds.map(async (build) => {
           const changedFilesWithReplacement = await getChangedFilesWithReplacement(ctx, build);