Skip to content
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
50 changes: 45 additions & 5 deletions bin/git.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,41 @@ export const getChangedFiles = (commits: Commit[]) => {
return changedFiles;
};

/**
* Returns true if the branch contains a merge commit whose parent is reachable from targetBranch
* (i.e. a genuine "merge from target" commit, not a merge of some unrelated branch).
* Uses the full targetBranch..sourceBranch range, ignoring baseCommit.
*/
export const hasMergeFromTarget = (sourceBranch: string, targetBranch: string): boolean => {
const mergeShas = spawnCommandInGhWorkspace(`git log --merges --pretty=format:%H ${targetBranch}..${sourceBranch}`)
.split('\n')
.filter(Boolean);

for (const sha of mergeShas) {
const parents = spawnCommandInGhWorkspace(`git log -1 --pretty=format:%P ${sha}`).trim().split(' ');
for (const parent of parents) {
// git merge-base A B outputs the common ancestor.
// If that equals A, then A is an ancestor of B (i.e. parent is reachable from targetBranch).
const mergeBase = spawnCommandInGhWorkspace(`git merge-base ${parent} ${targetBranch}`);
if (mergeBase === parent) {
return true;
}
}
}
return false;
};

/**
* Returns all files touched by non-merge commits on the branch (full history, ignoring baseCommit).
* Used to check whether the branch itself has any functional changes, independent of what master merged in.
*/
export const getBranchOnlyChangedFiles = (sourceBranch: string, targetBranch: string): string[] => {
const output = spawnCommandInGhWorkspace(
`git log --no-merges --name-only --pretty=format: ${targetBranch}..${sourceBranch}`,
);
return output.split('\n').filter(Boolean);
};

const SHA_REGEX = /^[0-9a-f]{40}$/i;

