From 769a768c30d4ca1209fcdd79475132bd1cf332a0 Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Wed, 19 Mar 2025 17:18:16 +0100 Subject: [PATCH 01/18] split up too big output files in e2e performance pipelein --- .github/workflows/e2ePerformanceTests.yml | 31 ++++++++++++++++++++++- 1 file changed, 30 insertions(+), 1 deletion(-) diff --git a/.github/workflows/e2ePerformanceTests.yml b/.github/workflows/e2ePerformanceTests.yml index 380b874c6c95..0c2aa802f82b 100644 --- a/.github/workflows/e2ePerformanceTests.yml +++ b/.github/workflows/e2ePerformanceTests.yml @@ -217,6 +217,20 @@ jobs: if: always() run: cat "./Host_Machine_Files/\$WORKING_DIRECTORY/output.md" + - name: Split and post large output files + if: always() + run: | + # Get the size of the output file in bytes + file_size=$(stat -f%z "./Host_Machine_Files/\$WORKING_DIRECTORY/output.md") + + # If file is larger than 65536 bytes (64KB), split it + if [ $file_size -gt 65536 ]; then + # Split the file into chunks of 65000 characters (leaving room for headers) + split -b 65000 "./Host_Machine_Files/\$WORKING_DIRECTORY/output.md" "./Host_Machine_Files/\$WORKING_DIRECTORY/output_" + fi + env: + GITHUB_TOKEN: ${{ github.token }} + - name: Check if test failed, if so post the results and add the DeployBlocker label id: checkIfRegressionDetected run: | @@ -225,7 +239,22 @@ jobs: echo "performanceRegressionDetected=true" >> "$GITHUB_OUTPUT" gh pr edit ${{ inputs.PR_NUMBER }} --add-label DeployBlockerCash - gh pr comment ${{ inputs.PR_NUMBER }} -F "./Host_Machine_Files/\$WORKING_DIRECTORY/output.md" + + if [ -f "./Host_Machine_Files/\$WORKING_DIRECTORY/output_0.md" ]; then + # Post each split file as a separate comment + for file in "./Host_Machine_Files/\$WORKING_DIRECTORY/output_*"; do + if [ -f "$file" ]; then + # Add a header to indicate which part this is + echo "## Performance Test Results (Part $(basename "$file" | sed 's/output_//'))" > temp.md + cat "$file" >> temp.md + gh pr comment ${{ inputs.PR_NUMBER }} -F temp.md + rm temp.md + fi + done + else + gh pr comment ${{ inputs.PR_NUMBER }} -F "./Host_Machine_Files/\$WORKING_DIRECTORY/output.md" + fi + gh pr comment ${{ inputs.PR_NUMBER }} -b "@Expensify/mobile-deployers 📣 Please look into this performance regression as it's a deploy blocker." else echo "performanceRegressionDetected=false" >> "$GITHUB_OUTPUT" From 5e03a1b2b802b47edacb0ef1da07f2494ab8d434 Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Wed, 19 Mar 2025 17:24:07 +0100 Subject: [PATCH 02/18] fix: decrease maximum file size threshold and improve check --- .github/workflows/e2ePerformanceTests.yml | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/.github/workflows/e2ePerformanceTests.yml b/.github/workflows/e2ePerformanceTests.yml index 0c2aa802f82b..5485c8c8bb46 100644 --- a/.github/workflows/e2ePerformanceTests.yml +++ b/.github/workflows/e2ePerformanceTests.yml @@ -223,10 +223,10 @@ jobs: # Get the size of the output file in bytes file_size=$(stat -f%z "./Host_Machine_Files/\$WORKING_DIRECTORY/output.md") - # If file is larger than 65536 bytes (64KB), split it - if [ $file_size -gt 65536 ]; then - # Split the file into chunks of 65000 characters (leaving room for headers) - split -b 65000 "./Host_Machine_Files/\$WORKING_DIRECTORY/output.md" "./Host_Machine_Files/\$WORKING_DIRECTORY/output_" + # If file is larger than 61440 bytes (60KB), split it + if [ $file_size -gt 61440 ]; then + # Split the file into chunks of 60KB files + split -b 61440 "./Host_Machine_Files/\$WORKING_DIRECTORY/output.md" "./Host_Machine_Files/\$WORKING_DIRECTORY/output_" fi env: GITHUB_TOKEN: ${{ github.token }} @@ -240,9 +240,10 @@ jobs: gh pr edit ${{ inputs.PR_NUMBER }} --add-label DeployBlockerCash - if [ -f "./Host_Machine_Files/\$WORKING_DIRECTORY/output_0.md" ]; then - # Post each split file as a separate comment - for file in "./Host_Machine_Files/\$WORKING_DIRECTORY/output_*"; do + # Check if there are any split files + if ls "./Host_Machine_Files/\$WORKING_DIRECTORY"/output_* 1> /dev/null 2>&1; then + # Post each split file as a separate comment + for file in "./Host_Machine_Files/\$WORKING_DIRECTORY/output_"*; do if [ -f "$file" ]; then # Add a header to indicate which part this is echo "## Performance Test Results (Part $(basename "$file" | sed 's/output_//'))" > temp.md From 8a591d0d262f252ea78005ed8de97b5de2d5b28d Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Wed, 19 Mar 2025 17:27:28 +0100 Subject: [PATCH 03/18] rename step --- .github/workflows/e2ePerformanceTests.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/e2ePerformanceTests.yml b/.github/workflows/e2ePerformanceTests.yml index 5485c8c8bb46..03c4d645bf0c 100644 --- a/.github/workflows/e2ePerformanceTests.yml +++ b/.github/workflows/e2ePerformanceTests.yml @@ -217,7 +217,7 @@ jobs: if: always() run: cat "./Host_Machine_Files/\$WORKING_DIRECTORY/output.md" - - name: Split and post large output files + - name: Split up too large output files if: always() run: | # Get the size of the output file in bytes From 6190d9cd020358325e633ac6276d71334c81ce36 Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Wed, 19 Mar 2025 19:33:41 +0100 Subject: [PATCH 04/18] fix: wrong stat command under linux --- .github/workflows/e2ePerformanceTests.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/e2ePerformanceTests.yml b/.github/workflows/e2ePerformanceTests.yml index 03c4d645bf0c..cf1d3337a8df 100644 --- a/.github/workflows/e2ePerformanceTests.yml +++ b/.github/workflows/e2ePerformanceTests.yml @@ -221,7 +221,7 @@ jobs: if: always() run: | # Get the size of the output file in bytes - file_size=$(stat -f%z "./Host_Machine_Files/\$WORKING_DIRECTORY/output.md") + file_size=$(stat -c %s "./Host_Machine_Files/\$WORKING_DIRECTORY/output.md") # If file is larger than 61440 bytes (60KB), split it if [ $file_size -gt 61440 ]; then From 6e61bdf8043cae23c2bba8589c9c7e9432eec2aa Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Sat, 29 Mar 2025 14:14:09 +0100 Subject: [PATCH 05/18] fix: split e2e performance test markdown into multiple files --- tests/e2e/compare/compare.ts | 6 +- tests/e2e/compare/output/markdown.ts | 92 ++++++++++++++++++++-------- tests/e2e/testRunner.ts | 2 +- 3 files changed, 70 insertions(+), 30 deletions(-) diff --git a/tests/e2e/compare/compare.ts b/tests/e2e/compare/compare.ts index ad38c249bff3..6e7c04129348 100644 --- a/tests/e2e/compare/compare.ts +++ b/tests/e2e/compare/compare.ts @@ -92,13 +92,13 @@ function compareResults(baselineEntries: Metric | string, compareEntries: Metric } type Options = { - outputFile: string; + outputDir: string; outputFormat: 'console' | 'markdown' | 'all'; metricForTest: Record; skippedTests: string[]; }; -export default (main: Metric | string, delta: Metric | string, {outputFile, outputFormat = 'all', metricForTest = {}, skippedTests}: Options) => { +export default (main: Metric | string, delta: Metric | string, {outputDir, outputFormat = 'all', metricForTest = {}, skippedTests}: Options) => { // IMPORTANT NOTE: make sure you are passing the main/baseline results first, then the delta/compare results: const outputData = compareResults(main, delta, metricForTest); @@ -107,7 +107,7 @@ export default (main: Metric | string, delta: Metric | string, {outputFile, outp } if (outputFormat === 'markdown' || outputFormat === 'all') { - return writeToMarkdown(outputFile, outputData, skippedTests); + return writeToMarkdown(outputDir, outputData, skippedTests); } }; export {compareResults}; diff --git a/tests/e2e/compare/output/markdown.ts b/tests/e2e/compare/output/markdown.ts index bd32d2d99ab2..721dc1177bda 100644 --- a/tests/e2e/compare/output/markdown.ts +++ b/tests/e2e/compare/output/markdown.ts @@ -7,6 +7,8 @@ import type {Data, Entry} from './console'; import * as format from './format'; import markdownTable from './markdownTable'; +const TIMES_TO_SPLIT_MEANINGLESS_CHANGES = 1; + const tableHeader = ['Name', 'Duration']; const collapsibleSection = (title: string, content: string) => `
\n${title}\n\n${content}\n
\n\n`; @@ -45,15 +47,27 @@ const formatEntryDuration = (entry: Entry): string => { return ''; }; -const buildDetailsTable = (entries: Entry[]) => { +const buildDetailsTable = (entries: Entry[], timesToSplit = 0) => { if (!entries.length) { return ''; } - const rows = entries.map((entry) => [entry.name, buildDurationDetailsEntry(entry)]); - const content = markdownTable([tableHeader, ...rows]); + const entriesPerTable = Math.floor(entries.length / timesToSplit); + const tables: string[] = []; + for (let i = 0; i < timesToSplit; i++) { + const start = i * entriesPerTable; + const end = i === timesToSplit - 1 ? entries.length : start + entriesPerTable; + const tableEntries = entries.slice(start, end); + + const rows = tableEntries.map((entry) => [entry.name, buildDurationDetailsEntry(entry)]); + const content = markdownTable([tableHeader, ...rows]); - return collapsibleSection('Show details', content); + const tableMarkdown = collapsibleSection('Show details', content); + + tables.push(tableMarkdown); + } + + return tables; }; const buildSummaryTable = (entries: Entry[], collapse = false) => { @@ -68,35 +82,49 @@ const buildSummaryTable = (entries: Entry[], collapse = false) => { }; const buildMarkdown = (data: Data, skippedTests: string[]) => { - let result = '## Performance Comparison Report 📊'; + let mainFile = '## Performance Comparison Report 📊'; + mainFile += TIMES_TO_SPLIT_MEANINGLESS_CHANGES > 0 ? ` (1/${TIMES_TO_SPLIT_MEANINGLESS_CHANGES + 1})` : ''; if (data.errors?.length) { - result += '\n\n### Errors\n'; + mainFile += '\n\n### Errors\n'; data.errors.forEach((message) => { - result += ` 1. 🛑 ${message}\n`; + mainFile += ` 1. 🛑 ${message}\n`; }); } if (data.warnings?.length) { - result += '\n\n### Warnings\n'; + mainFile += '\n\n### Warnings\n'; data.warnings.forEach((message) => { - result += ` 1. 🟡 ${message}\n`; + mainFile += ` 1. 🟡 ${message}\n`; }); } - result += '\n\n### Significant Changes To Duration'; - result += `\n${buildSummaryTable(data.significance)}`; - result += `\n${buildDetailsTable(data.significance)}`; - result += '\n\n### Meaningless Changes To Duration'; - result += `\n${buildSummaryTable(data.meaningless, true)}`; - result += `\n${buildDetailsTable(data.meaningless)}`; - result += '\n'; - if (skippedTests.length > 0) { - result += `⚠️ Some tests did not pass successfully, so some results are omitted from final report: ${skippedTests.join(', ')}`; + mainFile += `⚠️ Some tests did not pass successfully, so some results are omitted from final report: ${skippedTests.join(', ')}`; } - return result; + mainFile += '\n\n### Significant Changes To Duration'; + mainFile += `\n${buildSummaryTable(data.significance)}`; + mainFile += `\n${buildDetailsTable(data.significance).at(0)}`; + + const meaninglessDetailsTables = buildDetailsTable(data.meaningless, TIMES_TO_SPLIT_MEANINGLESS_CHANGES); + + const extraFiles: string[] = []; + for (let i = 0; i < TIMES_TO_SPLIT_MEANINGLESS_CHANGES; i++) { + let extraFile = '## Performance Comparison Report 📊'; + extraFile += TIMES_TO_SPLIT_MEANINGLESS_CHANGES > 0 ? ` (${i + 2}/${TIMES_TO_SPLIT_MEANINGLESS_CHANGES + 1})` : ''; + + extraFile += '\n\n### Meaningless Changes To Duration'; + extraFile += TIMES_TO_SPLIT_MEANINGLESS_CHANGES > 0 ? ` (${i + 1}/${TIMES_TO_SPLIT_MEANINGLESS_CHANGES + 1})` : ''; + + extraFile += `\n${buildSummaryTable(data.meaningless, true)}`; + extraFile += `\n${meaninglessDetailsTables[i]}`; + extraFile += '\n'; + + extraFiles.push(extraFile); + } + + return [mainFile, ...extraFiles]; }; const writeToFile = (filePath: string, content: string) => @@ -113,13 +141,25 @@ const writeToFile = (filePath: string, content: string) => throw error; }); -const writeToMarkdown = (filePath: string, data: Data, skippedTests: string[]) => { - const markdown = buildMarkdown(data, skippedTests); - Logger.info('Markdown was built successfully, writing to file...', markdown); - return writeToFile(filePath, markdown).catch((error) => { - console.error(error); - throw error; - }); +const writeToMarkdown = (outputDir: string, data: Data, skippedTests: string[]) => { + const markdownFiles = buildMarkdown(data, skippedTests); + const filesString = markdownFiles.join('\n\n'); + Logger.info('Markdown was built successfully, writing to file...', filesString); + + if (markdownFiles.length === 1) { + // eslint-disable-next-line @typescript-eslint/no-non-null-assertion + return writeToFile(path.join(outputDir, 'output.md'), markdownFiles.at(0)!); + } + + return Promise.all( + markdownFiles.map((file, index) => { + const filePath = `${outputDir}/output-${index + 1}.md`; + return writeToFile(filePath, file).catch((error) => { + console.error(error); + throw error; + }); + }), + ); }; export default writeToMarkdown; diff --git a/tests/e2e/testRunner.ts b/tests/e2e/testRunner.ts index 7d55e0c81e48..c639834dc4dc 100644 --- a/tests/e2e/testRunner.ts +++ b/tests/e2e/testRunner.ts @@ -357,7 +357,7 @@ const runTests = async (): Promise => { // Calculate statistics and write them to our work file Logger.info('Calculating statics and writing results'); await compare(results.main, results.delta, { - outputFile: `${config.OUTPUT_DIR}/output.md`, + outputDir: config.OUTPUT_DIR, outputFormat: 'all', metricForTest, skippedTests, From 50d729f95867168cec2b8db09fb221e863dfe6b8 Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Sat, 29 Mar 2025 14:17:18 +0100 Subject: [PATCH 06/18] chore: remove splitting up step in e2e performance workflow --- .github/workflows/e2ePerformanceTests.yml | 20 +------------------- 1 file changed, 1 insertion(+), 19 deletions(-) diff --git a/.github/workflows/e2ePerformanceTests.yml b/.github/workflows/e2ePerformanceTests.yml index cf1d3337a8df..42782cea5c7e 100644 --- a/.github/workflows/e2ePerformanceTests.yml +++ b/.github/workflows/e2ePerformanceTests.yml @@ -217,20 +217,6 @@ jobs: if: always() run: cat "./Host_Machine_Files/\$WORKING_DIRECTORY/output.md" - - name: Split up too large output files - if: always() - run: | - # Get the size of the output file in bytes - file_size=$(stat -c %s "./Host_Machine_Files/\$WORKING_DIRECTORY/output.md") - - # If file is larger than 61440 bytes (60KB), split it - if [ $file_size -gt 61440 ]; then - # Split the file into chunks of 60KB files - split -b 61440 "./Host_Machine_Files/\$WORKING_DIRECTORY/output.md" "./Host_Machine_Files/\$WORKING_DIRECTORY/output_" - fi - env: - GITHUB_TOKEN: ${{ github.token }} - - name: Check if test failed, if so post the results and add the DeployBlocker label id: checkIfRegressionDetected run: | @@ -245,11 +231,7 @@ jobs: # Post each split file as a separate comment for file in "./Host_Machine_Files/\$WORKING_DIRECTORY/output_"*; do if [ -f "$file" ]; then - # Add a header to indicate which part this is - echo "## Performance Test Results (Part $(basename "$file" | sed 's/output_//'))" > temp.md - cat "$file" >> temp.md - gh pr comment ${{ inputs.PR_NUMBER }} -F temp.md - rm temp.md + gh pr comment ${{ inputs.PR_NUMBER }} -F $file fi done else From 6b89ba3115a5e64fc1a8ba569a883947490b54d7 Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Mon, 31 Mar 2025 08:05:43 +0200 Subject: [PATCH 07/18] fix: improve file splitting even more --- tests/e2e/compare/output/markdown.ts | 36 ++++++++++++++++++++++------ 1 file changed, 29 insertions(+), 7 deletions(-) diff --git a/tests/e2e/compare/output/markdown.ts b/tests/e2e/compare/output/markdown.ts index 721dc1177bda..627e90ee9e5e 100644 --- a/tests/e2e/compare/output/markdown.ts +++ b/tests/e2e/compare/output/markdown.ts @@ -7,7 +7,9 @@ import type {Data, Entry} from './console'; import * as format from './format'; import markdownTable from './markdownTable'; -const TIMES_TO_SPLIT_MEANINGLESS_CHANGES = 1; +const MAX_CHARACTERS_PER_FILE = 65536; +const FILE_SIZE_SAFETY_MARGIN = 1000; +const MAX_CHARACTERS_PER_FILE_WITH_SAFETY_MARGIN = MAX_CHARACTERS_PER_FILE - FILE_SIZE_SAFETY_MARGIN; const tableHeader = ['Name', 'Duration']; @@ -81,9 +83,21 @@ const buildSummaryTable = (entries: Entry[], collapse = false) => { return collapse ? collapsibleSection('Show entries', content) : content; }; -const buildMarkdown = (data: Data, skippedTests: string[]) => { +const buildMarkdown = (data: Data, skippedTests: string[], numberOfExtraFiles?: number) => { + let nExtraFiles = numberOfExtraFiles ?? 0; + if (!numberOfExtraFiles) { + const singleMarkdown = buildMarkdown(data, skippedTests).at(0); + const totalCharacters = singleMarkdown?.length ?? 0; + + if (totalCharacters > MAX_CHARACTERS_PER_FILE_WITH_SAFETY_MARGIN) { + nExtraFiles = Math.ceil(totalCharacters / MAX_CHARACTERS_PER_FILE_WITH_SAFETY_MARGIN); + } else { + nExtraFiles = 0; + } + } + let mainFile = '## Performance Comparison Report 📊'; - mainFile += TIMES_TO_SPLIT_MEANINGLESS_CHANGES > 0 ? ` (1/${TIMES_TO_SPLIT_MEANINGLESS_CHANGES + 1})` : ''; + mainFile += nExtraFiles > 0 ? ` (1/${nExtraFiles + 1})` : ''; if (data.errors?.length) { mainFile += '\n\n### Errors\n'; @@ -107,15 +121,23 @@ const buildMarkdown = (data: Data, skippedTests: string[]) => { mainFile += `\n${buildSummaryTable(data.significance)}`; mainFile += `\n${buildDetailsTable(data.significance).at(0)}`; - const meaninglessDetailsTables = buildDetailsTable(data.meaningless, TIMES_TO_SPLIT_MEANINGLESS_CHANGES); + const meaninglessDetailsTables = buildDetailsTable(data.meaningless, nExtraFiles); + + if (nExtraFiles === 0) { + mainFile += '\n\n### Meaningless Changes To Duration'; + mainFile += `\n${buildSummaryTable(data.meaningless, true)}`; + mainFile += `\n${meaninglessDetailsTables[0]}`; + + return [mainFile]; + } const extraFiles: string[] = []; - for (let i = 0; i < TIMES_TO_SPLIT_MEANINGLESS_CHANGES; i++) { + for (let i = 0; i < nExtraFiles; i++) { let extraFile = '## Performance Comparison Report 📊'; - extraFile += TIMES_TO_SPLIT_MEANINGLESS_CHANGES > 0 ? ` (${i + 2}/${TIMES_TO_SPLIT_MEANINGLESS_CHANGES + 1})` : ''; + extraFile += nExtraFiles > 0 ? ` (${i + 2}/${nExtraFiles + 1})` : ''; extraFile += '\n\n### Meaningless Changes To Duration'; - extraFile += TIMES_TO_SPLIT_MEANINGLESS_CHANGES > 0 ? ` (${i + 1}/${TIMES_TO_SPLIT_MEANINGLESS_CHANGES + 1})` : ''; + extraFile += nExtraFiles > 0 ? ` (${i + 1}/${nExtraFiles + 1})` : ''; extraFile += `\n${buildSummaryTable(data.meaningless, true)}`; extraFile += `\n${meaninglessDetailsTables[i]}`; From 348a36f25673bb2549cee468202ae2e4d834897e Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Mon, 31 Mar 2025 08:09:53 +0200 Subject: [PATCH 08/18] fix: e2e performance tests lint pipeline build --- .github/workflows/checkE2ETestCode.yml | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/.github/workflows/checkE2ETestCode.yml b/.github/workflows/checkE2ETestCode.yml index 090b7a7f23e4..c8ac4c546f30 100644 --- a/.github/workflows/checkE2ETestCode.yml +++ b/.github/workflows/checkE2ETestCode.yml @@ -18,6 +18,8 @@ jobs: - name: Setup Node uses: ./.github/actions/composite/setupNode + with: + IS_DESKTOP_BUILD: 'true' - name: Verify e2e tests compile correctly - run: npm run e2e-test-runner-build \ No newline at end of file + run: npm run e2e-test-runner-build From 2d6ea56f2768ddf3bbef5ff8066c836a04de9d1c Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Mon, 31 Mar 2025 08:14:05 +0200 Subject: [PATCH 09/18] fix: endless recursion --- tests/e2e/compare/output/markdown.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/e2e/compare/output/markdown.ts b/tests/e2e/compare/output/markdown.ts index 627e90ee9e5e..b65973337d9d 100644 --- a/tests/e2e/compare/output/markdown.ts +++ b/tests/e2e/compare/output/markdown.ts @@ -86,7 +86,7 @@ const buildSummaryTable = (entries: Entry[], collapse = false) => { const buildMarkdown = (data: Data, skippedTests: string[], numberOfExtraFiles?: number) => { let nExtraFiles = numberOfExtraFiles ?? 0; if (!numberOfExtraFiles) { - const singleMarkdown = buildMarkdown(data, skippedTests).at(0); + const singleMarkdown = buildMarkdown(data, skippedTests, 0).at(0); const totalCharacters = singleMarkdown?.length ?? 0; if (totalCharacters > MAX_CHARACTERS_PER_FILE_WITH_SAFETY_MARGIN) { From 11b5b2f25e0997db848ce7e52d0ff1b1382ca239 Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Mon, 31 Mar 2025 08:14:52 +0200 Subject: [PATCH 10/18] fix: recursion --- tests/e2e/compare/output/markdown.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/e2e/compare/output/markdown.ts b/tests/e2e/compare/output/markdown.ts index b65973337d9d..fa29a23b5818 100644 --- a/tests/e2e/compare/output/markdown.ts +++ b/tests/e2e/compare/output/markdown.ts @@ -85,7 +85,7 @@ const buildSummaryTable = (entries: Entry[], collapse = false) => { const buildMarkdown = (data: Data, skippedTests: string[], numberOfExtraFiles?: number) => { let nExtraFiles = numberOfExtraFiles ?? 0; - if (!numberOfExtraFiles) { + if (numberOfExtraFiles === undefined) { const singleMarkdown = buildMarkdown(data, skippedTests, 0).at(0); const totalCharacters = singleMarkdown?.length ?? 0; From 2dcd54206a591af3891a92af88e8d2b26deca1f6 Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Mon, 31 Mar 2025 08:19:47 +0200 Subject: [PATCH 11/18] fix: buildDetailsTable --- tests/e2e/compare/output/markdown.ts | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/tests/e2e/compare/output/markdown.ts b/tests/e2e/compare/output/markdown.ts index fa29a23b5818..c5cb248678ea 100644 --- a/tests/e2e/compare/output/markdown.ts +++ b/tests/e2e/compare/output/markdown.ts @@ -49,16 +49,16 @@ const formatEntryDuration = (entry: Entry): string => { return ''; }; -const buildDetailsTable = (entries: Entry[], timesToSplit = 0) => { +const buildDetailsTable = (entries: Entry[], numberOfTables = 1) => { if (!entries.length) { return ''; } - const entriesPerTable = Math.floor(entries.length / timesToSplit); + const entriesPerTable = Math.floor(entries.length / numberOfTables); const tables: string[] = []; - for (let i = 0; i < timesToSplit; i++) { + for (let i = 0; i < numberOfTables; i++) { const start = i * entriesPerTable; - const end = i === timesToSplit - 1 ? entries.length : start + entriesPerTable; + const end = i === numberOfTables - 1 ? entries.length : start + entriesPerTable; const tableEntries = entries.slice(start, end); const rows = tableEntries.map((entry) => [entry.name, buildDurationDetailsEntry(entry)]); @@ -119,14 +119,14 @@ const buildMarkdown = (data: Data, skippedTests: string[], numberOfExtraFiles?: mainFile += '\n\n### Significant Changes To Duration'; mainFile += `\n${buildSummaryTable(data.significance)}`; - mainFile += `\n${buildDetailsTable(data.significance).at(0)}`; + mainFile += `\n${buildDetailsTable(data.significance, 1).at(0)}`; const meaninglessDetailsTables = buildDetailsTable(data.meaningless, nExtraFiles); if (nExtraFiles === 0) { mainFile += '\n\n### Meaningless Changes To Duration'; mainFile += `\n${buildSummaryTable(data.meaningless, true)}`; - mainFile += `\n${meaninglessDetailsTables[0]}`; + mainFile += `\n${meaninglessDetailsTables.at(0)}`; return [mainFile]; } From 8103ceecddf90192c860d8c3e496588e749bf450 Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Mon, 31 Mar 2025 08:27:31 +0200 Subject: [PATCH 12/18] Update E2EMarkdownTest.ts.snap --- .../__snapshots__/E2EMarkdownTest.ts.snap | 43 +++++++++---------- 1 file changed, 21 insertions(+), 22 deletions(-) diff --git a/tests/unit/__snapshots__/E2EMarkdownTest.ts.snap b/tests/unit/__snapshots__/E2EMarkdownTest.ts.snap index 9a66a0e2e823..7abea3e085ef 100644 --- a/tests/unit/__snapshots__/E2EMarkdownTest.ts.snap +++ b/tests/unit/__snapshots__/E2EMarkdownTest.ts.snap @@ -1,24 +1,23 @@ // Jest Snapshot v1, https://goo.gl/fbAQLP -exports[`markdown formatter should format significant changes properly 1`] = ` -"## Performance Comparison Report 📊 - -### Significant Changes To Duration -| Name | Duration | -| -------------- | --------------------------------------------------- | -| commentLinking | 131.681 ms → 421.806 ms (+290.125 ms, +220.3%) 🔴🔴 | -
-Show details - -| Name | Duration | -| -------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ | -| commentLinking | **Baseline**
Mean: 131.681 ms
Stdev: 19.883 ms (15.1%)
Runs: 100.5145680010319 112.0048420019448 121.8861090019345 124.26110899820924 135.1571460030973 140.33837900310755 158.5825610011816 160.7034499980509

