Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix #128 : On push, use files in commit to avoid "not found" error #130

Closed
wants to merge 7 commits into from
Closed

Fix #128 : On push, use files in commit to avoid "not found" error #130

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
2094902
List files for commit instead of compare
douglascayers Sep 22, 2022
8db1d24
Use TEST_GITHUB_TOKEN env var to avoid committing it in file
douglascayers Sep 22, 2022
cac0fd8
Update nock paths for tests
douglascayers Sep 22, 2022
a7b9d02
Rebuild index.js via npm run all
douglascayers Sep 22, 2022
fedaecc
Merge remote-tracking branch 'origin/main' into issue128/on-push-use-…
douglascayers Sep 26, 2022
2827931
revert util.test.js
douglascayers Sep 26, 2022
0f51caf
revert util.js
douglascayers Sep 26, 2022
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
2 changes: 1 addition & 1 deletion dist/index.js
View file
Open in desktop

Large diffs are not rendered by default.

8 changes: 4 additions & 4 deletions lib/util.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ const path = require('path');

// Load at most MAX_PAGE pages when determining modified files.
// This is used both for pull/{pull_number}/files as well as for
// repos/compareCommits API calls.
// repos/{owner}/{repo}/commits/{commit} API calls.
const MAX_PAGE = 10;