/**
Expand All @@ -40,17 +75,22 @@ export const parseBaseCommit = (shaOrCommit: string | undefined): string | undef
return sha;
};

const fetchAllBranchCommits = (sourceBranch: string, targetBranch: string): Commit[] => {
const commitsStrings = spawnCommandInGhWorkspace(
`git log --pretty=format:'${GIT_LOG_FORMAT}' ${targetBranch}..${sourceBranch}`,
).split('\n');
const commits = commitsStrings.map((commitString) => parseCommit(commitString));
commits.reverse();
return commits;
};

/**
* Gets the commits between sourceBranch and targetBranch (exclusive).
* - If baseCommit is provided, only returns commits after the baseCommit.
*/
export const getCommits = ({ sourceBranch, targetBranch, baseCommit }: Config): Commit[] => {
const baseCommitSha = parseBaseCommit(baseCommit);
const commitsStrings = spawnCommandInGhWorkspace(
`git log --pretty=format:'${GIT_LOG_FORMAT}' ${targetBranch}..${sourceBranch}`,
).split('\n');
const commits = commitsStrings.map((commitString) => parseCommit(commitString));
commits.reverse();
const commits = fetchAllBranchCommits(sourceBranch, targetBranch);

const baseCommitIndex = commits.findIndex((commit) => commit.sha === baseCommitSha);

Expand Down
74 changes: 52 additions & 22 deletions bin/main.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,11 @@ import { hideBin } from 'yargs/helpers';

import { deleteOldBuilds, runBuilds } from './build.js';
import { getChangedActors } from './diff-changes.js';
import { getChangedFiles, getCommits } from './git.js';
import { getBranchOnlyChangedFiles, getChangedFiles, getCommits, hasMergeFromTarget } from './git.js';
import { getPushData } from './github.js';
import { notifyToSlack } from './slack.js';
import { reportTestResults } from './test-report.js';
import type { Config } from './types.js';
import { getRepoActors, setCwd, spawnCommandInGhWorkspace } from './utils.js';

/**
Expand All @@ -34,6 +35,44 @@ const buildOptions = (y: Argv) => {
});
};

const resolveChangedActors = async (
{ targetBranch, sourceBranch, baseCommit }: Config,
{ isLatest }: { isLatest: boolean },
) => {
const actorConfigs = await getRepoActors();

// This is an optimization for the common case where a branch only has cosmetic changes but had to merge in
// functional changes from master (being up-to-date is a CI requirement). Master is already validated, and
// since the branch has no functional changes of its own, there is nothing new to validate.
// Exception: if the branch has any functional changes alongside the merge, we must re-test — even
// individually validated changes can have novel interactions when combined.
if (hasMergeFromTarget(sourceBranch, targetBranch)) {
console.error(
'[MERGE-FROM-TARGET-OPTIMIZATION]: There is merge from target branch, checking if there are no functional changes in our own branch. If so, we can skip tests',
);
const branchOnlyFiles = getBranchOnlyChangedFiles(sourceBranch, targetBranch);
// Omit baseCommit to get full branch history. Validated functional commits can still interact with merged ones
const allBranchCommits = getCommits({ sourceBranch, targetBranch, baseCommit: undefined });
const branchOnlyActorsChanged = getChangedActors({
filepathsChanged: branchOnlyFiles,
actorConfigs,
commits: allBranchCommits,
});
Comment thread
metalwarrior665 marked this conversation as resolved.
if (branchOnlyActorsChanged.length === 0) {
console.error('[MERGE-FROM-TARGET-OPTIMIZATION]: Branch itself has no functional changes, skipping tests');
return [];
}
console.error(
`[MERGE-FROM-TARGET-OPTIMIZATION]: Branch has ${branchOnlyActorsChanged.length} functional changes, cannot optimize, we continue with full check`,
);
}

// If the optimization doesn't apply, we check all branch commits including merges for full coverage. We don't reuse the merge optimization results because here we can apply baseCommit and check merge commits (they might be functional or just cosmetic)
const commits = getCommits({ targetBranch, sourceBranch, baseCommit });
const changedFiles = getChangedFiles(commits);
return getChangedActors({ filepathsChanged: changedFiles, actorConfigs, isLatest, commits });
};

await yargs()
.scriptName('public-actors-utils')
.option('dry-run', {
Expand Down Expand Up @@ -68,16 +107,11 @@ await yargs()
console.log(JSON.stringify(actorConfigs));
},
)
.command('get-affected-actors', '', buildOptions, async (args) => {
const commits = getCommits(args);
const changedFiles = getChangedFiles(commits);
const actorConfigs = await getRepoActors();
const actorsChanged = getChangedActors({
filepathsChanged: changedFiles,
actorConfigs,
isLatest: false,
commits,
});
.command('get-affected-actors', '', buildOptions, async ({ targetBranch, sourceBranch, baseCommit }) => {
const actorsChanged = await resolveChangedActors(
{ targetBranch, sourceBranch, baseCommit },
{ isLatest: false },
);
console.log(JSON.stringify(actorsChanged));
})
.command(
Expand All @@ -97,15 +131,11 @@ await yargs()
'build',
'',
(args) => buildOptions(args).option('dry-run', { type: 'boolean', default: false }),
async (args) => {
const commits = getCommits(args);
const changedFiles = getChangedFiles(commits);
const actorConfigs = await getRepoActors();
const actorsChanged = getChangedActors({
filepathsChanged: changedFiles,
actorConfigs,
commits,
});
async ({ targetBranch, sourceBranch, baseCommit, dryRun }) => {
const actorsChanged = await resolveChangedActors(
{ targetBranch, sourceBranch, baseCommit },
{ isLatest: false },
);
// https://github.com/apify-store/google-maps#:actors/lukaskrivka_google-maps-with-contact-details
// git@github.com:apify-store/google-maps#:actors/lukaskrivka_google-maps-with-contact-details
const repoUrl = spawnCommandInGhWorkspace(`git remote get-url origin`).replace(
Expand All @@ -116,8 +146,8 @@ await yargs()
const builds = await runBuilds({
repoUrl,
actorConfigs: actorsChanged,
branch: args.sourceBranch.replace('origin/', ''),
dryRun: args.dryRun,
branch: sourceBranch.replace('origin/', ''),
dryRun,
});
console.log(JSON.stringify(builds));
},
Expand Down
88 changes: 87 additions & 1 deletion test/unit/bin/git.test.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,13 @@
import type { MockInstance } from 'vitest';
import { beforeEach, describe, expect, it, vi } from 'vitest';

import { getChangedFiles, getCommits, parseBaseCommit } from '../../../bin/git.js';
import {
getBranchOnlyChangedFiles,
getChangedFiles,
getCommits,
hasMergeFromTarget,
parseBaseCommit,
} from '../../../bin/git.js';
import * as Utils from '../../../bin/utils.js';

describe('getCommits', () => {
Expand Down Expand Up @@ -117,6 +123,86 @@ describe('getChangedFiles', () => {
});
});

describe('hasMergeFromTarget', () => {
const sourceBranch = 'feature-branch';
const targetBranch = 'main';
const mergeSha = 'f'.repeat(40);
const branchParentSha = 'b'.repeat(40);
const targetParentSha = 't'.repeat(40);

let gitCommandSpy: MockInstance;

beforeEach(() => {
gitCommandSpy = vi.spyOn(Utils, 'spawnCommandInGhWorkspace');
});

it('should return false when there are no merge commits on the branch', () => {
gitCommandSpy.mockImplementation((cmd: string) => {
if (cmd.includes('--merges')) return '';
return '';
});

expect(hasMergeFromTarget(sourceBranch, targetBranch)).toBe(false);
expect(gitCommandSpy).toHaveBeenCalledWith(
`git log --merges --pretty=format:%H ${targetBranch}..${sourceBranch}`,
);
});

it('should return true when a merge commit has a parent reachable from targetBranch', () => {
gitCommandSpy.mockImplementation((cmd: string) => {
if (cmd.includes('--merges')) return mergeSha;
if (cmd.includes('--pretty=format:%P')) return `${branchParentSha} ${targetParentSha}`;
if (cmd.startsWith(`git merge-base ${branchParentSha}`)) return branchParentSha; // not ancestor
if (cmd.startsWith(`git merge-base ${targetParentSha}`)) return targetParentSha; // is ancestor
return '';
});

expect(hasMergeFromTarget(sourceBranch, targetBranch)).toBe(true);
});

it('should return false when the merge commit parent is not reachable from targetBranch (unrelated branch merge)', () => {
const unrelatedSha = 'e'.repeat(40);
const differentMergeBase = '0'.repeat(40);
gitCommandSpy.mockImplementation((cmd: string) => {
if (cmd.includes('--merges')) return mergeSha;
if (cmd.includes('--pretty=format:%P')) return `${branchParentSha} ${unrelatedSha}`;
// merge-base returns something other than the parent — not an ancestor
if (cmd.startsWith('git merge-base')) return differentMergeBase;
return '';
});

expect(hasMergeFromTarget(sourceBranch, targetBranch)).toBe(false);
});
});

describe('getBranchOnlyChangedFiles', () => {
const sourceBranch = 'feature-branch';
const targetBranch = 'main';

let gitCommandSpy: MockInstance;

beforeEach(() => {
gitCommandSpy = vi.spyOn(Utils, 'spawnCommandInGhWorkspace');
});

it('should return files touched by non-merge commits', () => {
gitCommandSpy.mockReturnValue('README.md\n\nactors/foo_bar/src/main.ts\n');

const result = getBranchOnlyChangedFiles(sourceBranch, targetBranch);

expect(result).toStrictEqual(['README.md', 'actors/foo_bar/src/main.ts']);
expect(gitCommandSpy).toHaveBeenCalledWith(
`git log --no-merges --name-only --pretty=format: ${targetBranch}..${sourceBranch}`,
);
});

it('should return empty array when there are no non-merge commits', () => {
gitCommandSpy.mockReturnValue('');

expect(getBranchOnlyChangedFiles(sourceBranch, targetBranch)).toStrictEqual([]);
});
});

const VALID_SHA = 'a'.repeat(40);
const VALID_JSON = JSON.stringify({ sha: VALID_SHA, author: 'test', date: 'now', message: 'msg' });

Expand Down
Loading