diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 91b9d0238..6f1716886 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -158,10 +158,13 @@ jobs: # branch for a pull request, the previous head for a master push, # master for a manual dispatch. Any raised limit, widened tolerance # or slack, or removed entry fails, unless a maintainer put the - # perf-baseline-increase label on the pull request (for a master - # push: on the pull request merged as the pushed commit). Labels - # are read from the API, not the event payload, so re-running this - # job after adding the label picks it up. + # perf-baseline-increase label on the pull request. For a master + # push the label counts only when the push added exactly one + # first-parent commit (a squash or merge of one PR) and that + # commit's PR carries it: the check compares the whole push, so a + # multi-commit push cannot borrow one PR's label for another's + # increase. Labels are read from the API, not the event payload, + # so re-running this job after adding the label picks it up. - name: Refuse baseline increases against the previous revision env: EVENT_NAME: ${{ github.event_name }} @@ -196,7 +199,13 @@ jobs: labels="$(gh api "repos/$REPOSITORY/issues/$PR_NUMBER/labels" --jq '.[].name')" ;; push) - labels="$(gh api "repos/$REPOSITORY/commits/$HEAD_SHA/pulls" --jq '.[].labels[].name')" + parent="$(gh api "repos/$REPOSITORY/commits/$HEAD_SHA" --jq '.parents[0].sha')" + if [ "$parent" = "$BEFORE_SHA" ]; then + labels="$(gh api "repos/$REPOSITORY/commits/$HEAD_SHA/pulls" --jq '.[].labels[].name')" + else + echo "This push added more than one commit; the $INCREASE_LABEL label is not consulted." + labels="" + fi ;; *) labels="" diff --git a/docs/architecture/performance-journeys.md b/docs/architecture/performance-journeys.md index b411d40ba..fbc445d14 100644 --- a/docs/architecture/performance-journeys.md +++ b/docs/architecture/performance-journeys.md @@ -358,7 +358,9 @@ The job also refuses a weakened baselines file: `master` for a manual dispatch) and fails when any entry's enforced limit (`value × toleranceRatio` or `value + slack`) went up, a tolerance or slack widened or an entry disappeared, so a PR cannot grow the -payload and raise the baseline to match. Lowered limits and new entries pass. +payload and raise the baseline to match. A counter's `value` may not go up +either, even when narrower slack lowers its limit. Lowered limits and new +entries pass. Baselines only move down. Lower `value` in the same PR as the change that earned it, set `updatedAt` and `evidencePr`, and paste the measurement output @@ -369,8 +371,10 @@ per-file breakdown, and ask a maintainer to add the `perf-baseline-increase` label. With the label the direction check prints the weakened entries as `ALLOWED` and passes; the job reads labels from the API when it runs, so re-run the job after the label is added. For a `master` push the label is -read from the pull request merged as the pushed commit, so a squash merge of -a labelled PR passes and a direct push that raises a baseline still fails. +read from the pull request merged as the pushed commit, and only when the +push added exactly one first-parent commit: a squash or merge of a labelled +PR passes, while a direct push, or a push of several commits (which the check +compares as a whole), that raises a baseline still fails. Only people with triage access can set labels, so the label is the maintainer decision. diff --git a/tools/performance/check-baseline-direction.mjs b/tools/performance/check-baseline-direction.mjs index e17b150a2..b0b8b9949 100644 --- a/tools/performance/check-baseline-direction.mjs +++ b/tools/performance/check-baseline-direction.mjs @@ -7,7 +7,9 @@ * check closes that gap: given the baselines file of the target branch and * the one of the PR, any entry whose enforced limit (`value × toleranceRatio` * or `value + slack`) went up, whose tolerance or slack widened, or that - * disappeared, is a failure. New entries and lowered limits pass. + * disappeared, is a failure. A counter's `value` may not go up either, even + * when narrower slack lowers its limit: the value is the measured evidence. + * New entries and lowered limits pass. * * `--allow-increase` turns those failures into printed "allowed" lines. CI * passes it only when a maintainer put the perf-baseline-increase label on @@ -97,6 +99,13 @@ export function compareBaselineDirection({ weakened.push( `${label}: baseline raised from ${formatNumber(baseLimit)} to ${formatNumber(headLimit)}${unit}. Baselines only move down; bring the measurement back under ${formatNumber(baseLimit)}, or make the case for the increase in the PR and ask a maintainer to add the ${BASELINE_INCREASE_LABEL} label.` ); + } else if ( + headEntry.toleranceRatio === undefined && + headEntry.value > baseEntry.value + ) { + weakened.push( + `${label}: baseline value raised from ${formatNumber(baseEntry.value)} to ${formatNumber(headEntry.value)}${unit} while slack narrowed from ${formatNumber(baseSlack)} to ${formatNumber(headSlack)}. A counter's value only moves down; narrowing its slack does not offset a raise. Ask a maintainer to add the ${BASELINE_INCREASE_LABEL} label if the raise is deliberate.` + ); } else if (headLimit < baseLimit) { result.lowered.push( `${label}: ${formatNumber(baseLimit)} -> ${formatNumber(headLimit)}${unit}.` diff --git a/tools/performance/check-baseline-direction.test.mjs b/tools/performance/check-baseline-direction.test.mjs index 87b3ba9e6..54b1346a2 100644 --- a/tools/performance/check-baseline-direction.test.mjs +++ b/tools/performance/check-baseline-direction.test.mjs @@ -184,6 +184,35 @@ test('a widened or newly added slack fails; a narrowed one lowers the limit', () assert.match(raised.failures[0], /perf-baseline-increase label/); }); +test('a counter value raised behind narrower slack still fails', () => { + const withSlack = (value, slack) => ({ + version: 1, + journeys: { + launch: { + 'renderer.initialBytes': { value, unit: 'bytes', slack }, + }, + }, + }); + const result = compareBaselineDirection({ + base: withSlack(100, 10), + head: withSlack(105, 0), + }); + assert.equal(result.failures.length, 1); + assert.deepEqual(result.lowered, []); + assert.match( + result.failures[0], + /renderer\.initialBytes: baseline value raised from 100 to 105 bytes while slack narrowed from 10 to 0/ + ); + + const allowed = compareBaselineDirection({ + base: withSlack(100, 10), + head: withSlack(105, 0), + allowIncrease: true, + }); + assert.deepEqual(allowed.failures, []); + assert.equal(allowed.allowed.length, 1); +}); + test('allowIncrease reports every weakening as allowed instead of failing', () => { const result = compareBaselineDirection({ base: file(100, { cdTicks: { value: 5 } }),