Skip to content

Commit

Permalink
bundle-size: Use mergeSha when showing code changes
Browse files Browse the repository at this point in the history
  • Loading branch information
jridgewell committed Mar 10, 2021
1 parent b1e6b29 commit 29bcf5a
Show file tree
Hide file tree
Showing 2 changed files with 114 additions and 4 deletions.
36 changes: 32 additions & 4 deletions bundle-size/api.js
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,7 @@ function erroredCheckOutput(partialBaseSha) {
*
* @param {string} headSha commit being checked
* @param {string} baseSha baseline commit for comparison
* @param {string} mergeSha merge commit combining the head and base
* @param {!Array<string>} bundleSizeDeltas text description of all bundle size
* changes.
* @param {!Array<string>} missingBundleSizes text description of missing bundle
Expand All @@ -110,15 +111,19 @@ function erroredCheckOutput(partialBaseSha) {
function extraBundleSizesSummary(
headSha,
baseSha,
mergeSha,
bundleSizeDeltasRequireApproval,
bundleSizeDeltasAutoApproved,
missingBundleSizes
) {
const compareUrl =
mergeSha ||
`https://github.com/ampproject/amphtml/compare/${baseSha}..${headSha}`;
let output =
'## Commit details\n' +
`**Head commit:** ${headSha}\n` +
`**Base commit:** ${baseSha}\n` +
`**Code changes:** https://github.com/ampproject/amphtml/compare/${baseSha}..${headSha}\n`;
`**Code changes:** ${compareUrl}\n`;

if (bundleSizeDeltasRequireApproval.length) {
output +=
Expand Down Expand Up @@ -248,14 +253,22 @@ exports.installApiRouter = (app, router, db, githubUtils) => {
*
* @param {!object} check GitHub Check database object.
* @param {string} baseSha commit SHA of the base commit being compared to.
* @param {string} mergeSha commit SHA of the merge commit that combines the head and base.
* @param {!Map<string, number>} prBundleSizes the bundle sizes of various
* dist files in the pull request in KB.
* @param {boolean} lastAttempt true if this is the last retry.
* @return {boolean} true if succeeded; false otherwise.
*/
async function tryReport(check, baseSha, prBundleSizes, lastAttempt = false) {
async function tryReport(
check,
baseSha,
mergeSha,
prBundleSizes,
lastAttempt = false
) {
const partialHeadSha = check.head_sha.substr(0, 7);
const partialBaseSha = baseSha.substr(0, 7);
const partialMergeSha = mergeSha.substr(0, 7);
const github = await app.auth(check.installation_id);
const githubOptions = {
owner: check.owner,
Expand Down Expand Up @@ -375,6 +388,7 @@ exports.installApiRouter = (app, router, db, githubUtils) => {
const reportMarkdown = extraBundleSizesSummary(
partialHeadSha,
partialBaseSha,
partialMergeSha,
bundleSizeDeltasRequireApproval,
bundleSizeDeltasAutoApproved,
missingBundleSizes
Expand Down Expand Up @@ -454,13 +468,21 @@ exports.installApiRouter = (app, router, db, githubUtils) => {

router.post('/commit/:headSha/report', async (request, response) => {
const {headSha} = request.params;
const {baseSha, bundleSizes} = request.body;
// mergeSha is new, and not all reports will have it.
const {baseSha, mergeSha = '', bundleSizes} = request.body;

if (typeof baseSha !== 'string' || !/^[0-9a-f]{40}$/.test(baseSha)) {
return response
.status(400)
.end('POST request to /report must have commit SHA field "baseSha"');
}
if (typeof mergeSha !== 'string' || !/^[0-9a-f]{40}$|^$/.test(mergeSha)) {
return response
.status(400)
.end(
'POST request to /report with "mergeSha" field must be a valid commit SHA'
);
}
if (
!bundleSizes ||
Object.values(bundleSizes).some(value => typeof value !== 'number') ||
Expand All @@ -486,7 +508,12 @@ exports.installApiRouter = (app, router, db, githubUtils) => {
}

try {
let reportSuccess = await tryReport(check, baseSha, bundleSizes);
let reportSuccess = await tryReport(
check,
baseSha,
mergeSha,
bundleSizes
);
if (reportSuccess) {
response.end();
} else {
Expand All @@ -501,6 +528,7 @@ exports.installApiRouter = (app, router, db, githubUtils) => {
reportSuccess = await tryReport(
check,
baseSha,
mergeSha,
bundleSizes,
/* lastAttempt */ retriesLeft == 0
);
Expand Down
82 changes: 82 additions & 0 deletions bundle-size/test/api.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -651,6 +651,88 @@ describe('bundle-size api', () => {
);
});

test('update check on bundle-size report with mergeSha (report/_delayed_-base = 12.34KB/12.23KB)', async () => {
await db('checks').insert({
head_sha: '26ddec3fbbd3c7bd94e05a701c8b8c3ea8826faa',
owner: 'ampproject',
repo: 'amphtml',
pull_request_id: 19603,
installation_id: 123456,
check_run_id: 555555,
approving_teams: null,
report_markdown: null,
});

baseBundleSizeFixture.content = Buffer.from(
'{"dist/v0.js":12.23}'
).toString('base64');

let notFoundCount = 2;
github.repos.getContent.mockImplementation(params => {
if (
params.path.endsWith('5f27002526a808c5c1ad5d0f1ab1cec471af0a33.json')
) {
if (notFoundCount > 0) {
notFoundCount--;
return Promise.reject({status: 404});
}
return Promise.resolve({data: baseBundleSizeFixture});
}
const fixture = params.path.replace(/^.+\/(.+)$/, '$1');
return Promise.resolve({data: getFixture(fixture)});
});

await request(probot.server)
.post('/v0/commit/26ddec3fbbd3c7bd94e05a701c8b8c3ea8826faa/report')
.send({
...jsonPayload,
mergeSha: '9f6fd877fc27c679ad318699ac25a3fb4e228fca',
})
.set('Content-Type', 'application/json')
.set('Accept', 'application/json')
.expect(202);

await expect(
db('checks')
.where({
head_sha: '26ddec3fbbd3c7bd94e05a701c8b8c3ea8826faa',
})
.first()
).resolves.toMatchObject({
head_sha: '26ddec3fbbd3c7bd94e05a701c8b8c3ea8826faa',
owner: 'ampproject',
repo: 'amphtml',
pull_request_id: 19603,
installation_id: 123456,
check_run_id: 555555,
approving_teams: 'ampproject/wg-performance,ampproject/wg-runtime',
report_markdown:
'## Commit details\n' +
'**Head commit:** 26ddec3\n' +
'**Base commit:** 5f27002\n' +
'**Code changes:** 9f6fd87\n\n' +
'## Bundle size changes that require approval\n' +
'* `dist/v0.js`: Δ +0.11KB\n' +
'## Bundle sizes missing from this PR\n' +
'* `dist/amp4ads-v0.js`: (11.22 KB) missing in `master`\n' +
'* `dist/v0/amp-accordion-0.1.js`: (1.11 KB) missing in `master`\n' +
'* `dist/v0/amp-ad-0.1.js`: (4.56 KB) missing in `master`',
});

expect(github.pulls.listRequestedReviewers).toHaveBeenCalled();
expect(github.pulls.listReviews).toHaveBeenCalled();
expect(github.pulls.requestReviewers).toHaveBeenCalled();
expect(github.checks.update).toHaveBeenCalledWith(
expect.objectContaining({
conclusion: 'action_required',
output: expect.objectContaining({
title:
'Approval required from one of [@ampproject/wg-performance, @ampproject/wg-runtime]',
}),
})
);
});

test('update check on bundle-size report on missing base size', async () => {
await db('checks').insert({
head_sha: '26ddec3fbbd3c7bd94e05a701c8b8c3ea8826faa',
Expand Down

0 comments on commit 29bcf5a

Please sign in to comment.