**Current**
Mean: 421.806 ms
Stdev: 30.185 ms (7.2%)
Runs: 361.5145680010319 402.8861090019345 412.0048420019448 414.26110899820924 425.1571460030973 440.33837900310755 458.7034499980509 459.5825610011816 | -
- - - -### Meaningless Changes To Duration -_There are no entries_ - -" -`; +exports[`markdown formatter should format significant changes properly 1`] = [ + '## Performance Comparison Report 📊\n' + + '\n' + + '### Significant Changes To Duration\n' + + '| Name | Duration |\n' + + '| -------------- | --------------------------------------------------- |\n' + + '| commentLinking | 131.681 ms → 421.806 ms (+290.125 ms, +220.3%) 🔴🔴 |\n' + + '
\n' + + 'Show details\n' + + '\n' + + '| Name | Duration |\n' + + '| -------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ |\n' + + '| commentLinking | **Baseline**
Mean: 131.681 ms
Stdev: 19.883 ms (15.1%)
Runs: 100.5145680010319 112.0048420019448 121.8861090019345 124.26110899820924 135.1571460030973 140.33837900310755 158.5825610011816 160.7034499980509

**Current**
Mean: 421.806 ms
Stdev: 30.185 ms (7.2%)
Runs: 361.5145680010319 402.8861090019345 412.0048420019448 414.26110899820924 425.1571460030973 440.33837900310755 458.7034499980509 459.5825610011816 |\n' + + '
\n' + + '\n' + + '\n' + + '\n' + + '### Meaningless Changes To Duration\n' + + '_There are no entries_\n' + + 'undefined' +]; From 4580aeafd0944e98b98a45eb3092a7274adb54c3 Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Mon, 31 Mar 2025 08:39:56 +0200 Subject: [PATCH 13/18] fix: output files in pipeline --- .github/workflows/e2ePerformanceTests.yml | 8 ++++---- tests/e2e/compare/output/markdown.ts | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/.github/workflows/e2ePerformanceTests.yml b/.github/workflows/e2ePerformanceTests.yml index 42782cea5c7e..6ed783571067 100644 --- a/.github/workflows/e2ePerformanceTests.yml +++ b/.github/workflows/e2ePerformanceTests.yml @@ -220,18 +220,18 @@ jobs: - name: Check if test failed, if so post the results and add the DeployBlocker label id: checkIfRegressionDetected run: | - if grep -q '🔴' "./Host_Machine_Files/\$WORKING_DIRECTORY/output.md"; then + if grep -q '🔴' "./Host_Machine_Files/\$WORKING_DIRECTORY/output1.md"; then # Create an output to the GH action that the test failed: echo "performanceRegressionDetected=true" >> "$GITHUB_OUTPUT" gh pr edit ${{ inputs.PR_NUMBER }} --add-label DeployBlockerCash # Check if there are any split files - if ls "./Host_Machine_Files/\$WORKING_DIRECTORY"/output_* 1> /dev/null 2>&1; then + if ls "./Host_Machine_Files/\$WORKING_DIRECTORY"/output2.md 1> /dev/null 2>&1; then # Post each split file as a separate comment - for file in "./Host_Machine_Files/\$WORKING_DIRECTORY/output_"*; do + for file in "./Host_Machine_Files/\$WORKING_DIRECTORY/output"*; do if [ -f "$file" ]; then - gh pr comment ${{ inputs.PR_NUMBER }} -F $file + gh pr comment ${{ inputs.PR_NUMBER }} -F "$file" fi done else diff --git a/tests/e2e/compare/output/markdown.ts b/tests/e2e/compare/output/markdown.ts index c5cb248678ea..823be3f91fe1 100644 --- a/tests/e2e/compare/output/markdown.ts +++ b/tests/e2e/compare/output/markdown.ts @@ -170,7 +170,7 @@ const writeToMarkdown = (outputDir: string, data: Data, skippedTests: string[]) if (markdownFiles.length === 1) { // eslint-disable-next-line @typescript-eslint/no-non-null-assertion - return writeToFile(path.join(outputDir, 'output.md'), markdownFiles.at(0)!); + return writeToFile(path.join(outputDir, 'output1.md'), markdownFiles.at(0)!); } return Promise.all( From 8c4b7bf38ad561c0338e1d54104665be5815ca9e Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Mon, 31 Mar 2025 09:02:17 +0200 Subject: [PATCH 14/18] fix: simplify and fix array logic --- tests/e2e/compare/output/markdown.ts | 26 +++++++++++++++----------- 1 file changed, 15 insertions(+), 11 deletions(-) diff --git a/tests/e2e/compare/output/markdown.ts b/tests/e2e/compare/output/markdown.ts index 823be3f91fe1..4ddec22764f1 100644 --- a/tests/e2e/compare/output/markdown.ts +++ b/tests/e2e/compare/output/markdown.ts @@ -51,7 +51,7 @@ const formatEntryDuration = (entry: Entry): string => { const buildDetailsTable = (entries: Entry[], numberOfTables = 1) => { if (!entries.length) { - return ''; + return ['']; } const entriesPerTable = Math.floor(entries.length / numberOfTables); @@ -83,17 +83,22 @@ const buildSummaryTable = (entries: Entry[], collapse = false) => { return collapse ? collapsibleSection('Show entries', content) : content; }; -const buildMarkdown = (data: Data, skippedTests: string[], numberOfExtraFiles?: number) => { +const buildMarkdown = (data: Data, skippedTests: string[], numberOfExtraFiles?: number): [string, ...string[]] => { + let singleFileOutput: string | undefined; let nExtraFiles = numberOfExtraFiles ?? 0; + + // If the user didn't specify the number of extra files, calculate it based on the size of the single file if (numberOfExtraFiles === undefined) { - const singleMarkdown = buildMarkdown(data, skippedTests, 0).at(0); - const totalCharacters = singleMarkdown?.length ?? 0; + singleFileOutput = buildMarkdown(data, skippedTests, 0)[0]; + const totalCharacters = singleFileOutput.length ?? 0; - if (totalCharacters > MAX_CHARACTERS_PER_FILE_WITH_SAFETY_MARGIN) { - nExtraFiles = Math.ceil(totalCharacters / MAX_CHARACTERS_PER_FILE_WITH_SAFETY_MARGIN); - } else { - nExtraFiles = 0; + // If the single file is small enough, return it + if (totalCharacters <= MAX_CHARACTERS_PER_FILE_WITH_SAFETY_MARGIN) { + return [singleFileOutput]; } + + // Otherwise, calculate the number of extra files needed + nExtraFiles = Math.ceil(totalCharacters / MAX_CHARACTERS_PER_FILE_WITH_SAFETY_MARGIN); } let mainFile = '## Performance Comparison Report 📊'; @@ -140,7 +145,7 @@ const buildMarkdown = (data: Data, skippedTests: string[], numberOfExtraFiles?: extraFile += nExtraFiles > 0 ? ` (${i + 1}/${nExtraFiles + 1})` : ''; extraFile += `\n${buildSummaryTable(data.meaningless, true)}`; - extraFile += `\n${meaninglessDetailsTables[i]}`; + extraFile += `\n${meaninglessDetailsTables.at(i)}`; extraFile += '\n'; extraFiles.push(extraFile); @@ -169,8 +174,7 @@ const writeToMarkdown = (outputDir: string, data: Data, skippedTests: string[]) Logger.info('Markdown was built successfully, writing to file...', filesString); if (markdownFiles.length === 1) { - // eslint-disable-next-line @typescript-eslint/no-non-null-assertion - return writeToFile(path.join(outputDir, 'output1.md'), markdownFiles.at(0)!); + return writeToFile(path.join(outputDir, 'output1.md'), markdownFiles[0]); } return Promise.all( From d02cb574c12ec4297a7e640d4f3bb2e92ca37f90 Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Mon, 31 Mar 2025 09:02:21 +0200 Subject: [PATCH 15/18] Revert "Update E2EMarkdownTest.ts.snap" This reverts commit 8103ceecddf90192c860d8c3e496588e749bf450. --- .../__snapshots__/E2EMarkdownTest.ts.snap | 43 ++++++++++--------- 1 file changed, 22 insertions(+), 21 deletions(-) diff --git a/tests/unit/__snapshots__/E2EMarkdownTest.ts.snap b/tests/unit/__snapshots__/E2EMarkdownTest.ts.snap index 7abea3e085ef..9a66a0e2e823 100644 --- a/tests/unit/__snapshots__/E2EMarkdownTest.ts.snap +++ b/tests/unit/__snapshots__/E2EMarkdownTest.ts.snap @@ -1,23 +1,24 @@ // Jest Snapshot v1, https://goo.gl/fbAQLP -exports[`markdown formatter should format significant changes properly 1`] = [ - '## Performance Comparison Report 📊\n' + - '\n' + - '### Significant Changes To Duration\n' + - '| Name | Duration |\n' + - '| -------------- | --------------------------------------------------- |\n' + - '| commentLinking | 131.681 ms → 421.806 ms (+290.125 ms, +220.3%) 🔴🔴 |\n' + - '
\n' + - 'Show details\n' + - '\n' + - '| Name | Duration |\n' + - '| -------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ |\n' + - '| commentLinking | **Baseline**
Mean: 131.681 ms
Stdev: 19.883 ms (15.1%)
Runs: 100.5145680010319 112.0048420019448 121.8861090019345 124.26110899820924 135.1571460030973 140.33837900310755 158.5825610011816 160.7034499980509

