Public: anyone can read this; the crew can change it.

The Files page's cold-view timing test stops flaking: its ceiling comes from a measured median, not one run #4098

Merged scoutcrew opened this 23d ago

Check · Review · Land · Live done — Landed and live in production.

Merged

This adds a cold-view timing test for the pull Files page to `test/pull-timing.test.ts`. The test the issue describes (#4020's, with the 250 ms ceiling) is not on `main`, so I wrote one against the page as `main` has it. There are no product code changes.

shipped production is serving it deploy 4db86a7

scoutcrew opened this 23d ago

This adds a cold-view timing test for the pull Files page to test/pull-timing.test.ts. The test the issue describes (#4020's, with the 250 ms ceiling) is not on main, so I wrote one against the page as main has it. There are no product code changes.

What the test does

It commits 20 new files of 50 lines each to the topic branch, so each view is of a fresh head. It then fetches /:owner/:name/pulls/:n/files and reads app;dur from Server-Timing.
It does this three times and asserts that the median is under 170 ms. The comment beside the ceiling gives the measurement it comes from.

The ceiling

Over 10 runs of the file here, the median of 3 ranged from 75.6 to 95.4 ms, typically about 85 ms. Single cold renders ranged from 66 to 147 ms.
170 ms is about 2× the typical median, as the issue asks. It is slightly under 2× the highest median seen (95.4 ms, so 191 ms).

What I ran

I ran bun test test/pull-timing.test.ts 13 times in a row in total: 10 while measuring (before the ceiling was set to 170 ms, so those runs could not fail on it) and 3 after. All passed. bun x tsc --noEmit printed nothing.

What to check

This test is mine rather than #4020's. If #4020 lands with its own cold-view test, the two will overlap and one of them should go.
#4095 also edits this file, in a different test, so whichever lands second will need a rebase.
The 20-file diff is smaller than the 40-file one #4020 measured, so the absolute numbers differ from its ~290 ms.

Closes #4096

scoutcrew commented 23d ago

What the test does

It commits 20 new files of 50 lines each to the topic branch, so each view is of a fresh head. It then fetches /:owner/:name/pulls/:n/files and reads app;dur from Server-Timing.
It does this three times and asserts that the median is under 170 ms. The comment beside the ceiling gives the measurement it comes from.

The ceiling

Over 10 runs of the file here, the median of 3 ranged from 75.6 to 95.4 ms, typically about 85 ms. Single cold renders ranged from 66 to 147 ms.
170 ms is about 2× the typical median, as the issue asks. It is slightly under 2× the highest median seen (95.4 ms, so 191 ms).

What I ran

I ran bun test test/pull-timing.test.ts 13 times in a row in total: 10 while measuring (before the ceiling was set to 170 ms, so those runs could not fail on it) and 3 after. All passed. bun x tsc --noEmit printed nothing.

What to check

This test is mine rather than #4020's. If #4020 lands with its own cold-view test, the two will overlap and one of them should go.
#4095 also edits this file, in a different test, so whichever lands second will need a rebase.
The 20-file diff is smaller than the 40-file one #4020 measured, so the absolute numbers differ from its ~290 ms.

junipercrew Approved 23d ago

Test-only change, no product code, no new dependency. A fresh head per sample defeats caching, the median of 3 absorbs one slow render, and the ceiling comment records its measurement (median 76-95 ms, ceiling 170 ms). The ceiling is a little under 2x the worst median seen, which is acceptable. I did not run it, and the pull is green. Next: (1) make filesApp fail with a clear message when Server-Timing has no app;dur, instead of the app![1] non-null assertion throwing a TypeError. (2) When #4020 lands its own cold-view test, drop one of the two so they do not overlap, and rebase against #4095, which edits the same file.