From 1a07cd511b1a415544104135ba7898943b0a3dba Mon Sep 17 00:00:00 2001 From: Jed Fox Date: Sat, 9 Apr 2022 17:28:54 -0400 Subject: [PATCH 1/7] Remove outdated attribution from perf-tester comments --- ci/perf-tester/src/index.js | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/ci/perf-tester/src/index.js b/ci/perf-tester/src/index.js index f9495303d..56027ca5e 100644 --- a/ci/perf-tester/src/index.js +++ b/ci/perf-tester/src/index.js @@ -133,9 +133,7 @@ async function run(octokit, context, token) { const comment = { ...commentInfo, - body: - markdownDiff + - '\n\nperformance-action', + body: markdownDiff, }; if (toBool(getInput("use-check"))) { From 19a1de17ecdca33a281b6123102db8fcc9be6f23 Mon Sep 17 00:00:00 2001 From: Jed Fox Date: Sat, 9 Apr 2022 17:29:26 -0400 Subject: [PATCH 2/7] try out the check --- ci/perf-tester/src/index.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/ci/perf-tester/src/index.js b/ci/perf-tester/src/index.js index 56027ca5e..110e474b4 100644 --- a/ci/perf-tester/src/index.js +++ b/ci/perf-tester/src/index.js @@ -136,7 +136,7 @@ async function run(octokit, context, token) { body: markdownDiff, }; - if (toBool(getInput("use-check"))) { + // if (toBool(getInput("use-check"))) { if (token) { const finish = await createCheck(octokit, context); await finish({ @@ -149,7 +149,7 @@ async function run(octokit, context, token) { } else { outputRawMarkdown = true; } - } else { + // } else { startGroup(`Updating stats PR comment`); let commentId; try { @@ -205,7 +205,7 @@ async function run(octokit, context, token) { outputRawMarkdown = true; } } - } + // } endGroup(); } From 90b9503bd023c48da5adbb651a1fb585cc7d1eca Mon Sep 17 00:00:00 2001 From: Jed Fox Date: Sat, 9 Apr 2022 17:37:49 -0400 Subject: [PATCH 3/7] Remove support for posting checks from perf-tester --- ci/perf-tester/src/index.js | 127 +++++++++++++----------------------- ci/perf-tester/src/utils.js | 6 -- 2 files changed, 45 insertions(+), 88 deletions(-) diff --git a/ci/perf-tester/src/index.js b/ci/perf-tester/src/index.js index 110e474b4..b1de154e7 100644 --- a/ci/perf-tester/src/index.js +++ b/ci/perf-tester/src/index.js @@ -29,7 +29,6 @@ const { averageBenchmarks, toDiff, diffTable, - toBool, } = require("./utils.js"); const benchmarkParallel = 2; @@ -44,7 +43,7 @@ const runBenchmarks = async () => { return averageBenchmarks(results); }; -async function run(octokit, context, token) { +async function run(octokit, context) { const { number: pull_number } = context.issue; const pr = context.payload.pull_request; @@ -136,76 +135,60 @@ async function run(octokit, context, token) { body: markdownDiff, }; - // if (toBool(getInput("use-check"))) { - if (token) { - const finish = await createCheck(octokit, context); - await finish({ - conclusion: "success", - output: { - title: `Compressed Size Action`, - summary: markdownDiff, - }, - }); - } else { - outputRawMarkdown = true; + startGroup(`Updating stats PR comment`); + let commentId; + try { + const comments = (await octokit.issues.listComments(commentInfo)).data; + for (let i = comments.length; i--; ) { + const c = comments[i]; + if ( + c.user.type === "Bot" && + /[\s\n]*performance-action/.test(c.body) + ) { + commentId = c.id; + break; + } } - // } else { - startGroup(`Updating stats PR comment`); - let commentId; + } catch (e) { + console.log("Error checking for previous comments: " + e.message); + } + + if (commentId) { + console.log(`Updating previous comment #${commentId}`); try { - const comments = (await octokit.issues.listComments(commentInfo)) - .data; - for (let i = comments.length; i--; ) { - const c = comments[i]; - if ( - c.user.type === "Bot" && - /[\s\n]*performance-action/.test(c.body) - ) { - commentId = c.id; - break; - } - } + await octokit.issues.updateComment({ + ...context.repo, + comment_id: commentId, + body: comment.body, + }); } catch (e) { - console.log("Error checking for previous comments: " + e.message); + console.log("Error editing previous comment: " + e.message); + commentId = null; } + } - if (commentId) { - console.log(`Updating previous comment #${commentId}`); + // no previous or edit failed + if (!commentId) { + console.log("Creating new comment"); + try { + await octokit.issues.createComment(comment); + } catch (e) { + console.log(`Error creating comment: ${e.message}`); + console.log(`Submitting a PR review comment instead...`); try { - await octokit.issues.updateComment({ - ...context.repo, - comment_id: commentId, + const issue = context.issue || pr; + await octokit.pulls.createReview({ + owner: issue.owner, + repo: issue.repo, + pull_number: issue.number, + event: "COMMENT", body: comment.body, }); } catch (e) { - console.log("Error editing previous comment: " + e.message); - commentId = null; + console.log("Error creating PR review."); + outputRawMarkdown = true; } } - - // no previous or edit failed - if (!commentId) { - console.log("Creating new comment"); - try { - await octokit.issues.createComment(comment); - } catch (e) { - console.log(`Error creating comment: ${e.message}`); - console.log(`Submitting a PR review comment instead...`); - try { - const issue = context.issue || pr; - await octokit.pulls.createReview({ - owner: issue.owner, - repo: issue.repo, - pull_number: issue.number, - event: "COMMENT", - body: comment.body, - }); - } catch (e) { - console.log("Error creating PR review."); - outputRawMarkdown = true; - } - } - // } endGroup(); } @@ -223,31 +206,11 @@ async function run(octokit, context, token) { console.log("All done!"); } -// create a check and return a function that updates (completes) it -async function createCheck(octokit, context) { - const check = await octokit.checks.create({ - ...context.repo, - name: "Compressed Size", - head_sha: context.payload.pull_request.head.sha, - status: "in_progress", - }); - - return async (details) => { - await octokit.checks.update({ - ...context.repo, - check_run_id: check.data.id, - completed_at: new Date().toISOString(), - status: "completed", - ...details, - }); - }; -} - (async () => { try { const token = getInput("repo-token", { required: true }); const octokit = new GitHub(token); - await run(octokit, context, token); + await run(octokit, context); } catch (e) { setFailed(e.message); } diff --git a/ci/perf-tester/src/utils.js b/ci/perf-tester/src/utils.js index 49fce603e..092c2e9dc 100644 --- a/ci/perf-tester/src/utils.js +++ b/ci/perf-tester/src/utils.js @@ -213,9 +213,3 @@ exports.diffTable = ( return out; }; - -/** - * Convert a string "true"/"yes"/"1" argument value to a boolean - * @param {string} v - */ -exports.toBool = (v) => /^(1|true|yes)$/.test(v); From e636bd1451b2a19c4b3fbb5cf4e18e2756b4ed36 Mon Sep 17 00:00:00 2001 From: Jed Fox Date: Sat, 9 Apr 2022 17:39:42 -0400 Subject: [PATCH 4/7] Fix finding the comment to update --- ci/perf-tester/src/index.js | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/ci/perf-tester/src/index.js b/ci/perf-tester/src/index.js index b1de154e7..df6614fc9 100644 --- a/ci/perf-tester/src/index.js +++ b/ci/perf-tester/src/index.js @@ -43,6 +43,9 @@ const runBenchmarks = async () => { return averageBenchmarks(results); }; +const perfActionComment = + ""; + async function run(octokit, context) { const { number: pull_number } = context.issue; @@ -132,7 +135,7 @@ async function run(octokit, context) { const comment = { ...commentInfo, - body: markdownDiff, + body: markdownDiff + "\n\n" + perfActionComment, }; startGroup(`Updating stats PR comment`); @@ -141,10 +144,7 @@ async function run(octokit, context) { const comments = (await octokit.issues.listComments(commentInfo)).data; for (let i = comments.length; i--; ) { const c = comments[i]; - if ( - c.user.type === "Bot" && - /[\s\n]*performance-action/.test(c.body) - ) { + if (c.user.type === "Bot" && c.body.includes(perfActionComment)) { commentId = c.id; break; } From 8e2c23bed5dae5c87247ff265665a74790536538 Mon Sep 17 00:00:00 2001 From: Jed Fox Date: Sat, 9 Apr 2022 17:40:51 -0400 Subject: [PATCH 5/7] bump number of benchmark runs (hopefully this will increase stability) --- ci/perf-tester/src/index.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ci/perf-tester/src/index.js b/ci/perf-tester/src/index.js index df6614fc9..ada5bb683 100644 --- a/ci/perf-tester/src/index.js +++ b/ci/perf-tester/src/index.js @@ -31,8 +31,8 @@ const { diffTable, } = require("./utils.js"); -const benchmarkParallel = 2; -const benchmarkSerial = 2; +const benchmarkParallel = 4; +const benchmarkSerial = 4; const runBenchmarks = async () => { let results = []; for (let i = 0; i < benchmarkSerial; i++) { From 1d86019354f1f5596e55b5fd39a18aff3fdb4cab Mon Sep 17 00:00:00 2001 From: Jed Fox Date: Sun, 10 Apr 2022 09:25:14 -0400 Subject: [PATCH 6/7] formatMS --- ci/perf-tester/src/utils.js | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/ci/perf-tester/src/utils.js b/ci/perf-tester/src/utils.js index 092c2e9dc..d5ba9288c 100644 --- a/ci/perf-tester/src/utils.js +++ b/ci/perf-tester/src/utils.js @@ -20,9 +20,13 @@ OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. */ -const fs = require("fs"); const { exec } = require("@actions/exec"); +const formatMS = (ms) => + `${ms.toLocaleString("en-US", { + maximumFractionDigits: 0, + })}ms`; + const getInput = (key) => ({ "build-script": "make bootstrap benchmark_setup", @@ -88,8 +92,7 @@ exports.toDiff = (before, after) => { * @param {number} difference */ function getDeltaText(delta, difference) { - let deltaText = - (delta > 0 ? "+" : "") + delta.toLocaleString("en-US") + "ms"; + let deltaText = (delta > 0 ? "+" : "") + formatMS(delta); if (delta && Math.abs(delta) > 1) { deltaText += ` (${Math.abs(difference)}%)`; } @@ -183,7 +186,7 @@ exports.diffTable = ( const columns = [ name, - time.toLocaleString("en-US") + "ms", + formatMS(time), getDeltaText(delta, difference), iconForDifference(difference), ]; @@ -198,16 +201,14 @@ exports.diffTable = ( if (unChangedRows.length !== 0) { const outUnchanged = markdownTable(unChangedRows); - out += `\n\n
ℹ️ View Unchanged\n\n${outUnchanged}\n\n
\n\n`; + out += `\n\n
View Unchanged\n\n${outUnchanged}\n\n
\n\n`; } if (showTotal) { const totalDifference = ((totalDelta / totalTime) * 100) | 0; let totalDeltaText = getDeltaText(totalDelta, totalDifference); let totalIcon = iconForDifference(totalDifference); - out = `**Total Time:** ${totalTime.toLocaleString( - "en-US" - )}ms\n\n${out}`; + out = `**Total Time:** ${formatMS(totalTime)}\n\n${out}`; out = `**Time Change:** ${totalDeltaText} ${totalIcon}\n\n${out}`; } From e841d55ca931c3d09e26519cea0d527059d54cf4 Mon Sep 17 00:00:00 2001 From: Jed Fox Date: Sun, 10 Apr 2022 09:28:01 -0400 Subject: [PATCH 7/7] refactor configuration --- ci/perf-tester/src/index.js | 19 +++++++------------ ci/perf-tester/src/utils.js | 17 +++++++---------- 2 files changed, 14 insertions(+), 22 deletions(-) diff --git a/ci/perf-tester/src/index.js b/ci/perf-tester/src/index.js index ada5bb683..e322e5f69 100644 --- a/ci/perf-tester/src/index.js +++ b/ci/perf-tester/src/index.js @@ -24,7 +24,7 @@ const { setFailed, startGroup, endGroup, debug } = require("@actions/core"); const { GitHub, context } = require("@actions/github"); const { exec } = require("@actions/exec"); const { - getInput, + config, runBenchmark, averageBenchmarks, toDiff, @@ -63,9 +63,8 @@ async function run(octokit, context) { `PR #${pull_number} is targetted at ${pr.base.ref} (${pr.base.sha})` ); - const buildScript = getInput("build-script"); - startGroup(`[current] Build using '${buildScript}'`); - await exec(buildScript); + startGroup(`[current] Build using '${config.buildScript}'`); + await exec(config.buildScript); endGroup(); startGroup(`[current] Running benchmark`); @@ -106,8 +105,8 @@ async function run(octokit, context) { } endGroup(); - startGroup(`[base] Build using '${buildScript}'`); - await exec(buildScript); + startGroup(`[base] Build using '${config.buildScript}'`); + await exec(config.buildScript); endGroup(); startGroup(`[base] Running benchmark`); @@ -120,10 +119,7 @@ async function run(octokit, context) { collapseUnchanged: true, omitUnchanged: false, showTotal: true, - minimumChangeThreshold: parseInt( - getInput("minimum-change-threshold"), - 10 - ), + minimumChangeThreshold: config.minimumChangeThreshold, }); let outputRawMarkdown = false; @@ -208,8 +204,7 @@ async function run(octokit, context) { (async () => { try { - const token = getInput("repo-token", { required: true }); - const octokit = new GitHub(token); + const octokit = new GitHub(process.env.GITHUB_TOKEN); await run(octokit, context); } catch (e) { setFailed(e.message); diff --git a/ci/perf-tester/src/utils.js b/ci/perf-tester/src/utils.js index d5ba9288c..a4b717ab7 100644 --- a/ci/perf-tester/src/utils.js +++ b/ci/perf-tester/src/utils.js @@ -27,19 +27,16 @@ const formatMS = (ms) => maximumFractionDigits: 0, })}ms`; -const getInput = (key) => - ({ - "build-script": "make bootstrap benchmark_setup", - benchmark: "make -s run_benchmark", - "minimum-change-threshold": 5, - "use-check": "no", - "repo-token": process.env.GITHUB_TOKEN, - }[key]); -exports.getInput = getInput; +const config = { + buildScript: "make bootstrap benchmark_setup", + benchmark: "make -s run_benchmark", + minimumChangeThreshold: 5, +}; +exports.config = config; exports.runBenchmark = async () => { let benchmarkBuffers = []; - await exec(getInput("benchmark"), [], { + await exec(config.benchmark, [], { listeners: { stdout: (data) => benchmarkBuffers.push(data), },