const downloadPmd = async function(version, token) {
Expand Down Expand Up @@ -140,13 +140,13 @@ const determineModifiedFiles = async function(token, sourcePath) {
// maximum of 300 files are loaded (page size is 30, max 10 pages)
let page;
for(page = 1; page <= MAX_PAGE; page++) {
const compareResponse = await octokit.rest.repos.compareCommitsWithBasehead({
const commitResponse = await octokit.rest.repos.getCommit({
...context.repo,
basehead: `${eventData.before}...${eventData.after}`,
ref: eventData.after,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change will now only consider the head commit of the commits that have been pushed. If multiple commits have been pushed at once, then the file list won't be complete.

The push event is described here: https://docs.github.com/en/developers/webhooks-and-events/webhooks/webhook-events-and-payloads#push
The sample payload seems to be for a tag, that has been pushed - interestingly it also has before=00000.... That means, the same problem (#128) should occur for if a tag has been pushed.

The payload also contains an array commits[] - in the example it's empty. I guess this is because it's a tag, that has been pushed. But for usual pushes, that should contain all the commits that are contained in the push. If there is no previous commit (before=0000....) we maybe need to loop through the commits and request each commit with your suggested method (getCommit).

Also there are other interesting flags in the push event: e.g. if a branch/tag is deleted, we would also get a push event and likely fail to determine the modified files.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the clarification on the push event payload. I misunderstood and thought the before was simply the immediate prior commit to what after represented. But it seems GitHub webhooks may be trying to be efficient by collapsing multiple commits into a single payload.

I'll reconsider my approach for handling the specific instance when before=0000.... per your recommendations.

per_page: 30,
page: page
});
const allFiles = compareResponse.data.files;
const allFiles = commitResponse.data.files;
if (allFiles === undefined || allFiles.length == 0) {
break;
}
Expand Down
57 changes: 32 additions & 25 deletions tests/util.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,13 @@ const tempPath = path.join(__dirname, 'TEMP')
process.env['RUNNER_TEMP'] = tempPath
process.env['RUNNER_TOOL_CACHE'] = cachePath

// Personal Access Token to perform tests with.
// https://github.com/settings/tokens/new
const token = process.env['TEST_GITHUB_TOKEN'];
if (!token) {
throw new Error('Set `export TEST_GITHUB_TOKEN="<your token>"` to run tests');
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change will break the CI build - as there is no env var "TEST_GITHUB_TOKEN" defined.
For the unit tests, no real token is required (hence I used my_test_token), because all the requests to the github api are mocked using nock. Since you added a new API call, we likely just need to mock this as well.

It seems, you updated the mock - so I guess this change here is not needed anymore.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, understood. I'll revert this change. Thanks

describe('pmd-github-action-util', function () {
let platformMock;
let execMock;
Expand Down Expand Up @@ -56,7 +63,7 @@ describe('pmd-github-action-util', function () {
.get('/pmd/pmd/releases/download/pmd_releases/6.40.0/pmd-bin-6.40.0.zip')
.replyWithFile(200, __dirname + '/data/pmd-bin-6.40.0.zip')

const pmdInfo = await util.downloadPmd('latest', 'my_test_token');
const pmdInfo = await util.downloadPmd('latest', token);

const toolCache = path.join(cachePath, 'pmd', '6.40.0', os.arch(), 'pmd-bin-6.40.0');
expect(pmdInfo).toStrictEqual({ path: toolCache, version: '6.40.0' });
Expand All @@ -72,7 +79,7 @@ describe('pmd-github-action-util', function () {
nock('https://github.com')
.get('/pmd/pmd/releases/download/pmd_releases/6.39.0/pmd-bin-6.39.0.zip')
.replyWithFile(200, __dirname + '/data/pmd-bin-6.39.0.zip');
const pmdInfo = await util.downloadPmd('6.39.0', 'my_test_token');
const pmdInfo = await util.downloadPmd('6.39.0', token);

const toolCache = path.join(cachePath, 'pmd', '6.39.0', os.arch(), 'pmd-bin-6.39.0');
expect(pmdInfo).toStrictEqual({ path: toolCache, version: '6.39.0' });
Expand All @@ -90,8 +97,8 @@ describe('pmd-github-action-util', function () {
.get('/pmd/pmd/releases/download/pmd_releases/6.39.0/pmd-bin-6.39.0.zip')
.once()
.replyWithFile(200, __dirname + '/data/pmd-bin-6.39.0.zip');
const pmdInfo = await util.downloadPmd('6.39.0', 'my_test_token');
const pmdInfo2 = await util.downloadPmd('6.39.0', 'my_test_token');
const pmdInfo = await util.downloadPmd('6.39.0', token);
const pmdInfo2 = await util.downloadPmd('6.39.0', token);

const toolCache = path.join(cachePath, 'pmd', '6.39.0', os.arch(), 'pmd-bin-6.39.0');
expect(pmdInfo).toStrictEqual({ path: toolCache, version: '6.39.0' });
Expand All @@ -109,7 +116,7 @@ describe('pmd-github-action-util', function () {
.get('/pmd/pmd/releases/download/pmd_releases/6.40.0/pmd-bin-6.40.0.zip')
.replyWithFile(200, __dirname + '/data/pmd-bin-6.40.0.zip')

const pmdInfo = await util.downloadPmd('latest', 'my_test_token');
const pmdInfo = await util.downloadPmd('latest', token);
const execOutput = await util.executePmd(pmdInfo, '.', 'ruleset.xml', 'sarif', 'pmd-report.sarif');
const reportFile = path.join('.', 'pmd-report.sarif');
await expect(fs.access(reportFile)).resolves.toBe(undefined);
Expand All @@ -130,7 +137,7 @@ describe('pmd-github-action-util', function () {
.get('/pmd/pmd/releases/download/pmd_releases/6.41.0/pmd-bin-6.41.0.zip')
.replyWithFile(200, __dirname + '/data/pmd-bin-6.41.0.zip')

const pmdInfo = await util.downloadPmd('6.41.0', 'my_test_token');
const pmdInfo = await util.downloadPmd('6.41.0', token);
const execOutput = await util.executePmd(pmdInfo, '.', 'ruleset.xml', 'sarif', 'pmd-report.sarif');
const reportFile = path.join('.', 'pmd-report.sarif');
await expect(fs.access(reportFile)).resolves.toBe(undefined);
Expand All @@ -146,7 +153,7 @@ describe('pmd-github-action-util', function () {
.get('/repos/pmd/pmd/releases/latest')
.reply(503, 'Test Internal Server Error');

expect(() => util.downloadPmd('latest', 'my_test_token')).rejects.toThrow();
expect(() => util.downloadPmd('latest', token)).rejects.toThrow();
})

it('failure while executing PMD', async () => {
Expand All @@ -165,7 +172,7 @@ describe('pmd-github-action-util', function () {
.get('/pmd/pmd/releases/download/pmd_releases/6.40.0/pmd-bin-6.40.0.zip')
.replyWithFile(200, __dirname + '/data/pmd-bin-6.40.0.zip')

const pmdInfo = await util.downloadPmd('latest', 'my_test_token');
const pmdInfo = await util.downloadPmd('latest', token);
await util.executePmd(pmdInfo, '.', 'ruleset.xml', 'sarif', 'pmd-report.sarif');

expect(execMock).toBeCalledWith(`${pmdInfo.path}\\bin\\pmd.bat`, [
Expand Down Expand Up @@ -194,7 +201,7 @@ describe('pmd-github-action-util', function () {
nock('https://api.github.com')
.get('/repos/pmd/pmd-github-action-tests/pulls/1/files?per_page=30&page=2')
.reply(200, []);
let fileList = await util.determineModifiedFiles('my_test_token', path.normalize('src/main/java'));
let fileList = await util.determineModifiedFiles(token, path.normalize('src/main/java'));
expect(fileList).toStrictEqual(['src/main/java/AvoidCatchingThrowableSample.java', 'src/main/java/NewFile.java', 'src/main/java/ChangedFile.java']
.map(f => path.normalize(f)));
})
Expand All @@ -213,7 +220,7 @@ describe('pmd-github-action-util', function () {
nock('https://api.github.com')
.get('/repos/pmd/pmd-github-action-tests/pulls/1/files?per_page=30&page=2')
.reply(200, []);
let fileList = await util.determineModifiedFiles('my_test_token', path.normalize('src/main/java'));
let fileList = await util.determineModifiedFiles(token, path.normalize('src/main/java'));
expect(fileList).toStrictEqual(['src/main/java/AvoidCatchingThrowableSample.java', 'src/main/java/NewFile.java', 'src/main/java/ChangedFile.java']
.map(f => path.normalize(f)));
})
Expand All @@ -231,15 +238,15 @@ describe('pmd-github-action-util', function () {
'Content-Type': 'application/json',
});
}
let fileList = await util.determineModifiedFiles('my_test_token', path.normalize('src/main/java'));
let fileList = await util.determineModifiedFiles(token, path.normalize('src/main/java'));
expect(fileList).toStrictEqual(['src/main/java/AvoidCatchingThrowableSample.java', 'src/main/java/NewFile.java', 'src/main/java/ChangedFile.java']
.map(f => path.normalize(f)));
})

test('return undefined for unsupported event', async () => {
process.env['GITHUB_REPOSITORY'] = 'pmd/pmd-github-action-tests'
process.env['GITHUB_EVENT_NAME'] = 'workflow_dispatch';
let result = await util.determineModifiedFiles('my_test_token', path.normalize('src/main/java'));
let result = await util.determineModifiedFiles(token, path.normalize('src/main/java'));
expect(result).toBe(undefined);
})

Expand All @@ -253,7 +260,7 @@ describe('pmd-github-action-util', function () {
.get('/pmd/pmd/releases/download/pmd_releases/6.40.0/pmd-bin-6.40.0.zip')
.replyWithFile(200, __dirname + '/data/pmd-bin-6.40.0.zip')

const pmdInfo = await util.downloadPmd('latest', 'my_test_token');
const pmdInfo = await util.downloadPmd('latest', token);
const execOutput = await util.executePmd(pmdInfo, ['src/file1.txt', 'src/file2.txt'], 'ruleset.xml', 'sarif', 'pmd-report.sarif');
const pmdFilelist = path.join('.', 'pmd.filelist');
await expect(fs.access(pmdFilelist)).resolves.toBe(undefined);
Expand All @@ -275,7 +282,7 @@ describe('pmd-github-action-util', function () {
.get('/pmd/pmd/releases/download/pmd_releases/6.41.0/pmd-bin-6.41.0.zip')
.replyWithFile(200, __dirname + '/data/pmd-bin-6.41.0.zip')

const pmdInfo = await util.downloadPmd('6.41.0', 'my_test_token');
const pmdInfo = await util.downloadPmd('6.41.0', token);
const execOutput = await util.executePmd(pmdInfo, ['src/file1.txt', 'src/file2.txt'], 'ruleset.xml', 'sarif', 'pmd-report.sarif');
const pmdFilelist = path.join('.', 'pmd.filelist');
await expect(fs.access(pmdFilelist)).resolves.toBe(undefined);
Expand All @@ -294,16 +301,16 @@ describe('pmd-github-action-util', function () {
process.env['GITHUB_EVENT_NAME'] = 'push';
process.env['GITHUB_EVENT_PATH'] = __dirname + '/data/push-event-data.json';
nock('https://api.github.com')
.get('/repos/pmd/pmd-github-action-tests/compare/44c1557134c7dbaf46ecdf796fb871c8df8989e4...8a7a25638d8ca5207cc824dea9571325b243c6a1?per_page=30&page=1')
.get('/repos/pmd/pmd-github-action-tests/commits/8a7a25638d8ca5207cc824dea9571325b243c6a1?per_page=30&page=1')
.replyWithFile(200, __dirname + '/data/compare-files-page1.json', {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please also rename this file "compare-files-page1.json" and update the contents - it's now not anymore the response from compare, but from commits, which potentially looks different.

You can just look at https://api.github.com/repos/pmd/pmd-github-action-tests/commits/8a7a25638d8ca5207cc824dea9571325b243c6a1?per_page=30&page=1 to get the json.

Note: I've simplified the original response for compare back then to make it smaller and contain only the essential parts - and modified the files part to contain the data I needed for tests.

'Content-Type': 'application/json',
});
nock('https://api.github.com')
.get('/repos/pmd/pmd-github-action-tests/compare/44c1557134c7dbaf46ecdf796fb871c8df8989e4...8a7a25638d8ca5207cc824dea9571325b243c6a1?per_page=30&page=2')
.get('/repos/pmd/pmd-github-action-tests/commits/8a7a25638d8ca5207cc824dea9571325b243c6a1?per_page=30&page=2')
.replyWithFile(200, __dirname + '/data/compare-files-page2.json', {
'Content-Type': 'application/json',
});
let fileList = await util.determineModifiedFiles('my_test_token', path.normalize('src/main/java'));
let fileList = await util.determineModifiedFiles(token, path.normalize('src/main/java'));
expect(fileList).toStrictEqual(['src/main/java/AvoidCatchingThrowableSample.java', 'src/main/java/NewFile.java', 'src/main/java/ChangedFile.java']
.map(f => path.normalize(f)));
})
Expand All @@ -316,12 +323,12 @@ describe('pmd-github-action-util', function () {
process.env['GITHUB_EVENT_PATH'] = __dirname + '/data/push-event-data.json';
for (let page = 1; page <= 10; page++) {
nock('https://api.github.com')
.get(`/repos/pmd/pmd-github-action-tests/compare/44c1557134c7dbaf46ecdf796fb871c8df8989e4...8a7a25638d8ca5207cc824dea9571325b243c6a1?per_page=30&page=${page}`)
.get(`/repos/pmd/pmd-github-action-tests/commits/8a7a25638d8ca5207cc824dea9571325b243c6a1?per_page=30&page=${page}`)
.replyWithFile(200, __dirname + '/data/compare-files-page1.json', {
'Content-Type': 'application/json',
});
}
let fileList = await util.determineModifiedFiles('my_test_token', path.normalize('src/main/java'));
let fileList = await util.determineModifiedFiles(token, path.normalize('src/main/java'));
expect(fileList).toStrictEqual(['src/main/java/AvoidCatchingThrowableSample.java', 'src/main/java/NewFile.java', 'src/main/java/ChangedFile.java']
.map(f => path.normalize(f)));
})
Expand All @@ -334,12 +341,12 @@ describe('pmd-github-action-util', function () {
process.env['GITHUB_EVENT_PATH'] = __dirname + '/data/push-event-data.json';
for (let page = 1; page <= 10; page++) {
nock('https://api.github.com')
.get(`/repos/pmd/pmd-github-action-tests/compare/44c1557134c7dbaf46ecdf796fb871c8df8989e4...8a7a25638d8ca5207cc824dea9571325b243c6a1?per_page=30&page=${page}`)
.get(`/repos/pmd/pmd-github-action-tests/commits/8a7a25638d8ca5207cc824dea9571325b243c6a1?per_page=30&page=${page}`)
.replyWithFile(200, __dirname + '/data/compare-files-page1-issue52.json', {
'Content-Type': 'application/json',
});
}
let fileList = await util.determineModifiedFiles('my_test_token', path.normalize('src/main/java'));
let fileList = await util.determineModifiedFiles(token, path.normalize('src/main/java'));
expect(fileList).toStrictEqual(['src/main/java/AvoidCatchingThrowableSample.java', 'src/main/java/ChangedFile.java']
.map(f => path.normalize(f)));
})
Expand All @@ -352,12 +359,12 @@ describe('pmd-github-action-util', function () {
process.env['GITHUB_EVENT_PATH'] = __dirname + '/data/push-event-data.json';
for (let page = 1; page <= 10; page++) {
nock('https://api.github.com')
.get(`/repos/pmd/pmd-github-action-tests/compare/44c1557134c7dbaf46ecdf796fb871c8df8989e4...8a7a25638d8ca5207cc824dea9571325b243c6a1?per_page=30&page=${page}`)
.get(`/repos/pmd/pmd-github-action-tests/commits/8a7a25638d8ca5207cc824dea9571325b243c6a1?per_page=30&page=${page}`)
.replyWithFile(200, __dirname + '/data/compare-files-page1-issue52.json', {
'Content-Type': 'application/json',
});
}
let fileList = await util.determineModifiedFiles('my_test_token', path.normalize('src/main/java/'));
let fileList = await util.determineModifiedFiles(token, path.normalize('src/main/java/'));
expect(fileList).toStrictEqual(['src/main/java/AvoidCatchingThrowableSample.java', 'src/main/java/ChangedFile.java']
.map(f => path.normalize(f)));
})
Expand All @@ -370,12 +377,12 @@ describe('pmd-github-action-util', function () {
process.env['GITHUB_EVENT_PATH'] = __dirname + '/data/push-event-data.json';
for (let page = 1; page <= 10; page++) {
nock('https://api.github.com')
.get(`/repos/pmd/pmd-github-action-tests/compare/44c1557134c7dbaf46ecdf796fb871c8df8989e4...8a7a25638d8ca5207cc824dea9571325b243c6a1?per_page=30&page=${page}`)
.get(`/repos/pmd/pmd-github-action-tests/commits/8a7a25638d8ca5207cc824dea9571325b243c6a1?per_page=30&page=${page}`)
.replyWithFile(200, __dirname + '/data/compare-files-page1-issue52.json', {
'Content-Type': 'application/json',
});
}
let fileList = await util.determineModifiedFiles('my_test_token', path.normalize('.'));
let fileList = await util.determineModifiedFiles(token, path.normalize('.'));
expect(fileList).toStrictEqual(['src/main/java/AvoidCatchingThrowableSample.java', 'src/main/java2/NewFile.java', 'src/main/java/ChangedFile.java', 'README.md']
.map(f => path.normalize(f)));
})
Expand Down