Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use replacement build for baseline build with uncommitted changes #988

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions node-src/git/findAncestorBuildWithCommit.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ const makeBuild = (build: Partial<Build> = {}): Build => ({
id: 'id',
number: 1,
commit: 'missing',
uncommittedHash: '',
...build,
});
const makeResult = (ancestorBuilds: Build[]): AncestorBuildsQueryResult => ({
Expand All @@ -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
Expand Down
8 changes: 5 additions & 3 deletions node-src/git/findAncestorBuildWithCommit.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ const AncestorBuildsQuery = gql`
id
number
commit
uncommittedHash
}
}
}
Expand All @@ -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
Expand Down Expand Up @@ -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];

Expand Down
2 changes: 2 additions & 0 deletions node-src/git/getBaselineBuilds.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ const BaselineCommitsQuery = gql`
status(legacy: false)
commit
committedAt
uncommittedHash
tmeasday marked this conversation as resolved.
Show resolved Hide resolved
changeCount
}
}
Expand All @@ -29,6 +30,7 @@ interface BaselineCommitsQueryResult {
status: string;
commit: string;
committedAt: number;
uncommittedHash: string;
changeCount: number;
}[];
};
Expand Down
61 changes: 55 additions & 6 deletions node-src/git/getChangedFilesWithReplacement.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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/);
});
});
10 changes: 6 additions & 4 deletions node-src/git/getChangedFilesWithReplacement.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
};

/**
Expand All @@ -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)/)) {
ghengeveld marked this conversation as resolved.
Show resolved Hide resolved
const replacementBuild = await findAncestorBuildWithCommit(context, build.number);

if (replacementBuild) {
Expand Down
1 change: 1 addition & 0 deletions node-src/tasks/gitInfo.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,7 @@ describe('setGitInfo', () => {
id: 'parent',
number: 1,
commit: '987bca',
uncommittedHash: '',
},
});
const ctx = { log, options: { onlyChanged: true }, client } as any;
Expand Down
2 changes: 1 addition & 1 deletion node-src/tasks/gitInfo.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
Loading