Skip to content

Commit 3c62f31

Browse files
authored
Merge pull request #58740 from margelo/@chrispader/split-up-e2e-performance-test-output-if-too-big
[NoQA] fix: Split up too big output files in E2E performance pipelines
2 parents 7edbad1 + fa2cb93 commit 3c62f31

File tree

8 files changed

+122
-41
lines changed

8 files changed

+122
-41
lines changed

.github/workflows/checkE2ETestCode.yml

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,9 @@ jobs:
1818

1919
- name: Setup Node
2020
uses: ./.github/actions/composite/setupNode
21+
with:
22+
IS_DESKTOP_BUILD: 'true'
2123

2224
- name: Verify e2e tests compile correctly
23-
run: npm run e2e-test-runner-build
25+
run: npm run e2e-test-runner-build
26+

.github/workflows/e2ePerformanceTests.yml

Lines changed: 16 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -215,17 +215,29 @@ jobs:
215215

216216
- name: Print AWS Device Farm run results
217217
if: always()
218-
run: cat "./Host_Machine_Files/\$WORKING_DIRECTORY/output.md"
218+
run: cat "./Host_Machine_Files/\$WORKING_DIRECTORY/output1.md"
219219

220220
- name: Check if test failed, if so post the results and add the DeployBlocker label
221221
id: checkIfRegressionDetected
222222
run: |
223-
if grep -q '🔴' "./Host_Machine_Files/\$WORKING_DIRECTORY/output.md"; then
223+
if grep -q '🔴' "./Host_Machine_Files/\$WORKING_DIRECTORY/output1.md"; then
224224
# Create an output to the GH action that the test failed:
225225
echo "performanceRegressionDetected=true" >> "$GITHUB_OUTPUT"
226226
227227
gh pr edit ${{ inputs.PR_NUMBER }} --add-label DeployBlockerCash
228-
gh pr comment ${{ inputs.PR_NUMBER }} -F "./Host_Machine_Files/\$WORKING_DIRECTORY/output.md"
228+
229+
# Check if there are any split files
230+
if ls "./Host_Machine_Files/\$WORKING_DIRECTORY"/output2.md 1> /dev/null 2>&1; then
231+
# Post each split file as a separate comment
232+
for file in "./Host_Machine_Files/\$WORKING_DIRECTORY/output"*; do
233+
if [ -f "$file" ]; then
234+
gh pr comment ${{ inputs.PR_NUMBER }} -F "$file"
235+
fi
236+
done
237+
else
238+
gh pr comment ${{ inputs.PR_NUMBER }} -F "./Host_Machine_Files/\$WORKING_DIRECTORY/output1.md"
239+
fi
240+
229241
gh pr comment ${{ inputs.PR_NUMBER }} -b "@Expensify/mobile-deployers 📣 Please look into this performance regression as it's a deploy blocker."
230242
else
231243
echo "performanceRegressionDetected=false" >> "$GITHUB_OUTPUT"
@@ -237,7 +249,7 @@ jobs:
237249
- name: Check if test has skipped tests
238250
id: checkIfSkippedTestsDetected
239251
run: |
240-
if grep -q '⚠️' "./Host_Machine_Files/\$WORKING_DIRECTORY/output.md"; then
252+
if grep -q '⚠️' "./Host_Machine_Files/\$WORKING_DIRECTORY/output1.md"; then
241253
# Create an output to the GH action that the tests were skipped:
242254
echo "skippedTestsDetected=true" >> "$GITHUB_OUTPUT"
243255
else

contributingGuides/REASSURE_PERFORMANCE_TEST.md

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -23,13 +23,13 @@ We use Reassure for monitoring performance regression. It helps us check if our
2323
- Identifying functions with heavy calculations.
2424
- Targeting functions that are frequently used throughout the app.
2525

26-
## Running tests locally
26+
## Running tests locally
2727

2828
- Checkout your base environment, eg. `git checkout main`.
2929
- Collect baseline metrics with `npm run perf-test -- --baseline`.
3030
- Apply any desired changes (for testing purposes you can eg. try to slow down a list).
3131
- Collect current metrics with `npm run perf-test`.
32-
- Open up the resulting `output.md` / `output.json` (see console output) to compare the results.
32+
- Open up the resulting `output1.md` (and possibly consecutive output files) / `output.json` (see console output) to compare the results.
3333
- With all that information, Reassure can present the render duration times as statistically significant or meaningless.
3434

3535
## Metrics for Regression Detection

tests/e2e/compare/compare.ts

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -92,13 +92,13 @@ function compareResults(baselineEntries: Metric | string, compareEntries: Metric
9292
}
9393

9494
type Options = {
95-
outputFile: string;
95+
outputDir: string;
9696
outputFormat: 'console' | 'markdown' | 'all';
9797
metricForTest: Record<string, Unit>;
9898
skippedTests: string[];
9999
};
100100

