From 230f3f385fcd71ab754d64187dc641a941f6a5bd Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Tue, 13 Aug 2024 13:26:22 +0200 Subject: [PATCH 1/9] ci: Improve size-limit action --- dev-packages/size-limit-gh-action/index.mjs | 79 ++++++++------------- 1 file changed, 31 insertions(+), 48 deletions(-) diff --git a/dev-packages/size-limit-gh-action/index.mjs b/dev-packages/size-limit-gh-action/index.mjs index c42e9c523e94..37c43104b766 100644 --- a/dev-packages/size-limit-gh-action/index.mjs +++ b/dev-packages/size-limit-gh-action/index.mjs @@ -31,12 +31,27 @@ class SizeLimit { return bytes.format(size, { unitSeparator: ' ' }); } - formatTime(seconds) { - if (seconds >= 1) { - return `${Math.ceil(seconds * 10) / 10} s`; + formatPercentageChange(base = 0, current = 0) { + if (base === 0) { + return 'added'; + } + + if (current === 0) { + return 'removed'; + } + + const value = ((current - base) / base) * 100; + const formatted = (Math.sign(value) * Math.ceil(Math.abs(value) * 100)) / 100; + + if (value > 0) { + return `+${formatted}%`; } - return `${Math.ceil(seconds * 1000)} ms`; + if (value === 0) { + return '-'; + } + + return `${formatted}%`; } formatChange(base = 0, current = 0) { @@ -48,18 +63,18 @@ class SizeLimit { return 'removed'; } - const value = ((current - base) / base) * 100; - const formatted = (Math.sign(value) * Math.ceil(Math.abs(value) * 100)) / 100; + const value = current - base; + const formatted = this.formatBytes(value); if (value > 0) { - return `+${formatted}% 🔺`; + return `+${formatted} 🔺`; } if (value === 0) { - return `${formatted}%`; + return '-'; } - return `${formatted}% 🔽`; + return `${formatted} 🔽`; } formatLine(value, change) { @@ -67,16 +82,11 @@ class SizeLimit { } formatSizeResult(name, base, current) { - return [name, this.formatLine(this.formatBytes(current.size), this.formatChange(base.size, current.size))]; - } - - formatTimeResult(name, base, current) { return [ name, - this.formatLine(this.formatBytes(current.size), this.formatChange(base.size, current.size)), - this.formatLine(this.formatTime(current.loading), this.formatChange(base.loading, current.loading)), - this.formatLine(this.formatTime(current.running), this.formatChange(base.running, current.running)), - this.formatTime(current.total), + this.formatBytes(current.size), + this.formatPercentageChange(base.size, current.size), + this.formatChange(base.size, current.size), ]; } @@ -84,26 +94,12 @@ class SizeLimit { const results = JSON.parse(output); return results.reduce((current, result) => { - let time = {}; - - if (result.loading !== undefined && result.running !== undefined) { - const loading = +result.loading; - const running = +result.running; - - time = { - running, - loading, - total: loading + running, - }; - } - return { // biome-ignore lint/performance/noAccumulatingSpread: ...current, [result.name]: { name: result.name, size: +result.size, - ...time, }, }; }, {}); @@ -111,12 +107,6 @@ class SizeLimit { hasSizeChanges(base, current, threshold = 0) { const names = [...new Set([...(base ? Object.keys(base) : []), ...Object.keys(current)])]; - const isSize = names.some(name => current[name] && current[name].total === undefined); - - // Always return true if time results are present - if (!isSize) { - return true; - } return !!names.find(name => { const baseResult = base?.[name] || EmptyResult; @@ -132,16 +122,12 @@ class SizeLimit { formatResults(base, current) { const names = [...new Set([...(base ? Object.keys(base) : []), ...Object.keys(current)])]; - const isSize = names.some(name => current[name] && current[name].total === undefined); - const header = isSize ? SIZE_RESULTS_HEADER : TIME_RESULTS_HEADER; + const header = SIZE_RESULTS_HEADER; const fields = names.map(name => { const baseResult = base?.[name] || EmptyResult; const currentResult = current[name] || EmptyResult; - if (isSize) { - return this.formatSizeResult(name, baseResult, currentResult); - } - return this.formatTimeResult(name, baseResult, currentResult); + return this.formatSizeResult(name, baseResult, currentResult); }); return [header, ...fields]; @@ -165,15 +151,12 @@ async function execSizeLimit() { return { status, output }; } -const SIZE_RESULTS_HEADER = ['Path', 'Size']; -const TIME_RESULTS_HEADER = ['Path', 'Size', 'Loading time (3g)', 'Running time (snapdragon)', 'Total time']; +const SIZE_RESULTS_HEADER = ['Path', 'Size', 'Change']; const EmptyResult = { name: '-', size: 0, - running: 0, - loading: 0, - total: 0, + change: 0, }; async function run() { From b49a31b44045d2ee8edb9ccfcd88e26bb2b90ffa Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Tue, 13 Aug 2024 13:37:56 +0200 Subject: [PATCH 2/9] fix heading --- dev-packages/size-limit-gh-action/index.mjs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dev-packages/size-limit-gh-action/index.mjs b/dev-packages/size-limit-gh-action/index.mjs index 37c43104b766..76b62ee4c4c0 100644 --- a/dev-packages/size-limit-gh-action/index.mjs +++ b/dev-packages/size-limit-gh-action/index.mjs @@ -151,7 +151,7 @@ async function execSizeLimit() { return { status, output }; } -const SIZE_RESULTS_HEADER = ['Path', 'Size', 'Change']; +const SIZE_RESULTS_HEADER = ['Path', 'Size', '% Change', 'Change']; const EmptyResult = { name: '-', From 266f4f6d5a0ab4e5d5e05079a7fc13376e29a16b Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Tue, 13 Aug 2024 13:39:37 +0200 Subject: [PATCH 3/9] debug it... --- dev-packages/size-limit-gh-action/index.mjs | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/dev-packages/size-limit-gh-action/index.mjs b/dev-packages/size-limit-gh-action/index.mjs index 76b62ee4c4c0..6d4aa6545135 100644 --- a/dev-packages/size-limit-gh-action/index.mjs +++ b/dev-packages/size-limit-gh-action/index.mjs @@ -362,9 +362,11 @@ export async function getArtifactsForBranchAndWorkflow(octokit, { owner, repo, w } // Do not allow downloading artifacts from a fork. - completedWorkflowRuns.push( - ...response.data.filter(workflowRun => workflowRun.head_repository.full_name === `${owner}/${repo}`), - ); + const filtered = response.data.filter(workflowRun => workflowRun.head_repository.full_name === `${owner}/${repo}`); + + console.log(JSON.stringify(filtered, null, 2)); + + completedWorkflowRuns.push(...filtered); if (completedWorkflowRuns.length) { break; From 129a3aedb1475071b130ba9726309de811400327 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Tue, 13 Aug 2024 14:04:15 +0200 Subject: [PATCH 4/9] log if base is not latest --- dev-packages/size-limit-gh-action/index.mjs | 85 ++++++++++++--------- 1 file changed, 49 insertions(+), 36 deletions(-) diff --git a/dev-packages/size-limit-gh-action/index.mjs b/dev-packages/size-limit-gh-action/index.mjs index 6d4aa6545135..7363de47b5f5 100644 --- a/dev-packages/size-limit-gh-action/index.mjs +++ b/dev-packages/size-limit-gh-action/index.mjs @@ -210,6 +210,7 @@ async function run() { // Else, we run size limit for the current branch, AND fetch it for the comparison branch let base; let current; + let baseIsNotLatest = false; try { const artifacts = await getArtifactsForBranchAndWorkflow(octokit, { @@ -231,6 +232,11 @@ async function run() { }); base = JSON.parse(await fs.readFile(resultsFilePath, { encoding: 'utf8' })); + + if (!artifacts.isLatest) { + baseIsNotLatest = true; + core.info('Base artifact is not the latest one. This may lead to incorrect results.'); + } } catch (error) { core.startGroup('Warning, unable to find base results'); core.error(error); @@ -254,7 +260,17 @@ async function run() { isNaN(thresholdNumber) || limit.hasSizeChanges(base, current, thresholdNumber) || sizeLimitComment; if (shouldComment) { - const body = [SIZE_LIMIT_HEADING, markdownTable(limit.formatResults(base, current))].join('\r\n'); + const bodyParts = [SIZE_LIMIT_HEADING]; + + if (baseIsNotLatest) { + bodyParts.push( + '⚠️ **Warning:** Base artifact is not the latest one, because the latest workflow run is not done yet. This may lead to incorrect results.', + ); + } + + bodyParts.push(markdownTable(limit.formatResults(base, current))); + + const body = bodyParts.join('\r\n'); try { if (!sizeLimitComment) { @@ -303,7 +319,7 @@ const DEFAULT_PAGE_LIMIT = 10; * This is a bit hacky since GitHub Actions currently does not directly * support downloading artifacts from other workflows */ -export async function getArtifactsForBranchAndWorkflow(octokit, { owner, repo, workflowName, branch, artifactName }) { +async function getArtifactsForBranchAndWorkflow(octokit, { owner, repo, workflowName, branch, artifactName }) { core.startGroup(`getArtifactsForBranchAndWorkflow - workflow:"${workflowName}", branch:"${branch}"`); let repositoryWorkflow = null; @@ -344,14 +360,13 @@ export async function getArtifactsForBranchAndWorkflow(octokit, { owner, repo, w const workflow_id = repositoryWorkflow.id; let currentPage = 0; - const completedWorkflowRuns = []; + let latestWorkflowRun = null; for await (const response of octokit.paginate.iterator(octokit.rest.actions.listWorkflowRuns, { owner, repo, workflow_id, branch, - status: 'completed', per_page: DEFAULT_PAGE_LIMIT, event: 'push', })) { @@ -364,12 +379,38 @@ export async function getArtifactsForBranchAndWorkflow(octokit, { owner, repo, w // Do not allow downloading artifacts from a fork. const filtered = response.data.filter(workflowRun => workflowRun.head_repository.full_name === `${owner}/${repo}`); - console.log(JSON.stringify(filtered, null, 2)); + // Store the first workflow run, to determine if this is the latest one... + if (!latestWorkflowRun) { + latestWorkflowRun = filtered[0]; + } - completedWorkflowRuns.push(...filtered); + // Search through workflow artifacts until we find a workflow run w/ artifact name that we are looking for + for (const workflowRun of filtered) { + core.info(`Checking artifacts for workflow run: ${workflowRun.html_url}`); - if (completedWorkflowRuns.length) { - break; + const { + data: { artifacts }, + } = await octokit.rest.actions.listWorkflowRunArtifacts({ + owner, + repo, + run_id: workflowRun.id, + }); + + if (!artifacts) { + core.warning( + `Unable to fetch artifacts for branch: ${branch}, workflow: ${workflow_id}, workflowRunId: ${workflowRun.id}`, + ); + } else { + const foundArtifact = artifacts.find(({ name }) => name === artifactName); + if (foundArtifact) { + core.info(`Found suitable artifact: ${foundArtifact.url}`); + return { + artifact: foundArtifact, + workflowRun, + isLatest: latestWorkflowRun.id === workflowRun.id, + }; + } + } } if (currentPage > DEFAULT_MAX_PAGES) { @@ -381,34 +422,6 @@ export async function getArtifactsForBranchAndWorkflow(octokit, { owner, repo, w currentPage++; } - // Search through workflow artifacts until we find a workflow run w/ artifact name that we are looking for - for (const workflowRun of completedWorkflowRuns) { - core.info(`Checking artifacts for workflow run: ${workflowRun.html_url}`); - - const { - data: { artifacts }, - } = await octokit.rest.actions.listWorkflowRunArtifacts({ - owner, - repo, - run_id: workflowRun.id, - }); - - if (!artifacts) { - core.warning( - `Unable to fetch artifacts for branch: ${branch}, workflow: ${workflow_id}, workflowRunId: ${workflowRun.id}`, - ); - } else { - const foundArtifact = artifacts.find(({ name }) => name === artifactName); - if (foundArtifact) { - core.info(`Found suitable artifact: ${foundArtifact.url}`); - return { - artifact: foundArtifact, - workflowRun, - }; - } - } - } - core.warning(`Artifact not found: ${artifactName}`); core.endGroup(); return null; From f91fc9dbc1a301c994b6e3208205bbc55df45e6f Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Tue, 13 Aug 2024 15:22:26 +0200 Subject: [PATCH 5/9] debug stuff --- dev-packages/size-limit-gh-action/index.mjs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/dev-packages/size-limit-gh-action/index.mjs b/dev-packages/size-limit-gh-action/index.mjs index 7363de47b5f5..378df9ad347f 100644 --- a/dev-packages/size-limit-gh-action/index.mjs +++ b/dev-packages/size-limit-gh-action/index.mjs @@ -156,7 +156,6 @@ const SIZE_RESULTS_HEADER = ['Path', 'Size', '% Change', 'Change']; const EmptyResult = { name: '-', size: 0, - change: 0, }; async function run() { @@ -220,6 +219,8 @@ async function run() { workflowName: `${process.env.GITHUB_WORKFLOW || ''}`, }); + core.info(`Artifacts: ${JSON.stringify(artifacts, null, 2)}`); + if (!artifacts) { throw new Error('No artifacts found'); } From 79ffdb066e4b4df96f4e2c377dab6e9f2848ccc4 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Tue, 13 Aug 2024 16:12:21 +0200 Subject: [PATCH 6/9] link comparison workflow --- dev-packages/size-limit-gh-action/index.mjs | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/dev-packages/size-limit-gh-action/index.mjs b/dev-packages/size-limit-gh-action/index.mjs index 378df9ad347f..a0705df5f09a 100644 --- a/dev-packages/size-limit-gh-action/index.mjs +++ b/dev-packages/size-limit-gh-action/index.mjs @@ -210,6 +210,7 @@ async function run() { let base; let current; let baseIsNotLatest = false; + let baseWorkflowRun; try { const artifacts = await getArtifactsForBranchAndWorkflow(octokit, { @@ -219,12 +220,12 @@ async function run() { workflowName: `${process.env.GITHUB_WORKFLOW || ''}`, }); - core.info(`Artifacts: ${JSON.stringify(artifacts, null, 2)}`); - if (!artifacts) { throw new Error('No artifacts found'); } + baseWorkflowRun = artifacts.workflowRun; + await downloadOtherWorkflowArtifact(octokit, { ...repo, artifactName: ARTIFACT_NAME, @@ -271,6 +272,10 @@ async function run() { bodyParts.push(markdownTable(limit.formatResults(base, current))); + if (baseWorkflowRun) { + bodyParts.push(`[View base workflow run](${baseWorkflowRun.html_url})`); + } + const body = bodyParts.join('\r\n'); try { From aa24b4d15e5bb0bc45ab8c02f62fdb412027ffa4 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Tue, 13 Aug 2024 16:21:59 +0200 Subject: [PATCH 7/9] fix it... --- dev-packages/size-limit-gh-action/index.mjs | 1 + 1 file changed, 1 insertion(+) diff --git a/dev-packages/size-limit-gh-action/index.mjs b/dev-packages/size-limit-gh-action/index.mjs index a0705df5f09a..4ed24c7c8d5f 100644 --- a/dev-packages/size-limit-gh-action/index.mjs +++ b/dev-packages/size-limit-gh-action/index.mjs @@ -273,6 +273,7 @@ async function run() { bodyParts.push(markdownTable(limit.formatResults(base, current))); if (baseWorkflowRun) { + bodyParts.push(''); bodyParts.push(`[View base workflow run](${baseWorkflowRun.html_url})`); } From bcc7e8bc8776f616a97492e9772bdfd16bcbc2bb Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Tue, 13 Aug 2024 16:58:44 +0200 Subject: [PATCH 8/9] some more fixes? --- dev-packages/size-limit-gh-action/index.mjs | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/dev-packages/size-limit-gh-action/index.mjs b/dev-packages/size-limit-gh-action/index.mjs index 4ed24c7c8d5f..b42cdb50743b 100644 --- a/dev-packages/size-limit-gh-action/index.mjs +++ b/dev-packages/size-limit-gh-action/index.mjs @@ -386,6 +386,11 @@ async function getArtifactsForBranchAndWorkflow(octokit, { owner, repo, workflow // Do not allow downloading artifacts from a fork. const filtered = response.data.filter(workflowRun => workflowRun.head_repository.full_name === `${owner}/${repo}`); + // Sort to ensure the latest workflow run is the first + filtered.sort((a, b) => { + return new Date(b.created_at).getTime() - new Date(a.created_at).getTime(); + }); + // Store the first workflow run, to determine if this is the latest one... if (!latestWorkflowRun) { latestWorkflowRun = filtered[0]; @@ -416,6 +421,8 @@ async function getArtifactsForBranchAndWorkflow(octokit, { owner, repo, workflow workflowRun, isLatest: latestWorkflowRun.id === workflowRun.id, }; + } else { + core.info(`No artifact found for ${artifactName}, trying next workflow run...`); } } } From 117430aa99fb704307980ada4de697d44df29135 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Wed, 14 Aug 2024 10:41:57 +0200 Subject: [PATCH 9/9] text change --- dev-packages/size-limit-gh-action/index.mjs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dev-packages/size-limit-gh-action/index.mjs b/dev-packages/size-limit-gh-action/index.mjs index b42cdb50743b..3dbb8aa22127 100644 --- a/dev-packages/size-limit-gh-action/index.mjs +++ b/dev-packages/size-limit-gh-action/index.mjs @@ -266,7 +266,7 @@ async function run() { if (baseIsNotLatest) { bodyParts.push( - '⚠️ **Warning:** Base artifact is not the latest one, because the latest workflow run is not done yet. This may lead to incorrect results.', + '⚠️ **Warning:** Base artifact is not the latest one, because the latest workflow run is not done yet. This may lead to incorrect results. Try to re-run all tests to get up to date results.', ); }