Skip to content

Commit 88fa085

Browse files
ya-nshya-nsh
committed
fix: harden performance budget validation
1 parent 13b7a8e commit 88fa085

3 files changed

Lines changed: 261 additions & 18 deletions

File tree

‎packages/react-native/scripts/performance/__tests__/performance-budget-cli-test.js‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -44,15 +44,15 @@ describe('performance-budget-cli', () => {
4444
{
4545
flow: 'cold-start-home',
4646
message:
47-
'cold-start-home startupTimeMs has no reported p75 value',
47+
'cold-start-home startupTimeMs p75 has no baseline value for regression comparison',
4848
},
4949
],
5050
}),
5151
).toEqual([
5252
'Performance budget passed',
5353
'',
5454
'Warnings:',
55-
'- cold-start-home startupTimeMs has no reported p75 value',
55+
'- cold-start-home startupTimeMs p75 has no baseline value for regression comparison',
5656
]);
5757
});
5858

‎packages/react-native/scripts/performance/__tests__/performance-budget-test.js‎

Lines changed: 154 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -175,7 +175,7 @@ describe('checkPerformanceBudget', () => {
175175
]);
176176
});
177177

178-
it('warns when budgeted data is missing from reports', () => {
178+
it('fails when budgeted data is missing from reports', () => {
179179
const result = checkPerformanceBudget(
180180
{
181181
flows: {
@@ -202,20 +202,170 @@ describe('checkPerformanceBudget', () => {
202202
);
203203

204204
expect(result).toEqual({
205-
ok: true,
206-
failures: [],
207-
warnings: [
205+
ok: false,
206+
failures: [
208207
{
208+
type: 'missing',
209209
flow: 'cold-start-home',
210210
metric: 'startupTimeMs',
211211
stat: 'p75',
212212
message: 'cold-start-home startupTimeMs has no reported p75 value',
213213
},
214214
{
215+
type: 'missing',
215216
flow: 'open-search-screen',
216217
message: 'open-search-screen has no performance report',
217218
},
218219
],
220+
warnings: [],
221+
});
222+
});
223+
224+
it('fails when duplicate flow reports would make input ambiguous', () => {
225+
const result = checkPerformanceBudget(
226+
{
227+
flows: {
228+
'cold-start-home': {
229+
startupTimeMs: {
230+
p75: 2200,
231+
},
232+
},
233+
},
234+
},
235+
[
236+
{
237+
flow: 'cold-start-home',
238+
metrics: {
239+
startupTimeMs: {
240+
p75: 2460,
241+
},
242+
},
243+
},
244+
{
245+
flow: 'cold-start-home',
246+
metrics: {
247+
startupTimeMs: {
248+
p75: 2100,
249+
},
250+
},
251+
},
252+
],
253+
);
254+
255+
expect(result.ok).toBe(false);
256+
expect(result.failures).toEqual([
257+
{
258+
type: 'duplicate_report',
259+
flow: 'cold-start-home',
260+
reportKind: 'current',
261+
firstReportIndex: 0,
262+
duplicateReportIndex: 1,
263+
message:
264+
'current performance reports contain duplicate flow cold-start-home at indexes 0 and 1',
265+
},
266+
{
267+
type: 'absolute',
268+
flow: 'cold-start-home',
269+
metric: 'startupTimeMs',
270+
stat: 'p75',
271+
actual: 2460,
272+
budget: 2200,
273+
message:
274+
'cold-start-home startupTimeMs p75 is 2460, exceeding budget 2200',
275+
},
276+
]);
277+
});
278+
279+
it('fails when duplicate baseline reports would make regression comparison ambiguous', () => {
280+
const result = checkPerformanceBudget(
281+
{
282+
flows: {
283+
'cold-start-home': {
284+
startupTimeMs: {
285+
p75: 2200,
286+
maxRegressionPct: 10,
287+
},
288+
},
289+
},
290+
},
291+
[
292+
{
293+
flow: 'cold-start-home',
294+
metrics: {
295+
startupTimeMs: {
296+
p75: 1080,
297+
},
298+
},
299+
},
300+
],
301+
[
302+
{
303+
flow: 'cold-start-home',
304+
metrics: {
305+
startupTimeMs: {
306+
p75: 1000,
307+
},
308+
},
309+
},
310+
{
311+
flow: 'cold-start-home',
312+
metrics: {
313+
startupTimeMs: {
314+
p75: 500,
315+
},
316+
},
317+
},
318+
],
319+
);
320+
321+
expect(result.ok).toBe(false);
322+
expect(result.failures).toEqual([
323+
{
324+
type: 'duplicate_report',
325+
flow: 'cold-start-home',
326+
reportKind: 'baseline',
327+
firstReportIndex: 0,
328+
duplicateReportIndex: 1,
329+
message:
330+
'baseline performance reports contain duplicate flow cold-start-home at indexes 0 and 1',
331+
},
332+
]);
333+
});
334+
335+
it('fails with structured output for invalid report shapes', () => {
336+
const result = checkPerformanceBudget(
337+
{
338+
flows: {
339+
'cold-start-home': {
340+
startupTimeMs: {
341+
p75: 2200,
342+
},
343+
},
344+
},
345+
},
346+
[
347+
{
348+
flow: 'cold-start-home',
349+
},
350+
],
351+
);
352+
353+
expect(result).toEqual({
354+
ok: false,
355+
failures: [
356+
{
357+
type: 'invalid_report',
358+
reportKind: 'current',
359+
reportIndex: 0,
360+
message: 'current performance report at index 0 must include metrics',
361+
},
362+
{
363+
type: 'missing',
364+
flow: 'cold-start-home',
365+
message: 'cold-start-home has no performance report',
366+
},
367+
],
368+
warnings: [],
219369
});
220370
});
221371

‎packages/react-native/scripts/performance/performance-budget.js‎

Lines changed: 105 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -61,7 +61,38 @@ type RegressionFailure = $ReadOnly<{
6161
+message: string,
6262
}>;
6363
64-
type Failure = AbsoluteFailure | RegressionFailure;
64+
type MissingFailure = $ReadOnly<{
65+
+type: 'missing',
66+
+flow: string,
67+
+metric?: string,
68+
+stat?: string,
69+
+message: string,
70+
}>;
71+
72+
type ReportKind = 'current' | 'baseline';
73+
74+
type InvalidReportFailure = $ReadOnly<{
75+
+type: 'invalid_report',
76+
+reportKind: ReportKind,
77+
+reportIndex: number,
78+
+message: string,
79+
}>;
80+
81+
type DuplicateReportFailure = $ReadOnly<{
82+
+type: 'duplicate_report',
83+
+reportKind: ReportKind,
84+
+flow: string,
85+
+firstReportIndex: number,
86+
+duplicateReportIndex: number,
87+
+message: string,
88+
}>;
89+
90+
type Failure =
91+
| AbsoluteFailure
92+
| RegressionFailure
93+
| MissingFailure
94+
| InvalidReportFailure
95+
| DuplicateReportFailure;
6596
6697
type Warning = $ReadOnly<{
6798
+flow: string,
@@ -77,30 +108,90 @@ type BudgetResult = $ReadOnly<{
77108
}>;
78109
*/
79110

111+
function isObject(value /*: mixed */) /*: boolean */ {
112+
return typeof value === 'object' && value != null && !Array.isArray(value);
113+
}
114+
80115
function reportByFlow(
81-
reports /*: $ReadOnlyArray<PerformanceReport> */,
116+
reports /*: $ReadOnlyArray<mixed> */,
117+
reportKind /*: ReportKind */,
118+
failures /*: Array<Failure> */,
82119
) /*: Map<string, PerformanceReport> */ {
83120
const byFlow = new Map /*:: <string, PerformanceReport> */();
84-
for (const report of reports) {
85-
byFlow.set(report.flow, report);
86-
}
121+
const reportIndexes = new Map /*:: <string, number> */();
122+
123+
reports.forEach((reportValue, reportIndex) => {
124+
if (!isObject(reportValue)) {
125+
failures.push({
126+
type: 'invalid_report',
127+
reportKind,
128+
reportIndex,
129+
message: `${reportKind} performance report at index ${reportIndex} must be an object`,
130+
});
131+
return;
132+
}
133+
134+
// $FlowFixMe[incompatible-type] Runtime validation above confirms this is an object.
135+
const report /*: {[string]: mixed} */ = reportValue;
136+
const flow = report.flow;
137+
if (typeof flow !== 'string' || flow === '') {
138+
failures.push({
139+
type: 'invalid_report',
140+
reportKind,
141+
reportIndex,
142+
message: `${reportKind} performance report at index ${reportIndex} must include flow`,
143+
});
144+
return;
145+
}
146+
147+
const metrics = report.metrics;
148+
if (!isObject(metrics)) {
149+
failures.push({
150+
type: 'invalid_report',
151+
reportKind,
152+
reportIndex,
153+
message: `${reportKind} performance report at index ${reportIndex} must include metrics`,
154+
});
155+
return;
156+
}
157+
158+
const firstReportIndex = reportIndexes.get(flow);
159+
if (firstReportIndex != null) {
160+
failures.push({
161+
type: 'duplicate_report',
162+
reportKind,
163+
flow,
164+
firstReportIndex,
165+
duplicateReportIndex: reportIndex,
166+
message: `${reportKind} performance reports contain duplicate flow ${flow} at indexes ${firstReportIndex} and ${reportIndex}`,
167+
});
168+
return;
169+
}
170+
171+
// $FlowFixMe[incompatible-type] Runtime validation confirms the report contract used below.
172+
const validatedReport /*: PerformanceReport */ = {flow, metrics};
173+
reportIndexes.set(flow, reportIndex);
174+
byFlow.set(flow, validatedReport);
175+
});
176+
87177
return byFlow;
88178
}
89179

90180
function checkPerformanceBudget(
91181
budget /*: PerformanceBudget */,
92-
reports /*: $ReadOnlyArray<PerformanceReport> */,
93-
baselineReports /*: $ReadOnlyArray<PerformanceReport> */ = [],
182+
reports /*: $ReadOnlyArray<mixed> */,
183+
baselineReports /*: $ReadOnlyArray<mixed> */ = [],
94184
) /*: BudgetResult */ {
95185
const failures /*: Array<Failure> */ = [];
96186
const warnings /*: Array<Warning> */ = [];
97-
const reportsByFlow = reportByFlow(reports);
98-
const baselinesByFlow = reportByFlow(baselineReports);
187+
const reportsByFlow = reportByFlow(reports, 'current', failures);
188+
const baselinesByFlow = reportByFlow(baselineReports, 'baseline', failures);
99189

100190
for (const flow of Object.keys(budget.flows)) {
101191
const report = reportsByFlow.get(flow);
102192
if (report == null) {
103-
warnings.push({
193+
failures.push({
194+
type: 'missing',
104195
flow,
105196
message: `${flow} has no performance report`,
106197
});
@@ -111,7 +202,8 @@ function checkPerformanceBudget(
111202
for (const metric of Object.keys(budgetedMetrics)) {
112203
const metricStats = report.metrics[metric];
113204
if (metricStats == null) {
114-
warnings.push({
205+
failures.push({
206+
type: 'missing',
115207
flow,
116208
metric,
117209
message: `${flow} ${metric} has no reported metrics`,
@@ -128,7 +220,8 @@ function checkPerformanceBudget(
128220
const budgetValue = budgetedStats[stat];
129221
const actual = metricStats[stat];
130222
if (actual == null) {
131-
warnings.push({
223+
failures.push({
224+
type: 'missing',
132225
flow,
133226
metric,
134227
stat,

0 commit comments

Comments
 (0)