Skip to content

Add synthetic p90 performance budget spec#92

Merged
shayancoin merged 2 commits intomainfrom
codex/add-performance-test-for-p90-value
Oct 17, 2025
Merged

Add synthetic p90 performance budget spec#92
shayancoin merged 2 commits intomainfrom
codex/add-performance-test-for-p90-value

Conversation

@shayancoin
Copy link
Owner

@shayancoin shayancoin commented Oct 16, 2025

Summary

  • add a Playwright spec that parses perf-budget.yml with an inline YAML parser
  • simulate warmup and sampled runs, attach run artifacts, and compute percentile summaries
  • assert the deterministic p90 for the configurator's first-contentful-paint metric

Testing

  • Tests not run (not requested)

https://chatgpt.com/codex/tasks/task_e_68f12bec86e88330bc2117c70c7cfc8f

Summary by CodeRabbit

  • Tests
    • Added automated performance testing suite to continuously monitor and validate application performance metrics against established targets.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 16, 2025

Note

Other AI code review bot(s) detected

CodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review.

Walkthrough

Adds a new Playwright end-to-end performance test file that parses a performance budget YAML configuration, generates synthetic metric values across multiple runs per page, computes 90th percentile values, and validates them against configured thresholds.

Changes

Cohort / File(s) Summary
New E2E Performance Test
frontend/tests/e2e/perf.p90.spec.ts
Introduces performance budget testing with YAML parsing, type definitions (PerfMetric, PerfPage, PerfBudget), utility functions for deterministic seed generation and synthetic metric simulation, percentile computation, and test logic that runs warmup and simulated page runs, collects metric samples, computes p90 values, and validates against thresholds including a specific FCP target check for the configurator page.

Sequence Diagram(s)

sequenceDiagram
    actor Test
    participant YAMLFile as perf-budget.yml
    participant Parser as YAML Parser
    participant Budget as PerfBudget
    participant Runner as Test Runner
    participant Metrics as Metric Generator
    participant Aggregator as Aggregator
    participant Assertions as Assertions

    Test->>YAMLFile: Read perf-budget.yml
    Test->>Parser: Parse YAML
    Parser->>Budget: Create PerfBudget
    Budget-->>Test: Budget loaded & attached

    loop For each page in budget
        Test->>Runner: Warmup run
        Runner->>Metrics: Generate warmup metrics
        Metrics-->>Runner: Warmup data
        Runner-->>Test: Attach warmup data

        loop For N runs (runCount)
            Test->>Metrics: Simulate metric values
            Note over Metrics: Use deterministic seed
            Metrics-->>Test: Per-run metrics
            Test->>Aggregator: Collect samples
        end

        Aggregator->>Aggregator: Compute p90 percentile
        Aggregator-->>Test: Per-page summary
    end

    Test->>Assertions: Validate p90 ≤ threshold
    Assertions->>Assertions: Check configurator FCP p90 = 1363.54
    Assertions-->>Test: Pass/Fail
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

The addition involves multiple interrelated concerns: type system design, custom YAML parsing logic with scalar and nested structure handling, deterministic synthetic data generation utilities, percentile computation, test orchestration across multiple runs, and metric validation logic. While homogeneous in intent (performance testing), the code density and variety of algorithms (hashing, seeding, simulation, aggregation) warrant careful review.

Possibly related PRs

Poem

🐰 Hops through metrics with digital cheer,
Parsing budgets, p90s crystal clear!
Synthetic runs dance in deterministic grace,
Performance thresholds met, test won the race!

Pre-merge checks and finishing touches

❌ Failed checks (2 warnings)
Check name Status Explanation Resolution
Description Check ⚠️ Warning The pull request description does not follow the repository’s template because it lacks the required “# PR Type,” “# Short Description,” and “# Tests Added” sections and instead uses “## Summary” and “## Testing,” making it incomplete. Please update the description to include the “# PR Type,” “# Short Description,” and “# Tests Added” headings from the template and provide the corresponding details for each section.
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (1 passed)
Check name Status Explanation
Title Check ✅ Passed The title succinctly and accurately summarizes the main change by indicating a new synthetic p90 performance budget spec is being added and is concise and specific to the changeset.
✨ Finishing touches
  • 📝 Docstrings were successfully generated.
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch codex/add-performance-test-for-p90-value

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ad4b85c and 69aee62.

📒 Files selected for processing (1)
  • frontend/tests/e2e/perf.p90.spec.ts (1 hunks)
🧰 Additional context used
🪛 Biome (2.1.2)
frontend/tests/e2e/perf.p90.spec.ts

[error] 188-188: Unexpected empty object pattern.

(lint/correctness/noEmptyPattern)


Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting

Comment on lines +1 to +3
import { test, expect } from '@playwright/test';
import { readFileSync } from 'node:fs';
import { resolve } from 'node:path';

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Ensure the Playwright spec runs under an installed test runner

This spec lives in /tests and imports @playwright/test, but the only Playwright configuration and dependency in the repo are under the frontend package (frontend/playwright.config.ts with testDir: './tests/e2e'). As written, running the existing Playwright tests from frontend will neither discover this file nor resolve @playwright/test from the repository root, so the new test is never executed. To make the check effective, move the spec into frontend/tests/e2e (or add a root Playwright project) so it is picked up by the configured runner and can resolve its dependency.

Useful? React with 👍 / 👎.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d2a35c4 and ad4b85c.

📒 Files selected for processing (1)
  • tests/perf.p90.spec.ts (1 hunks)
🧰 Additional context used
🪛 Biome (2.1.2)
tests/perf.p90.spec.ts

[error] 188-188: Unexpected empty object pattern.

(lint/correctness/noEmptyPattern)

return result;
}

test('perf budget p90 percentile stays under the synthetic threshold', async ({}, testInfo) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Remove empty destructuring to satisfy Biome

Biome flags the empty destructuring pattern here. Switch to an unused binding so the fixtures remain available without tripping lint.

-test('perf budget p90 percentile stays under the synthetic threshold', async ({}, testInfo) => {
+test('perf budget p90 percentile stays under the synthetic threshold', async (_context, testInfo) => {
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
test('perf budget p90 percentile stays under the synthetic threshold', async ({}, testInfo) => {
test('perf budget p90 percentile stays under the synthetic threshold', async (_context, testInfo) => {
🧰 Tools
🪛 Biome (2.1.2)

[error] 188-188: Unexpected empty object pattern.

(lint/correctness/noEmptyPattern)

🤖 Prompt for AI Agents
In tests/perf.p90.spec.ts around line 188, the test callback currently uses an
empty destructuring pattern ({}, testInfo) which Biome flags; replace the empty
destructuring with a single unused binding (e.g. (_unused, testInfo) or (_ ,
testInfo)) so the fixtures parameter remains available without triggering the
lint rule, and update any other tests in the file using the same pattern
similarly.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 17, 2025

Caution

Docstrings generation - FAILED

No docstrings were generated.

@shayancoin shayancoin merged commit b7c2949 into main Oct 17, 2025
2 of 7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant