diff --git a/bin/git.ts b/bin/git.ts index 707893f..4bc626c 100644 --- a/bin/git.ts +++ b/bin/git.ts @@ -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; /** @@ -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); diff --git a/bin/main.ts b/bin/main.ts index 5b8d868..21d2fb1 100644 --- a/bin/main.ts +++ b/bin/main.ts @@ -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'; /** @@ -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, + }); + 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', { @@ -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( @@ -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( @@ -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)); }, diff --git a/test/unit/bin/git.test.ts b/test/unit/bin/git.test.ts index ea785a1..a663b6d 100644 --- a/test/unit/bin/git.test.ts +++ b/test/unit/bin/git.test.ts @@ -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', () => { @@ -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' });