Repository navigation
CodeVitals: isolate per-scenario measurement failures from posting - #50453
Merged
LiamSarsfield merged 6 commits intoJul 14, 2026
Merged
LiamSarsfield merged 6 commits into
LiamSarsfield merged 6 commits into
Conversation
One broken scenario no longer blanks every trend. Scenarios declare an optional flag; measure-lcp.js exits 0 when every required scenario measured (the posting policy lives once, in that exit code), so the runner flows to posting and the poster skips the errored measurements. A required-scenario failure still exits 1 before the posting step, keeping retries duplicate-free on the append-only store. An unknown SCENARIO filter now fails fast instead of green-exiting with zero measurements, and a green build with skipped scenarios emits a TeamCity WARNING service message.
Contributor
|
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 Follow this PR Review Process:
If you have questions about anything, reach out in #jetpack-developers for guidance! |
github-actions
Bot
added
the
[Status] Needs Author Reply
… contract, test coverage
The poster now fails closed (exit 2) on a results file recording a required
scenario's measurement failure, so the direct 'pnpm report' entrypoint cannot
post a red run's optional survivors and set up retry duplicates. Scenarios
absent from a SCENARIO-filtered artifact still skip normally, keeping targeted
optional runs posting.
The isolation contract is scoped in docs where it was overclaiming: the
optional flag isolates measurement failures; a measured-but-out-of-range value
still trips the pre-existing atomic sanity gate on purpose (data integrity
beats isolation), now a row in the README truth table.
reportSkippedScenarios derives its names from computeRunOutcome (one tested
classifier), is exported, and gains unit tests including the exact TeamCity
message; a CLI test pins the SCENARIO-typo exit path; the scenario contract
test pins the exact policy map and key/cliName uniqueness; the scenario catch
guarantees a truthy error record so Error('') cannot green a required failure.
…ew round 2) A scenario whose summary drops a posted field (the strict-majority rule in summarizeField) previously greened the measure step, evaded every optional-failure warning, and then tripped the poster's atomic sanity gate on the undefined median — one flaky optional field blanking the whole post, required Dashboard included. measure-lcp now converts an incomplete summary into a scenario error (findIncompleteSummaryFields), so the optional/required policy applies to it like any other measurement failure; the poster's atomic gate remains the backstop for stale artifacts. Also: tcEscape covers the Unicode line terminators the TeamCity spec requires (U+0085/U+2028/U+2029), and tests pin both sides of the partial-summary path (measure-layer classification plus the poster backstop).
…view round 3) Both review vendors converged on one remaining gap: a mutation test showed that deleting the findIncompleteSummaryFields() call in main()'s scenario loop left every focused test green, so a refactor could silently regress the partial-summary classification. Rather than adding injectable test seams, the check moves into measureLCP's tail, extracted as the pure finalizeMeasurement( scenario, results, iterations, url ) — the validResults filter, the all-iterations-failed throw, buildSummary, and the completeness throw — so both refusal paths are unit-tested through the real aggregation and the untested wiring shrinks to a one-line tail call plus main()'s pre-existing catch. Riders: the partial-summary message now reports the valid (not attempted) iteration count, and a new test pins the flat-LCP-mirror invariant the legacy ['lcp'] fallback relies on.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the tools/performance CodeVitals pipeline so a single scenario’s measurement failure no longer blocks posting healthy scenarios. It introduces an explicit per-scenario failure policy (optional) and centralizes the “green build means safe to post” decision in measure-lcp.js, with warnings surfaced for green-but-partial runs.
Changes:
- Add per-scenario required/optional failure semantics and enforce them via
measure-lcpexit code (and a poster-side “fail closed” backstop for required failures). - Treat partial/incomplete summaries as measurement failures during measurement (so they can be isolated by policy rather than tripping the atomic sanity gate later).
- Emit TeamCity WARNING messages for optional-scenario failures that don’t fail the build, and add unit tests covering the new policy and edge cases.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tools/performance/scripts/scenarios.js | Adds optional failure-policy flag per scenario; removes legacy isBaseline usage. |
| tools/performance/scripts/run-performance-tests.js | Reports optional-scenario failures on green runs via TeamCity WARNINGs; exports helpers for testing. |
| tools/performance/scripts/post-to-codevitals.js | Skips optional failures but fails closed if a required scenario is present and unusable (prevent survivor posting from a red run artifact). |
| tools/performance/scripts/post-to-codevitals.test.js | Adds coverage for policy invariants and required/optional posting behavior. |
| tools/performance/scripts/measure-lcp.js | Implements scenario-set validation, outcome computation, and partial-summary detection as measurement failures. |
| tools/performance/scripts/measure-lcp-outcome.test.js | New unit test suite pinning scenario selection, exit semantics, summary completeness checks, and TeamCity escaping/warnings. |
| tools/performance/README.md | Documents the per-scenario failure isolation semantics and the resulting posting/build outcomes. |
When more than one optional scenario fails, the TeamCity warning read "measurement failed; its CodeVitals keys" regardless of count. Pick measurement/measurements and its/their from the failure count, and pin the full multi-scenario sentence in the test instead of just the joined names.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
tools/performance/scripts/run-performance-tests.js:15
- This module is imported by unit tests (measure-lcp-outcome.test.js) to access tcEscape/reportSkippedScenarios. Loading
.envat module scope means a developer’s local tools/performance/.env can mutate process.env during unit tests, creating non-deterministic test behavior. Gate the dotenv load behind direct invocation so imports stay side-effect-free.
import { computeRunOutcome } from './measure-lcp.js';
import { isDirectInvocation, VALIDATION_FAILED_EXIT_CODE } from './post-to-codevitals.js';
import { SCENARIOS, getScenarioUrl } from './scenarios.js';
// Load .env file from the performance directory if it exists
…'s skip warning The poster printed the same "No measurement data" warning whether a scenario was absent from the results file (a targeted SCENARIO run, nothing wrong) or present but failed. A targeted pnpm report run therefore looked like the required Dashboard had broken. Name the two cases separately and pin both wordings in the suffix test.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
tools/performance/scripts/run-performance-tests.js:15
- Importing this module now happens in unit tests (measure-lcp-outcome.test.js), which means this top-level
.envauto-loading can mutate process.env during the test run if a localtools/performance/.envexists. That makes the unit tests non-hermetic and can cause hard-to-debug local failures. Consider only loading.envwhen the script is directly invoked (not when imported for tcEscape/reportSkippedScenarios exports).
import { computeRunOutcome } from './measure-lcp.js';
import { isDirectInvocation, VALIDATION_FAILED_EXIT_CODE } from './post-to-codevitals.js';
import { SCENARIOS, getScenarioUrl } from './scenarios.js';
// Load .env file from the performance directory if it exists
LiamSarsfield
deleted the
forms-728-isolate-per-scenario-measurement-failures-from-codevitals
branch
July 14, 2026 09:33
github-actions
Bot
removed
[Status] In Progress
[Status] Needs Author Reply
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes FORMS-728
Proposed changes
Every CodeVitals series froze at
ad4a454f7f(2026-07-07): #49272 broke theformsResponsesscenario, and one broken scenario blanks the whole run, healthy measurements included. ~180 trunk commits are unposted.This makes failure handling per-scenario. Each scenario declares a policy:
jetpackConnected(the Dashboard baseline) is required,formsResponsesandmyJetpackare optional. An optional failure keeps the build green, skips that scenario's keys, and emits a TeamCity warning. A required failure, or a run where every scenario failed, stays red and posts nothing.Two choices worth knowing the reason for:
optionalcovers measurement failures only: a throw, or a missing/partial summary. A measured value outsideSANITY_RANGESstill trips the pre-existing atomic gate, since anomalous data should red the build for a human.The full semantics table is in the README ("Per-scenario failure isolation"). Fixing
formsResponsesitself is a separate ticket.Related product discussion/links
Does this pull request change what data or activity we track or use?
No. Same metrics, same keys; this only changes when a build may post them.
Testing instructions
Run the unit tests:
All 141 should pass.
I also validated this live against the current outage, since the broken
formsResponsesscenario is the exact optional-failure case this PR is for (noCODEVITALS_TOKENset, so nothing posted):ITERATIONS=2 node scripts/run-performance-tests.js --skip-codevitalsexits 0. Dashboard and My Jetpack get measured, forms showsFAILED (optional …)plus a TeamCity warning.pnpm report:dryon those results has noforms-responses-*keys.SCENARIOtypo, and a targeted run where everything fails all exit 1.