From be8d5e7efff25834b04d6de4abb0b42a99ecbf56 Mon Sep 17 00:00:00 2001 From: aGallea Date: Wed, 25 Mar 2026 09:34:43 +0200 Subject: [PATCH 1/2] fix: use pulls.listFiles API for PR changed files The compareCommitsWithBasehead API can return 404 for PRs, likely due to URL encoding of branch names containing slashes (e.g. fix/something becomes fix%2Fsomething in the API path). Switch to pulls.listFiles for PR events, which uses the PR number instead of branch names and is the canonical API for getting PR changed files. Keep compareCommitsWithBasehead as fallback for push events. Also wrap getChangedFiles in try-catch in main.ts so the coverage report still posts even if changed file detection fails (just without diff coverage). Co-Authored-By: Claude Opus 4.6 (1M context) --- dist/index.js | 100 +++++++++++++++++++++++++++----------- src/changedFiles.ts | 86 ++++++++++++++++++++------------ src/eventInfo.ts | 2 + src/main.ts | 20 +++++++- src/types.d.ts | 1 + test/actions.spy.ts | 5 ++ test/changedFiles.test.ts | 41 +++++++--------- 7 files changed, 170 insertions(+), 85 deletions(-) diff --git a/dist/index.js b/dist/index.js index 21eb5d5..6bbba8b 100644 --- a/dist/index.js +++ b/dist/index.js @@ -9,40 +9,65 @@ Object.defineProperty(exports, "__esModule", ({ value: true })); exports.getChangedFiles = void 0; const github_1 = __nccwpck_require__(3228); +const emptyFilesStatus = () => ({ + all: [], + added: [], + removed: [], + modified: [], + renamed: [], + copied: [], + changed: [], + unchanged: [], +}); +const addFile = (allFiles, filename, status) => { + allFiles.all.push(filename); + const key = status; + if (key !== 'all' && key in allFiles) { + allFiles[key].push(filename); + } +}; const getChangedFiles = async (eventInfo) => { - const allFiles = { - all: [], - added: [], - removed: [], - modified: [], - renamed: [], - copied: [], - changed: [], - unchanged: [], - }; + const allFiles = emptyFilesStatus(); const octokit = (0, github_1.getOctokit)(eventInfo.token); - const perPage = 50; - let currPage = 1; - let hasMorePages = true; - while (hasMorePages) { - const { data: { files }, } = await octokit.rest.repos.compareCommitsWithBasehead({ - owner: eventInfo.owner, - repo: eventInfo.repo, - basehead: `${eventInfo.baseRef}...${eventInfo.headRef}`, - per_page: perPage, - page: currPage, - }); - if (files) { + if (eventInfo.prNumber) { + const perPage = 50; + let currPage = 1; + let hasMorePages = true; + while (hasMorePages) { + const { data: files } = await octokit.rest.pulls.listFiles({ + owner: eventInfo.owner, + repo: eventInfo.repo, + pull_number: eventInfo.prNumber, + per_page: perPage, + page: currPage, + }); for (const file of files) { - allFiles.all.push(file.filename); - const status = `${file.status}`; - if (status !== 'all' && status in allFiles) { - allFiles[status].push(file.filename); + addFile(allFiles, file.filename, `${file.status}`); + } + hasMorePages = files.length >= perPage; + currPage++; + } + } + else if (eventInfo.baseRef && eventInfo.headRef) { + const perPage = 50; + let currPage = 1; + let hasMorePages = true; + while (hasMorePages) { + const { data: { files }, } = await octokit.rest.repos.compareCommitsWithBasehead({ + owner: eventInfo.owner, + repo: eventInfo.repo, + basehead: `${eventInfo.baseRef}...${eventInfo.headRef}`, + per_page: perPage, + page: currPage, + }); + if (files) { + for (const file of files) { + addFile(allFiles, file.filename, `${file.status}`); } } + hasMorePages = (files?.length ?? 0) >= perPage; + currPage++; } - hasMorePages = (files?.length ?? 0) >= perPage; - currPage++; } return allFiles; }; @@ -450,12 +475,14 @@ const getEventInfo = () => { commitSha: '', headRef: '', baseRef: '', + prNumber: undefined, pwd: process.env.GITHUB_WORKSPACE || '', }; if (github_1.context.eventName === 'pull_request' && github_1.context.payload) { eventInfo.commitSha = github_1.context.payload.pull_request?.head.sha; eventInfo.headRef = github_1.context.payload.pull_request?.head.ref; eventInfo.baseRef = github_1.context.payload.pull_request?.base.ref; + eventInfo.prNumber = github_1.context.payload.pull_request?.number; } else if (github_1.context.eventName === 'push') { eventInfo.commitSha = github_1.context.payload.after; @@ -586,7 +613,22 @@ const main = async () => { : [], junit: eventInfo.showJunit ? await (0, junit_1.parse)(eventInfo.junitPath) : undefined, }; - const changedFile = await (0, changedFiles_1.getChangedFiles)(eventInfo); + let changedFile = { + all: [], + added: [], + removed: [], + modified: [], + renamed: [], + copied: [], + changed: [], + unchanged: [], + }; + try { + changedFile = await (0, changedFiles_1.getChangedFiles)(eventInfo); + } + catch (error) { + core.warning(`Failed to get changed files: ${error instanceof Error ? error.message : String(error)}`); + } const diffInfo = await (0, diffCover_1.diffCover)(eventInfo, changedFile, coverageInfo); await (0, commentCoverage_1.commentCoverage)(eventInfo, (0, commentCoverage_1.buildBody)(eventInfo, coverageInfo.junit, diffInfo)); } diff --git a/src/changedFiles.ts b/src/changedFiles.ts index e1cc53b..2dad4c9 100644 --- a/src/changedFiles.ts +++ b/src/changedFiles.ts @@ -1,43 +1,69 @@ import { getOctokit } from '@actions/github'; import { FilesStatus, EventInfo } from './types'; +const emptyFilesStatus = (): FilesStatus => ({ + all: [], + added: [], + removed: [], + modified: [], + renamed: [], + copied: [], + changed: [], + unchanged: [], +}); + +const addFile = (allFiles: FilesStatus, filename: string, status: string): void => { + allFiles.all.push(filename); + const key = status as keyof FilesStatus; + if (key !== 'all' && key in allFiles) { + allFiles[key].push(filename); + } +}; + export const getChangedFiles = async (eventInfo: EventInfo): Promise => { - const allFiles: FilesStatus = { - all: [], - added: [], - removed: [], - modified: [], - renamed: [], - copied: [], - changed: [], - unchanged: [], - }; + const allFiles = emptyFilesStatus(); const octokit = getOctokit(eventInfo.token); - const perPage = 50; - let currPage = 1; - let hasMorePages = true; - while (hasMorePages) { - const { - data: { files }, - } = await octokit.rest.repos.compareCommitsWithBasehead({ - owner: eventInfo.owner, - repo: eventInfo.repo, - basehead: `${eventInfo.baseRef}...${eventInfo.headRef}`, - per_page: perPage, - page: currPage, - }); - if (files) { + if (eventInfo.prNumber) { + const perPage = 50; + let currPage = 1; + let hasMorePages = true; + while (hasMorePages) { + const { data: files } = await octokit.rest.pulls.listFiles({ + owner: eventInfo.owner, + repo: eventInfo.repo, + pull_number: eventInfo.prNumber, + per_page: perPage, + page: currPage, + }); for (const file of files) { - allFiles.all.push(file.filename); - const status = `${file.status}` as keyof FilesStatus; - if (status !== 'all' && status in allFiles) { - allFiles[status].push(file.filename); + addFile(allFiles, file.filename, `${file.status}`); + } + hasMorePages = files.length >= perPage; + currPage++; + } + } else if (eventInfo.baseRef && eventInfo.headRef) { + const perPage = 50; + let currPage = 1; + let hasMorePages = true; + while (hasMorePages) { + const { + data: { files }, + } = await octokit.rest.repos.compareCommitsWithBasehead({ + owner: eventInfo.owner, + repo: eventInfo.repo, + basehead: `${eventInfo.baseRef}...${eventInfo.headRef}`, + per_page: perPage, + page: currPage, + }); + if (files) { + for (const file of files) { + addFile(allFiles, file.filename, `${file.status}`); } } + hasMorePages = (files?.length ?? 0) >= perPage; + currPage++; } - hasMorePages = (files?.length ?? 0) >= perPage; - currPage++; } return allFiles; }; diff --git a/src/eventInfo.ts b/src/eventInfo.ts index e2ea19a..2803d09 100644 --- a/src/eventInfo.ts +++ b/src/eventInfo.ts @@ -38,12 +38,14 @@ export const getEventInfo = (): EventInfo => { commitSha: '', headRef: '', baseRef: '', + prNumber: undefined, pwd: process.env.GITHUB_WORKSPACE || '', }; if (context.eventName === 'pull_request' && context.payload) { eventInfo.commitSha = context.payload.pull_request?.head.sha; eventInfo.headRef = context.payload.pull_request?.head.ref; eventInfo.baseRef = context.payload.pull_request?.base.ref; + eventInfo.prNumber = context.payload.pull_request?.number; } else if (context.eventName === 'push') { eventInfo.commitSha = context.payload.after; eventInfo.headRef = context.ref; diff --git a/src/main.ts b/src/main.ts index c8fdfd0..b2fdb81 100644 --- a/src/main.ts +++ b/src/main.ts @@ -1,5 +1,5 @@ import { getChangedFiles } from './changedFiles'; -import { CoverageTypeInfo, DiffInfo, EventInfo } from './types'; +import { CoverageTypeInfo, DiffInfo, EventInfo, FilesStatus } from './types'; import { getEventInfo } from './eventInfo'; import { diffCover } from './diffCover'; import { parseFile as parseLcovFile } from './parsers/lcov'; @@ -30,7 +30,23 @@ export const main = async (): Promise => { : [], junit: eventInfo.showJunit ? await parseJunit(eventInfo.junitPath) : undefined, }; - const changedFile = await getChangedFiles(eventInfo); + let changedFile: FilesStatus = { + all: [], + added: [], + removed: [], + modified: [], + renamed: [], + copied: [], + changed: [], + unchanged: [], + }; + try { + changedFile = await getChangedFiles(eventInfo); + } catch (error) { + core.warning( + `Failed to get changed files: ${error instanceof Error ? error.message : String(error)}`, + ); + } const diffInfo: DiffInfo[] = await diffCover(eventInfo, changedFile, coverageInfo); await commentCoverage(eventInfo, buildBody(eventInfo, coverageInfo.junit, diffInfo)); diff --git a/src/types.d.ts b/src/types.d.ts index a96bef9..0099012 100644 --- a/src/types.d.ts +++ b/src/types.d.ts @@ -29,6 +29,7 @@ export interface EventInfo { commitSha: string; headRef: string; baseRef: string; + prNumber: number | undefined; pwd: string; filesStatus?: FilesStatus; } diff --git a/test/actions.spy.ts b/test/actions.spy.ts index 7b5e4b0..410c87f 100644 --- a/test/actions.spy.ts +++ b/test/actions.spy.ts @@ -69,6 +69,11 @@ export function spyActions(data = defaultData, eventName = 'pull_request') { data: data.compareCommitsWithBasehead, })), }, + pulls: { + listFiles: jest.fn(async () => ({ + data: data.compareCommitsWithBasehead.files || [], + })), + }, }, }) as any, ); diff --git a/test/changedFiles.test.ts b/test/changedFiles.test.ts index 0f66641..1747641 100644 --- a/test/changedFiles.test.ts +++ b/test/changedFiles.test.ts @@ -85,21 +85,17 @@ describe('eventInput tests', () => { filename: `page2-${i}.file`, })); - const mockCompare = jest + const mockListFiles = jest .fn() - .mockResolvedValueOnce({ - data: { total_commits: 1, files: page1Files }, - }) - .mockResolvedValueOnce({ - data: { total_commits: 1, files: page2Files }, - }); + .mockResolvedValueOnce({ data: page1Files }) + .mockResolvedValueOnce({ data: page2Files }); jest.spyOn(github, 'getOctokit').mockImplementation( () => ({ rest: { - repos: { - compareCommitsWithBasehead: mockCompare, + pulls: { + listFiles: mockListFiles, }, }, }) as any, @@ -111,26 +107,23 @@ describe('eventInput tests', () => { expect(filesStatus.all).toHaveLength(60); expect(filesStatus.added).toHaveLength(50); expect(filesStatus.modified).toHaveLength(10); - expect(mockCompare).toHaveBeenCalledTimes(2); + expect(mockListFiles).toHaveBeenCalledTimes(2); }); test('getChangedFiles skips files with unknown status', async () => { - const mockCompare = jest.fn().mockResolvedValueOnce({ - data: { - total_commits: 1, - files: [ - { status: 'added', filename: 'known.file' }, - { status: 'unknown_status', filename: 'unknown.file' }, - ], - }, + const mockListFiles = jest.fn().mockResolvedValueOnce({ + data: [ + { status: 'added', filename: 'known.file' }, + { status: 'unknown_status', filename: 'unknown.file' }, + ], }); jest.spyOn(github, 'getOctokit').mockImplementation( () => ({ rest: { - repos: { - compareCommitsWithBasehead: mockCompare, + pulls: { + listFiles: mockListFiles, }, }, }) as any, @@ -145,16 +138,16 @@ describe('eventInput tests', () => { }); test('getChangedFiles handles empty files response', async () => { - const mockCompare = jest.fn().mockResolvedValueOnce({ - data: { total_commits: 0, files: undefined }, + const mockListFiles = jest.fn().mockResolvedValueOnce({ + data: [], }); jest.spyOn(github, 'getOctokit').mockImplementation( () => ({ rest: { - repos: { - compareCommitsWithBasehead: mockCompare, + pulls: { + listFiles: mockListFiles, }, }, }) as any, From 1bf0bfab739660f2163f3e172ddccbf407884919 Mon Sep 17 00:00:00 2001 From: aGallea Date: Wed, 25 Mar 2026 09:41:19 +0200 Subject: [PATCH 2/2] test: add coverage for compareCommitsWithBasehead fallback and error handling - Test compareCommitsWithBasehead path (push events without prNumber) - Test pagination for compareCommitsWithBasehead - Test undefined files response in compareCommitsWithBasehead - Test empty refs returns empty result - Test getChangedFiles failure in main.ts logs warning and continues Co-Authored-By: Claude Opus 4.6 (1M context) --- test/changedFiles.test.ts | 111 ++++++++++++++++++++++++++++++++++++++ test/main.test.ts | 30 +++++++++++ 2 files changed, 141 insertions(+) diff --git a/test/changedFiles.test.ts b/test/changedFiles.test.ts index 1747641..5d27c52 100644 --- a/test/changedFiles.test.ts +++ b/test/changedFiles.test.ts @@ -137,6 +137,117 @@ describe('eventInput tests', () => { expect(filesStatus.added[0]).toEqual('known.file'); }); + test('getChangedFiles falls back to compareCommitsWithBasehead for push events', async () => { + const mockCompare = jest.fn().mockResolvedValueOnce({ + data: { + total_commits: 1, + files: [ + { status: 'added', filename: 'push.file' }, + { status: 'modified', filename: 'push2.file' }, + ], + }, + }); + + jest.spyOn(github, 'getOctokit').mockImplementation( + () => + ({ + rest: { + repos: { + compareCommitsWithBasehead: mockCompare, + }, + }, + }) as any, + ); + + const eventInfo: EventInfo = getEventInfo(); + // Simulate push event: no prNumber but baseRef/headRef set + eventInfo.prNumber = undefined; + eventInfo.baseRef = 'main'; + eventInfo.headRef = 'some-branch'; + const filesStatus: FilesStatus = await getChangedFiles(eventInfo); + + expect(filesStatus.all).toHaveLength(2); + expect(filesStatus.added).toHaveLength(1); + expect(filesStatus.added[0]).toEqual('push.file'); + expect(filesStatus.modified).toHaveLength(1); + expect(mockCompare).toHaveBeenCalledWith( + expect.objectContaining({ + basehead: 'main...some-branch', + }), + ); + }); + + test('getChangedFiles compareCommitsWithBasehead paginates correctly', async () => { + const page1Files = Array.from({ length: 50 }, (_, i) => ({ + status: 'added', + filename: `cmp-page1-${i}.file`, + })); + const page2Files = Array.from({ length: 5 }, (_, i) => ({ + status: 'modified', + filename: `cmp-page2-${i}.file`, + })); + + const mockCompare = jest + .fn() + .mockResolvedValueOnce({ data: { total_commits: 1, files: page1Files } }) + .mockResolvedValueOnce({ data: { total_commits: 1, files: page2Files } }); + + jest.spyOn(github, 'getOctokit').mockImplementation( + () => + ({ + rest: { + repos: { + compareCommitsWithBasehead: mockCompare, + }, + }, + }) as any, + ); + + const eventInfo: EventInfo = getEventInfo(); + eventInfo.prNumber = undefined; + eventInfo.baseRef = 'main'; + eventInfo.headRef = 'feature'; + const filesStatus: FilesStatus = await getChangedFiles(eventInfo); + + expect(filesStatus.all).toHaveLength(55); + expect(mockCompare).toHaveBeenCalledTimes(2); + }); + + test('getChangedFiles compareCommitsWithBasehead handles undefined files', async () => { + const mockCompare = jest.fn().mockResolvedValueOnce({ + data: { total_commits: 0, files: undefined }, + }); + + jest.spyOn(github, 'getOctokit').mockImplementation( + () => + ({ + rest: { + repos: { + compareCommitsWithBasehead: mockCompare, + }, + }, + }) as any, + ); + + const eventInfo: EventInfo = getEventInfo(); + eventInfo.prNumber = undefined; + eventInfo.baseRef = 'main'; + eventInfo.headRef = 'feature'; + const filesStatus: FilesStatus = await getChangedFiles(eventInfo); + + expect(filesStatus.all).toHaveLength(0); + }); + + test('getChangedFiles returns empty when no prNumber and no refs', async () => { + const eventInfo: EventInfo = getEventInfo(); + eventInfo.prNumber = undefined; + eventInfo.baseRef = ''; + eventInfo.headRef = ''; + const filesStatus: FilesStatus = await getChangedFiles(eventInfo); + + expect(filesStatus.all).toHaveLength(0); + }); + test('getChangedFiles handles empty files response', async () => { const mockListFiles = jest.fn().mockResolvedValueOnce({ data: [], diff --git a/test/main.test.ts b/test/main.test.ts index 12652e9..4d8616a 100644 --- a/test/main.test.ts +++ b/test/main.test.ts @@ -33,6 +33,36 @@ describe('main tests', () => { '\n## Tests Report Mock :page_facing_up:\n', ); }); + test('getChangedFiles failure logs warning and continues', async () => { + // Mock getChangedFiles to throw + jest.spyOn(github, 'getOctokit').mockImplementation( + () => + ({ + rest: { + pulls: { + listFiles: jest.fn().mockRejectedValue(new Error('API rate limit')), + }, + }, + }) as any, + ); + + const commentCoverageSpy = jest + .spyOn(Comment, 'commentCoverage') + .mockImplementation(async (): Promise => { + return; + }); + const warningSpy = jest.spyOn(core, 'warning'); + + await main(); + + // Should warn but not fail + expect(warningSpy).toHaveBeenCalledWith( + expect.stringContaining('Failed to get changed files'), + ); + // Should still post the coverage comment + expect(commentCoverageSpy).toHaveBeenCalled(); + }); + test('exception', async () => { jest .spyOn(Comment, 'commentCoverage')