From 975d49753412647187f7c5595aed7bf963f59271 Mon Sep 17 00:00:00 2001 From: Kenil Shah Date: Thu, 18 Jun 2026 15:25:03 +0530 Subject: [PATCH] feat: enhance PRAuthor to manage branch existence --- service/src/github/pr-author.test.ts | 127 ++++++++++++++++-- service/src/github/pr-author.ts | 100 ++++++++++---- .../jobs/processors/coverage-analysis.test.ts | 74 +++++++++- .../src/jobs/processors/coverage-analysis.ts | 17 ++- 4 files changed, 272 insertions(+), 46 deletions(-) diff --git a/service/src/github/pr-author.test.ts b/service/src/github/pr-author.test.ts index e87a0e7..9192a1b 100644 --- a/service/src/github/pr-author.test.ts +++ b/service/src/github/pr-author.test.ts @@ -36,12 +36,6 @@ describe('PRAuthor.commitFiles', () => { patch: vi.fn(), }; - // 1. baseSha tree resolution - api.get.mockImplementationOnce(async (url: string) => { - expect(url).toBe('/repos/kenil27/band/git/commits/sha-base'); - return { data: { tree: { sha: 'tree-base' } } }; - }); - // refExists: branch lookup returns 404 api.get.mockImplementationOnce(async (url: string) => { expect(url).toBe('/repos/kenil27/band/git/ref/heads/openreview/tests/pr-1'); @@ -51,6 +45,12 @@ describe('PRAuthor.commitFiles', () => { throw err; }); + // baseSha tree resolution + api.get.mockImplementationOnce(async (url: string) => { + expect(url).toBe('/repos/kenil27/band/git/commits/sha-base'); + return { data: { tree: { sha: 'tree-base' } } }; + }); + api.post .mockResolvedValueOnce({ data: { sha: 'blob-a' } }) // create blob .mockResolvedValueOnce({ data: { sha: 'tree-new' } }) // create tree @@ -86,6 +86,16 @@ describe('PRAuthor.commitFiles', () => { ], }); + // Check commit parents on the feature tip for a brand-new branch + const commitCall = api.post.mock.calls[2]; + expect(commitCall[0]).toBe('/repos/kenil27/band/git/commits'); + expect(commitCall[1]).toEqual( + expect.objectContaining({ + message: 'test: add a', + parents: ['sha-base'], + }), + ); + // Check ref creation used the fully qualified ref string and POST (not PATCH) const refCall = api.post.mock.calls[3]; expect(refCall[0]).toBe('/repos/kenil27/band/git/refs'); @@ -96,12 +106,16 @@ describe('PRAuthor.commitFiles', () => { expect(api.patch).not.toHaveBeenCalled(); }); - it('force-updates an existing branch with PATCH', async () => { + it('appends a commit when the feature branch has not advanced', async () => { const api = { get: vi.fn(), post: vi.fn(), patch: vi.fn() }; api.get - .mockResolvedValueOnce({ data: { tree: { sha: 'tree-base' } } }) - .mockResolvedValueOnce({ data: { ref: 'refs/heads/x' } }); // ref exists + .mockResolvedValueOnce({ data: { object: { sha: 'tip-old' } } }) // refExists + .mockResolvedValueOnce({ data: { object: { sha: 'tip-old' } } }) // getRefSha + .mockResolvedValueOnce({ + data: { merge_base_commit: { sha: 'sha-base' } }, + }) // isAncestor + .mockResolvedValueOnce({ data: { tree: { sha: 'tree-head' } } }); // headSha tree api.post .mockResolvedValueOnce({ data: { sha: 'blob-1' } }) @@ -118,9 +132,64 @@ describe('PRAuthor.commitFiles', () => { commitMessage: 'msg', }); + const treeCall = api.post.mock.calls[1]; + expect(treeCall[1]).toEqual({ + base_tree: 'tree-head', + tree: [ + { + path: 'tests/a.test.ts', + mode: '100644', + type: 'blob', + sha: 'blob-1', + }, + ], + }); + + const commitCall = api.post.mock.calls[2]; + expect(commitCall[1]).toEqual( + expect.objectContaining({ + parents: ['tip-old'], + }), + ); + expect(api.patch).toHaveBeenCalledWith( '/repos/kenil27/band/git/refs/heads/openreview/tests/pr-42', - { sha: 'commit-1', force: true }, + { sha: 'commit-1' }, + ); + expect(api.patch.mock.calls[0][1]).not.toHaveProperty('force'); + }); + + it('creates a merge commit when the feature branch has advanced', async () => { + const api = { get: vi.fn(), post: vi.fn(), patch: vi.fn() }; + + api.get + .mockResolvedValueOnce({ data: { object: { sha: 'tip-old' } } }) // refExists + .mockResolvedValueOnce({ data: { object: { sha: 'tip-old' } } }) // getRefSha + .mockResolvedValueOnce({ + data: { merge_base_commit: { sha: 'merge-old' } }, + }) // isAncestor — feature tip is NOT merged + .mockResolvedValueOnce({ data: { tree: { sha: 'tree-head' } } }); // headSha tree + + api.post + .mockResolvedValueOnce({ data: { sha: 'blob-1' } }) + .mockResolvedValueOnce({ data: { sha: 'tree-1' } }) + .mockResolvedValueOnce({ data: { sha: 'commit-1' } }); + + api.patch.mockResolvedValueOnce({ data: {} }); + + const author = new PRAuthor(makeClient(api)); + await author.commitFiles({ + branch: 'openreview/tests/pr-42', + baseSha: 'sha-head', + files: [{ path: 'tests/a.test.ts', content: 'noop' }], + commitMessage: 'msg', + }); + + const commitCall = api.post.mock.calls[2]; + expect(commitCall[1]).toEqual( + expect.objectContaining({ + parents: ['tip-old', 'sha-head'], + }), ); }); }); @@ -145,6 +214,7 @@ describe('PRAuthor.openOrUpdatePR', () => { url: 'https://github.com/kenil27/band/pull/99', number: 99, created: true, + updated: false, }); expect(api.get).toHaveBeenCalledWith( @@ -169,25 +239,54 @@ describe('PRAuthor.openOrUpdatePR', () => { ); }); - it('returns the existing PR without creating a new one', async () => { - const api = { get: vi.fn(), post: vi.fn() }; + it('PATCHes title and body on an existing open PR', async () => { + const api = { get: vi.fn(), post: vi.fn(), patch: vi.fn() }; api.get.mockResolvedValueOnce({ data: [{ number: 7, html_url: 'https://github.com/kenil27/band/pull/7' }], }); + api.patch.mockResolvedValueOnce({ data: {} }); const author = new PRAuthor(makeClient(api)); const res = await author.openOrUpdatePR({ base: 'feature/foo', head: 'openreview/tests/pr-1', - title: 't', - body: 'b', + title: 'updated title', + body: 'updated body', }); expect(res).toEqual({ url: 'https://github.com/kenil27/band/pull/7', number: 7, created: false, + updated: true, + }); + expect(api.patch).toHaveBeenCalledWith('/repos/kenil27/band/pulls/7', { + title: 'updated title', + body: 'updated body', }); expect(api.post).not.toHaveBeenCalled(); }); }); + +describe('PRAuthor.branchExists', () => { + it('returns true when the branch ref is present', async () => { + const api = { get: vi.fn(), post: vi.fn(), patch: vi.fn() }; + api.get.mockResolvedValueOnce({ data: { ref: 'refs/heads/openreview/tests/pr-1' } }); + + const author = new PRAuthor(makeClient(api)); + await expect(author.branchExists('openreview/tests/pr-1')).resolves.toBe(true); + expect(api.get).toHaveBeenCalledWith( + '/repos/kenil27/band/git/ref/heads/openreview/tests/pr-1', + ); + }); + + it('returns false when the branch ref is missing', async () => { + const api = { get: vi.fn(), post: vi.fn(), patch: vi.fn() }; + api.get.mockRejectedValueOnce( + Object.assign(new Error('not found'), { response: { status: 404 } }), + ); + + const author = new PRAuthor(makeClient(api)); + await expect(author.branchExists('openreview/tests/pr-99')).resolves.toBe(false); + }); +}); diff --git a/service/src/github/pr-author.ts b/service/src/github/pr-author.ts index ca6a5c7..df855c5 100644 --- a/service/src/github/pr-author.ts +++ b/service/src/github/pr-author.ts @@ -30,11 +30,19 @@ export class PRAuthor { } /** - * Commit one or more files atomically on top of `baseSha`, creating or - * fast-forwarding `branch` to point at the new commit. + * Commit generated test files on `branch`. * - * Empty `files` is a no-op that returns `null` so the caller can decide - * whether to skip PR creation. + * Stacked test PRs target the feature branch (`headRef`) and should only + * *show* test-file deltas in the PR diff. We always build the commit tree + * from the current feature tip (`baseSha`) plus test overlays — never + * re-import an older snapshot of source files. + * + * History strategy: + * - **First push:** single parent = feature tip. + * - **Refresh, feature unchanged:** single parent = test-branch tip. + * - **Refresh, feature advanced:** merge commit with parents + * `[test-branch tip, feature tip]` so the branch stays aligned with the + * feature branch and GitHub's PR diff does not resurrect `.js` changes. */ async commitFiles(opts: { branch: string; @@ -44,13 +52,24 @@ export class PRAuthor { }): Promise<{ branchRef: string; commitSha: string } | null> { if (opts.files.length === 0) return null; - // 1) Resolve the tree SHA at baseSha so we can extend it. - const { data: baseCommit } = await this.api.get( - `${this.repoPath}/git/commits/${opts.baseSha}`, - ); - const baseTreeSha = baseCommit.tree.sha as string; + const refPath = `heads/${opts.branch}`; + const fullyQualified = `refs/heads/${opts.branch}`; + const headSha = opts.baseSha; + const branchExists = await this.refExists(refPath); + + let parents: string[]; + if (!branchExists) { + parents = [headSha]; + } else { + const branchTip = await this.getRefSha(refPath); + const headMerged = + branchTip === headSha || + (await this.isAncestor(headSha, branchTip)); + parents = headMerged ? [branchTip] : [branchTip, headSha]; + } + + const baseTreeSha = await this.getCommitTreeSha(headSha); - // 2) Upload each file as a blob. const blobs = await Promise.all( opts.files.map(async (f) => { const { data } = await this.api.post(`${this.repoPath}/git/blobs`, { @@ -61,7 +80,6 @@ export class PRAuthor { }), ); - // 3) Build a new tree referencing the new blobs. const { data: newTree } = await this.api.post( `${this.repoPath}/git/trees`, { @@ -75,25 +93,19 @@ export class PRAuthor { }, ); - // 4) Create the commit pointing at the new tree, parented on baseSha. const { data: newCommit } = await this.api.post( `${this.repoPath}/git/commits`, { message: opts.commitMessage, tree: newTree.sha, - parents: [opts.baseSha], + parents, }, ); const commitSha = newCommit.sha as string; - // 5) Create the branch ref, or fast-forward an existing one. - const refPath = `heads/${opts.branch}`; - const fullyQualified = `refs/heads/${opts.branch}`; - const exists = await this.refExists(refPath); - if (exists) { + if (branchExists) { await this.api.patch(`${this.repoPath}/git/refs/${refPath}`, { sha: commitSha, - force: true, }); } else { await this.api.post(`${this.repoPath}/git/refs`, { @@ -105,20 +117,35 @@ export class PRAuthor { return { branchRef: fullyQualified, commitSha }; } + /** Whether `refs/heads/` already exists (used to pick a commit message). */ + async branchExists(branch: string): Promise { + return this.refExists(`heads/${branch}`); + } + /** - * Open a PR with `head` -> `base`, or return the open one if it exists. - * The {@link created} flag tells the caller whether to post a fresh - * "tests generated" comment or to skip it. + * Open a PR with `head` -> `base`, or refresh title/body on the open one. + * + * When a matching open PR exists, its title and body are PATCHed so + * re-runs after `pull_request.synchronize` show up-to-date coverage stats. */ async openOrUpdatePR(opts: { base: string; head: string; title: string; body: string; - }): Promise<{ url: string; number: number; created: boolean }> { + }): Promise<{ url: string; number: number; created: boolean; updated: boolean }> { const existing = await this.findOpenPR(opts.head, opts.base); if (existing) { - return { url: existing.url, number: existing.number, created: false }; + await this.api.patch(`${this.repoPath}/pulls/${existing.number}`, { + title: opts.title, + body: opts.body, + }); + return { + url: existing.url, + number: existing.number, + created: false, + updated: true, + }; } const { data } = await this.api.post(`${this.repoPath}/pulls`, { @@ -132,11 +159,36 @@ export class PRAuthor { url: data.html_url as string, number: data.number as number, created: true, + updated: false, }; } /* ---- Internal helpers --------------------------------------------- */ + private async getRefSha(refPath: string): Promise { + const { data } = await this.api.get(`${this.repoPath}/git/ref/${refPath}`); + return data.object.sha as string; + } + + private async getCommitTreeSha(commitSha: string): Promise { + const { data } = await this.api.get( + `${this.repoPath}/git/commits/${commitSha}`, + ); + return data.tree.sha as string; + } + + /** True when `ancestorSha` is reachable from `descendantSha` (inclusive). */ + private async isAncestor( + ancestorSha: string, + descendantSha: string, + ): Promise { + if (ancestorSha === descendantSha) return true; + const { data } = await this.api.get( + `${this.repoPath}/compare/${ancestorSha}...${descendantSha}`, + ); + return (data.merge_base_commit?.sha as string | undefined) === ancestorSha; + } + private async refExists(refPath: string): Promise { try { await this.api.get(`${this.repoPath}/git/ref/${refPath}`); diff --git a/service/src/jobs/processors/coverage-analysis.test.ts b/service/src/jobs/processors/coverage-analysis.test.ts index bcda250..656c894 100644 --- a/service/src/jobs/processors/coverage-analysis.test.ts +++ b/service/src/jobs/processors/coverage-analysis.test.ts @@ -124,6 +124,7 @@ describe('processCoverageAnalysis', () => { }; const prAuthor = { + branchExists: vi.fn(async () => false), commitFiles: vi.fn(async () => ({ branchRef: 'refs/heads/openreview/tests/pr-1', commitSha: 'commit-new', @@ -132,6 +133,7 @@ describe('processCoverageAnalysis', () => { url: 'https://github.com/kenil27/band/pull/99', number: 99, created: true, + updated: false, })), }; @@ -148,7 +150,10 @@ describe('processCoverageAnalysis', () => { 'findOrCreateRepository' | 'triggerAnalysis' | 'waitForPrRun' >, prAuthorFactory: () => - prAuthor as unknown as Pick, + prAuthor as unknown as Pick< + PRAuthor, + 'branchExists' | 'commitFiles' | 'openOrUpdatePR' + >, }); expect(coverageClient.findOrCreateRepository).toHaveBeenCalledWith( @@ -185,6 +190,66 @@ describe('processCoverageAnalysis', () => { expect(summary).toContain('https://github.com/kenil27/band/pull/99'); }); + it('uses a refresh commit message when the stacked branch already exists', async () => { + makeRuntime(); + + const coverageClient = { + findOrCreateRepository: vi.fn(async () => baseRepo), + triggerAnalysis: vi.fn(async () => ({ prRunId: 'run-1', status: 'enqueued' })), + waitForPrRun: vi.fn( + async (): Promise => ({ + ...baseRun, + generatedTestFiles: [ + { + id: 't1', + filePath: 'tests/a.test.ts', + targetFile: 'src/a.ts', + passed: true, + fileContent: 'test("x", () => {});', + }, + ], + }), + ), + }; + + const prAuthor = { + branchExists: vi.fn(async () => true), + commitFiles: vi.fn(async () => ({ + branchRef: 'refs/heads/openreview/tests/pr-1', + commitSha: 'commit-refresh', + })), + openOrUpdatePR: vi.fn(async () => ({ + url: 'https://github.com/kenil27/band/pull/7', + number: 7, + created: false, + updated: true, + })), + }; + + await processCoverageAnalysis(job, { + auth, + logger: makeLogger(), + cfg, + coverageClientFactory: () => + coverageClient as unknown as Pick< + CoverageServiceClient, + 'findOrCreateRepository' | 'triggerAnalysis' | 'waitForPrRun' + >, + prAuthorFactory: () => + prAuthor as unknown as Pick< + PRAuthor, + 'branchExists' | 'commitFiles' | 'openOrUpdatePR' + >, + }); + + expect(prAuthor.commitFiles).toHaveBeenCalledWith( + expect.objectContaining({ + commitMessage: expect.stringContaining('refresh tests for PR #1 @ sha-hea'), + }), + ); + expect(prAuthor.openOrUpdatePR).toHaveBeenCalled(); + }); + it('skips triggerAnalysis on retry when prRunId is already persisted', async () => { makeRuntime(); @@ -220,7 +285,7 @@ describe('processCoverageAnalysis', () => { triggerAnalysis: vi.fn(async () => ({ prRunId: 'run-1' })), waitForPrRun: vi.fn(async (): Promise => baseRun), }; - const prAuthor = { commitFiles: vi.fn(), openOrUpdatePR: vi.fn() }; + const prAuthor = { branchExists: vi.fn(), commitFiles: vi.fn(), openOrUpdatePR: vi.fn() }; await processCoverageAnalysis(job, { auth, @@ -232,7 +297,10 @@ describe('processCoverageAnalysis', () => { 'findOrCreateRepository' | 'triggerAnalysis' | 'waitForPrRun' >, prAuthorFactory: () => - prAuthor as unknown as Pick, + prAuthor as unknown as Pick< + PRAuthor, + 'branchExists' | 'commitFiles' | 'openOrUpdatePR' + >, }); expect(prAuthor.commitFiles).not.toHaveBeenCalled(); diff --git a/service/src/jobs/processors/coverage-analysis.ts b/service/src/jobs/processors/coverage-analysis.ts index 46d410f..ea3f02a 100644 --- a/service/src/jobs/processors/coverage-analysis.ts +++ b/service/src/jobs/processors/coverage-analysis.ts @@ -40,7 +40,7 @@ export interface CoverageAnalysisDeps { /** Injection seam for tests. */ prAuthorFactory?: ( client: GitHubClient, - ) => Pick; + ) => Pick; } /** @@ -205,14 +205,20 @@ async function openStackedTestPR(args: { const author = (args.prAuthorFactory ?? ((c) => new PRAuthor(c)))(client); const branch = `${cfg.coverageServiceBranchPrefix || 'openreview/tests'}/pr-${job.prNumber}`; + const branchExisted = await author.branchExists(branch); + const shortSha = job.headSha.slice(0, 7); + + const commitMessage = branchExisted + ? `test: OpenReview — refresh tests for PR #${job.prNumber} @ ${shortSha}\n\nAuto-generated by the coverage service.` + : `test: OpenReview — add ${files.length} generated test file${ + files.length === 1 ? '' : 's' + } for PR #${job.prNumber}\n\nAuto-generated by the coverage service.`; const commit = await author.commitFiles({ branch, baseSha: job.headSha, files, - commitMessage: `test: OpenReview — add ${files.length} generated test file${ - files.length === 1 ? '' : 's' - } for PR #${job.prNumber}\n\nAuto-generated by the coverage service.`, + commitMessage, }); if (!commit) { @@ -237,9 +243,10 @@ async function openStackedTestPR(args: { commitSha: commit.commitSha, testPrNumber: opened.number, created: opened.created, + updated: opened.updated, fileCount: files.length, }, - 'stacked test PR ready', + opened.updated ? 'stacked test PR updated' : 'stacked test PR ready', ); return { url: opened.url, count: files.length };