**Current**
Mean: 421.806 ms
Stdev: 30.185 ms (7.2%)
Runs: 361.5145680010319 402.8861090019345 412.0048420019448 414.26110899820924 425.1571460030973 440.33837900310755 458.7034499980509 459.5825610011816 |\n' + - '
\n' + - '\n' + - '\n' + - '\n' + - '### Meaningless Changes To Duration\n' + - '_There are no entries_\n' + - 'undefined' -]; +exports[`markdown formatter should format significant changes properly 1`] = ` +"## Performance Comparison Report 📊 + +### Significant Changes To Duration +| Name | Duration | +| -------------- | --------------------------------------------------- | +| commentLinking | 131.681 ms → 421.806 ms (+290.125 ms, +220.3%) 🔴🔴 | +
+Show details + +| Name | Duration | +| -------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ | +| commentLinking | **Baseline**
Mean: 131.681 ms
Stdev: 19.883 ms (15.1%)
Runs: 100.5145680010319 112.0048420019448 121.8861090019345 124.26110899820924 135.1571460030973 140.33837900310755 158.5825610011816 160.7034499980509

**Current**
Mean: 421.806 ms
Stdev: 30.185 ms (7.2%)
Runs: 361.5145680010319 402.8861090019345 412.0048420019448 414.26110899820924 425.1571460030973 440.33837900310755 458.7034499980509 459.5825610011816 | +
+ + + +### Meaningless Changes To Duration +_There are no entries_ + +" +`; From 7c96cdcdb4efda25490236a8adbb4567744dd795 Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Mon, 31 Mar 2025 09:03:58 +0200 Subject: [PATCH 16/18] fix: e2e markdown test --- tests/unit/E2EMarkdownTest.ts | 3 ++- tests/unit/__snapshots__/E2EMarkdownTest.ts.snap | 1 - 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/unit/E2EMarkdownTest.ts b/tests/unit/E2EMarkdownTest.ts index 766ec708f31b..bbf33cc60a24 100644 --- a/tests/unit/E2EMarkdownTest.ts +++ b/tests/unit/E2EMarkdownTest.ts @@ -13,6 +13,7 @@ const results = { describe('markdown formatter', () => { it('should format significant changes properly', () => { const data = compareResults(results.main, results.delta, {commentLinking: 'ms'}); - expect(buildMarkdown(data, [])).toMatchSnapshot(); + const markdown = buildMarkdown(data, []).join('\nEOF\n\n'); + expect(markdown).toMatchSnapshot(); }); }); diff --git a/tests/unit/__snapshots__/E2EMarkdownTest.ts.snap b/tests/unit/__snapshots__/E2EMarkdownTest.ts.snap index 9a66a0e2e823..c82c0f9c045d 100644 --- a/tests/unit/__snapshots__/E2EMarkdownTest.ts.snap +++ b/tests/unit/__snapshots__/E2EMarkdownTest.ts.snap @@ -19,6 +19,5 @@ exports[`markdown formatter should format significant changes properly 1`] = ` ### Meaningless Changes To Duration _There are no entries_ - " `; From 66c66c6be9f9ae5a12c2bc73fdbf957ec3919b9c Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Mon, 31 Mar 2025 12:28:34 +0200 Subject: [PATCH 17/18] fix: wrong output filename in pipeline --- .github/workflows/e2ePerformanceTests.yml | 6 +++--- contributingGuides/REASSURE_PERFORMANCE_TEST.md | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/.github/workflows/e2ePerformanceTests.yml b/.github/workflows/e2ePerformanceTests.yml index 6ed783571067..6ad886e17993 100644 --- a/.github/workflows/e2ePerformanceTests.yml +++ b/.github/workflows/e2ePerformanceTests.yml @@ -215,7 +215,7 @@ jobs: - name: Print AWS Device Farm run results if: always() - run: cat "./Host_Machine_Files/\$WORKING_DIRECTORY/output.md" + run: cat "./Host_Machine_Files/\$WORKING_DIRECTORY/output1.md" - name: Check if test failed, if so post the results and add the DeployBlocker label id: checkIfRegressionDetected @@ -235,7 +235,7 @@ jobs: fi done else - gh pr comment ${{ inputs.PR_NUMBER }} -F "./Host_Machine_Files/\$WORKING_DIRECTORY/output.md" + gh pr comment ${{ inputs.PR_NUMBER }} -F "./Host_Machine_Files/\$WORKING_DIRECTORY/output1.md" fi gh pr comment ${{ inputs.PR_NUMBER }} -b "@Expensify/mobile-deployers 📣 Please look into this performance regression as it's a deploy blocker." @@ -249,7 +249,7 @@ jobs: - name: Check if test has skipped tests id: checkIfSkippedTestsDetected run: | - if grep -q '⚠️' "./Host_Machine_Files/\$WORKING_DIRECTORY/output.md"; then + if grep -q '⚠️' "./Host_Machine_Files/\$WORKING_DIRECTORY/output1.md"; then # Create an output to the GH action that the tests were skipped: echo "skippedTestsDetected=true" >> "$GITHUB_OUTPUT" else diff --git a/contributingGuides/REASSURE_PERFORMANCE_TEST.md b/contributingGuides/REASSURE_PERFORMANCE_TEST.md index 0e3f856819a9..a41d49517303 100644 --- a/contributingGuides/REASSURE_PERFORMANCE_TEST.md +++ b/contributingGuides/REASSURE_PERFORMANCE_TEST.md @@ -23,13 +23,13 @@ We use Reassure for monitoring performance regression. It helps us check if our - Identifying functions with heavy calculations. - Targeting functions that are frequently used throughout the app. -## Running tests locally +## Running tests locally - Checkout your base environment, eg. `git checkout main`. - Collect baseline metrics with `npm run perf-test -- --baseline`. - Apply any desired changes (for testing purposes you can eg. try to slow down a list). - Collect current metrics with `npm run perf-test`. -- Open up the resulting `output.md` / `output.json` (see console output) to compare the results. +- Open up the resulting `output1.md` (and possibly consecutive output files) / `output.json` (see console output) to compare the results. - With all that information, Reassure can present the render duration times as statistically significant or meaningless. ## Metrics for Regression Detection From fa2cb935fee45a2b8db3fe93f9a9e74a98927354 Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Mon, 31 Mar 2025 15:54:17 +0200 Subject: [PATCH 18/18] Update .github/workflows/checkE2ETestCode.yml Co-authored-by: Vit Horacek <36083550+mountiny@users.noreply.github.com> --- .github/workflows/checkE2ETestCode.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/.github/workflows/checkE2ETestCode.yml b/.github/workflows/checkE2ETestCode.yml index c8ac4c546f30..79fe28ac3118 100644 --- a/.github/workflows/checkE2ETestCode.yml +++ b/.github/workflows/checkE2ETestCode.yml @@ -23,3 +23,4 @@ jobs: - name: Verify e2e tests compile correctly run: npm run e2e-test-runner-build +