101-
export default (main: Metric | string, delta: Metric | string, {outputFile, outputFormat = 'all', metricForTest = {}, skippedTests}: Options) => {
101+
export default (main: Metric | string, delta: Metric | string, {outputDir, outputFormat = 'all', metricForTest = {}, skippedTests}: Options) => {
102102
// IMPORTANT NOTE: make sure you are passing the main/baseline results first, then the delta/compare results:
103103
const outputData = compareResults(main, delta, metricForTest);
104104

@@ -107,7 +107,7 @@ export default (main: Metric | string, delta: Metric | string, {outputFile, outp
107107
}
108108

109109
if (outputFormat === 'markdown' || outputFormat === 'all') {
110-
return writeToMarkdown(outputFile, outputData, skippedTests);
110+
return writeToMarkdown(outputDir, outputData, skippedTests);
111111
}
112112
};
113113
export {compareResults};

tests/e2e/compare/output/markdown.ts

Lines changed: 94 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,10 @@ import type {Data, Entry} from './console';
77
import * as format from './format';
88
import markdownTable from './markdownTable';
99

10+
const MAX_CHARACTERS_PER_FILE = 65536;
11+
const FILE_SIZE_SAFETY_MARGIN = 1000;
12+
const MAX_CHARACTERS_PER_FILE_WITH_SAFETY_MARGIN = MAX_CHARACTERS_PER_FILE - FILE_SIZE_SAFETY_MARGIN;
13+
1014
const tableHeader = ['Name', 'Duration'];
1115

1216
const collapsibleSection = (title: string, content: string) => `<details>\n<summary>${title}</summary>\n\n${content}\n</details>\n\n`;
@@ -45,15 +49,27 @@ const formatEntryDuration = (entry: Entry): string => {
4549
return '';
4650
};
4751

48-
const buildDetailsTable = (entries: Entry[]) => {
52+
const buildDetailsTable = (entries: Entry[], numberOfTables = 1) => {
4953
if (!entries.length) {
50-
return '';
54+
return [''];
5155
}
5256

53-
const rows = entries.map((entry) => [entry.name, buildDurationDetailsEntry(entry)]);
54-
const content = markdownTable([tableHeader, ...rows]);
57+
const entriesPerTable = Math.floor(entries.length / numberOfTables);
58+
const tables: string[] = [];
59+
for (let i = 0; i < numberOfTables; i++) {
60+
const start = i * entriesPerTable;
61+
const end = i === numberOfTables - 1 ? entries.length : start + entriesPerTable;
62+
const tableEntries = entries.slice(start, end);
63+
64+
const rows = tableEntries.map((entry) => [entry.name, buildDurationDetailsEntry(entry)]);
65+
const content = markdownTable([tableHeader, ...rows]);
66+
67+
const tableMarkdown = collapsibleSection('Show details', content);
5568

56-
return collapsibleSection('Show details', content);
69+
tables.push(tableMarkdown);
70+
}
71+
72+
return tables;
5773
};
5874

5975
const buildSummaryTable = (entries: Entry[], collapse = false) => {
@@ -67,36 +83,75 @@ const buildSummaryTable = (entries: Entry[], collapse = false) => {
6783
return collapse ? collapsibleSection('Show entries', content) : content;
6884
};
6985

70-
const buildMarkdown = (data: Data, skippedTests: string[]) => {
71-
let result = '## Performance Comparison Report 📊';
86+
const buildMarkdown = (data: Data, skippedTests: string[], numberOfExtraFiles?: number): [string, ...string[]] => {
87+
let singleFileOutput: string | undefined;
88+
let nExtraFiles = numberOfExtraFiles ?? 0;
89+
90+
// If the user didn't specify the number of extra files, calculate it based on the size of the single file
91+
if (numberOfExtraFiles === undefined) {
92+
singleFileOutput = buildMarkdown(data, skippedTests, 0)[0];
93+
const totalCharacters = singleFileOutput.length ?? 0;
94+
95+
// If the single file is small enough, return it
96+
if (totalCharacters <= MAX_CHARACTERS_PER_FILE_WITH_SAFETY_MARGIN) {
97+
return [singleFileOutput];
98+
}
99+
100+
// Otherwise, calculate the number of extra files needed
101+
nExtraFiles = Math.ceil(totalCharacters / MAX_CHARACTERS_PER_FILE_WITH_SAFETY_MARGIN);
102+
}
103+
104+
let mainFile = '## Performance Comparison Report 📊';
105+
mainFile += nExtraFiles > 0 ? ` (1/${nExtraFiles + 1})` : '';
72106

73107
if (data.errors?.length) {
74-
result += '\n\n### Errors\n';
108+
mainFile += '\n\n### Errors\n';
75109
data.errors.forEach((message) => {
76-
result += ` 1. 🛑 ${message}\n`;
110+
mainFile += ` 1. 🛑 ${message}\n`;
77111
});
78112
}
79113

80114
if (data.warnings?.length) {
81-
result += '\n\n### Warnings\n';
115+
mainFile += '\n\n### Warnings\n';
82116
data.warnings.forEach((message) => {
83-
result += ` 1. 🟡 ${message}\n`;
117+
mainFile += ` 1. 🟡 ${message}\n`;
84118
});
85119
}
86120

87-
result += '\n\n### Significant Changes To Duration';
88-
result += `\n${buildSummaryTable(data.significance)}`;
89-
result += `\n${buildDetailsTable(data.significance)}`;
90-
result += '\n\n### Meaningless Changes To Duration';
91-
result += `\n${buildSummaryTable(data.meaningless, true)}`;
92-
result += `\n${buildDetailsTable(data.meaningless)}`;
93-
result += '\n';
94-
95121
if (skippedTests.length > 0) {
96-
result += `⚠️ Some tests did not pass successfully, so some results are omitted from final report: ${skippedTests.join(', ')}`;
122+
mainFile += `⚠️ Some tests did not pass successfully, so some results are omitted from final report: ${skippedTests.join(', ')}`;
97123
}
98124

99-
return result;
125+
mainFile += '\n\n### Significant Changes To Duration';
126+
mainFile += `\n${buildSummaryTable(data.significance)}`;
127+
mainFile += `\n${buildDetailsTable(data.significance, 1).at(0)}`;
128+
129+
const meaninglessDetailsTables = buildDetailsTable(data.meaningless, nExtraFiles);
130+
131+
if (nExtraFiles === 0) {
132+
mainFile += '\n\n### Meaningless Changes To Duration';
133+
mainFile += `\n${buildSummaryTable(data.meaningless, true)}`;
134+
mainFile += `\n${meaninglessDetailsTables.at(0)}`;
135+
136+
return [mainFile];
137+
}
138+
139+
const extraFiles: string[] = [];
140+
for (let i = 0; i < nExtraFiles; i++) {
141+
let extraFile = '## Performance Comparison Report 📊';
142+
extraFile += nExtraFiles > 0 ? ` (${i + 2}/${nExtraFiles + 1})` : '';
143+
144+
extraFile += '\n\n### Meaningless Changes To Duration';
145+
extraFile += nExtraFiles > 0 ? ` (${i + 1}/${nExtraFiles + 1})` : '';
146+
147+
extraFile += `\n${buildSummaryTable(data.meaningless, true)}`;
148+
extraFile += `\n${meaninglessDetailsTables.at(i)}`;
149+
extraFile += '\n';
150+
151+
extraFiles.push(extraFile);
152+
}
153+
154+
return [mainFile, ...extraFiles];
100155
};
101156

102157
const writeToFile = (filePath: string, content: string) =>
@@ -113,13 +168,24 @@ const writeToFile = (filePath: string, content: string) =>
113168
throw error;
114169
});
115170

116-
const writeToMarkdown = (filePath: string, data: Data, skippedTests: string[]) => {
117-
const markdown = buildMarkdown(data, skippedTests);
118-
Logger.info('Markdown was built successfully, writing to file...', markdown);
119-
return writeToFile(filePath, markdown).catch((error) => {
120-
console.error(error);
121-
throw error;
122-
});
171+
const writeToMarkdown = (outputDir: string, data: Data, skippedTests: string[]) => {
172+
const markdownFiles = buildMarkdown(data, skippedTests);
173+
const filesString = markdownFiles.join('\n\n');
174+
Logger.info('Markdown was built successfully, writing to file...', filesString);
175+
176+
if (markdownFiles.length === 1) {
177+
return writeToFile(path.join(outputDir, 'output1.md'), markdownFiles[0]);
178+
}
179+
180+
return Promise.all(
181+
markdownFiles.map((file, index) => {
182+
const filePath = `${outputDir}/output-${index + 1}.md`;
183+
return writeToFile(filePath, file).catch((error) => {
184+
console.error(error);
185+
throw error;
186+
});
187+
}),
188+
);
123189
};
124190

125191
export default writeToMarkdown;

tests/e2e/testRunner.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -357,7 +357,7 @@ const runTests = async (): Promise<void> => {
357357
// Calculate statistics and write them to our work file
358358
Logger.info('Calculating statics and writing results');
359359
await compare(results.main, results.delta, {
360-
outputFile: `${config.OUTPUT_DIR}/output.md`,
360+
outputDir: config.OUTPUT_DIR,
361361
outputFormat: 'all',
362362
metricForTest,
363363
skippedTests,

tests/unit/E2EMarkdownTest.ts

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ const results = {
1313
describe('markdown formatter', () => {
1414
it('should format significant changes properly', () => {
1515
const data = compareResults(results.main, results.delta, {commentLinking: 'ms'});
16-
expect(buildMarkdown(data, [])).toMatchSnapshot();
16+
const markdown = buildMarkdown(data, []).join('\nEOF\n\n');
17+
expect(markdown).toMatchSnapshot();
1718
});
1819
});

tests/unit/__snapshots__/E2EMarkdownTest.ts.snap

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,5 @@ exports[`markdown formatter should format significant changes properly 1`] = `
1919
2020
### Meaningless Changes To Duration
2121
_There are no entries_
22-
2322
"
2423
`;

0 commit comments

Comments
 (0)