From d5cc74c01a88cfeb81f60795e1aa3bbbd9dafae4 Mon Sep 17 00:00:00 2001 From: metalwarrior665 Date: Wed, 8 Apr 2026 10:24:24 +0200 Subject: [PATCH 1/3] perf(bin): don't test if we merge master into cosmetic only changes --- bin/git.ts | 50 +++++++++++++++++++--- bin/main.ts | 68 ++++++++++++++++++++---------- test/unit/bin/git.test.ts | 88 ++++++++++++++++++++++++++++++++++++++- 3 files changed, 178 insertions(+), 28 deletions(-) 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..741eb38 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,38 @@ 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)) { + 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 builds'); + return []; + } + } + + // 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 +101,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 +125,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 +140,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' }); From 8e0a9d42b6b426fd118b5d89cfdfd33bfdda33ac Mon Sep 17 00:00:00 2001 From: metalwarrior665 Date: Wed, 8 Apr 2026 10:26:52 +0200 Subject: [PATCH 2/3] increment version --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index d7ef1ad..128012f 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "apify-test-tools", - "version": "0.6.2", + "version": "0.6.4", "type": "module", "description": "TBD", "repository": { From 4670e4ab4ea5b35767589459e00aba052ea42519 Mon Sep 17 00:00:00 2001 From: metalwarrior665 Date: Wed, 8 Apr 2026 21:47:59 +0200 Subject: [PATCH 3/3] more comments --- bin/main.ts | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/bin/main.ts b/bin/main.ts index 741eb38..21d2fb1 100644 --- a/bin/main.ts +++ b/bin/main.ts @@ -47,6 +47,9 @@ const resolveChangedActors = async ( // 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 }); @@ -56,9 +59,12 @@ const resolveChangedActors = async ( commits: allBranchCommits, }); if (branchOnlyActorsChanged.length === 0) { - console.error('[MERGE-FROM-TARGET-OPTIMIZATION]: Branch itself has no functional changes, skipping builds'); + 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)