Skip to content

fix metrics test#916

Merged
BilalG1 merged 1 commit into
devfrom
fix-flaky-metrics-test
Sep 24, 2025
Merged

fix metrics test#916
BilalG1 merged 1 commit into
devfrom
fix-flaky-metrics-test

Conversation

@BilalG1
Copy link
Copy Markdown
Collaborator

@BilalG1 BilalG1 commented Sep 24, 2025

High-level PR Summary

This PR fixes a test in the internal metrics endpoint by replacing a simple wait timer with a more robust retry mechanism. Instead of waiting for a fixed 3000ms delay to allow for asynchronous event logging, the PR implements a Result.retry approach that attempts to fetch metrics up to 5 times with exponential backoff, only proceeding when the metrics response differs from the initial state. This change makes the test more reliable by ensuring proper test conditions are met before assertions are made rather than relying on an arbitrary timeout.

⏱️ Estimated Review Time: 5-15 minutes

💡 Review Order Suggestion
Order File Path
1 apps/e2e/tests/backend/endpoints/api/v1/internal-metrics.test.ts

Need help? Join our Discord

Summary by CodeRabbit

  • Tests
    • Improved backend metrics verification with a retry-based approach to handle eventual consistency, reducing test flakiness.
    • Replaced static waits with structured retries for faster, more reliable execution.
    • Strengthened assertions on metrics fields (e.g., totals and recent activity) for clearer validation.
    • Maintains checks to ensure anonymous users remain excluded from metrics.

@vercel
Copy link
Copy Markdown

vercel Bot commented Sep 24, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Preview Comments Updated (UTC)
stack-backend Ready Ready Preview Comment Sep 24, 2025 10:07pm
stack-dashboard Ready Ready Preview Comment Sep 24, 2025 10:07pm
stack-demo Ready Ready Preview Comment Sep 24, 2025 10:07pm
stack-docs Ready Ready Preview Comment Sep 24, 2025 10:07pm

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Sep 24, 2025

Walkthrough

Replaces direct metrics assertions in an end-to-end test with a retry-based approach using Result.retry to wait for eventual consistency, adjusts assertions to use result.data.body fields, and integrates retry outcome into final checks and helper invocation.

Changes

Cohort / File(s) Summary of Changes
E2E test: internal metrics retry flow
apps/e2e/tests/backend/endpoints/api/v1/internal-metrics.test.ts
Added Result import; replaced static wait and immediate assertions with Result.retry loop comparing responses to a captured baseline; handled retry errors by asserting equality then throwing; updated assertions to use result.data.body for multiple metrics; passed retry result data to helper ensuring anonymous users are excluded.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor Tester
  participant Test as E2E Test
  participant Retry as Result.retry
  participant API as Internal Metrics API

  Tester->>Test: Run internal-metrics test
  Test->>API: Capture baseline metrics
  alt Retry until metrics match baseline
    Test->>Retry: Invoke with fetch+compare
    loop Attempts with backoff
      Retry->>API: Fetch metrics
      API-->>Retry: Response
      Retry->>Retry: Compare to baseline
    end
    Retry-->>Test: Success (data.body)
  else Error after retries
    Retry-->>Test: Error
    Test->>Test: Assert last metrics equal to baseline
    Test-->>Tester: Throw error
  end
  Test->>Test: Assert totals/recent/daily/by-country on result.data.body
  Test->>Test: ensureAnonymousUsersAreStillExcluded(result.data)
  Test-->>Tester: Complete
Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

I twitch my nose at flakey nights,
Hop-hop through retried heights—
No sleepy waits, just loops that sing,
Until the numbers match in spring.
Metrics align, my paws approve,
Consistent trails in every groove. 🐇✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning, 1 inconclusive)
Check name Status Explanation Resolution
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.
Title Check ❓ Inconclusive The pull request title “fix metrics test” is a vague and overly generic description that does not clearly specify which metrics test is being fixed or what the main change entails, making it difficult for a teammate to understand the context. Please improve the title to clearly indicate which metrics test is being fixed and summarize the key change, for example “Improve retry logic in internal metrics test to replace static wait.”
✅ Passed checks (1 passed)
Check name Status Explanation
Description Check ✅ Passed The pull request description includes the required CONTRIBUTING.md HTML comment template and provides a detailed high-level summary of the change, so it meets the repository’s description template requirements.
✨ Finishing touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix-flaky-metrics-test

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

Copy link
Copy Markdown

@recurseml recurseml Bot left a comment

Choose a reason for hiding this comment

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

Review by RecurseML

🔍 Review performed on 3fe82b6..99fe457

  Severity     Location     Issue     Delete  
Medium apps/e2e/tests/backend/endpoints/api/v1/internal-metrics.test.ts:130 Missing runAsynchronously wrapper for async operation

Comment thread apps/e2e/tests/backend/endpoints/api/v1/internal-metrics.test.ts
Comment thread apps/e2e/tests/backend/endpoints/api/v1/internal-metrics.test.ts
Copy link
Copy Markdown
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

🧹 Nitpick comments (1)
apps/e2e/tests/backend/endpoints/api/v1/internal-metrics.test.ts (1)

139-140: Compare the response bodies for clearer diagnostics
Line 139 is comparing beforeMetrics.body to the entire NiceResponse stored in result.error, so the assertion will always fail even when the bodies match. Swapping to result.error.body keeps the branch’s intent—surfacing the diff between the payloads—without a guaranteed mismatch.

-    expect(beforeMetrics.body).toEqual(result.error);
+    expect(beforeMetrics.body).toEqual(result.error.body);
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 3fe82b6 and 99fe457.

📒 Files selected for processing (1)
  • apps/e2e/tests/backend/endpoints/api/v1/internal-metrics.test.ts (2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.test.{ts,tsx,js}

📄 CodeRabbit inference engine (AGENTS.md)

In tests, prefer .toMatchInlineSnapshot where possible; refer to snapshot-serializer.ts for snapshot formatting and handling of non-deterministic values

Files:

  • apps/e2e/tests/backend/endpoints/api/v1/internal-metrics.test.ts
**/*.{ts,tsx,js,jsx}

📄 CodeRabbit inference engine (AGENTS.md)

Prefer ES6 Map over Record when representing key–value collections

Files:

  • apps/e2e/tests/backend/endpoints/api/v1/internal-metrics.test.ts
🧬 Code graph analysis (1)
apps/e2e/tests/backend/endpoints/api/v1/internal-metrics.test.ts (1)
apps/e2e/tests/backend/backend-helpers.ts (1)
  • niceBackendFetch (107-171)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
  • GitHub Check: docker
  • GitHub Check: docker
  • GitHub Check: lint_and_build (latest)
  • GitHub Check: restart-dev-and-test
  • GitHub Check: all-good
  • GitHub Check: build (22.x)
  • GitHub Check: setup-tests
  • GitHub Check: build (22.x)
  • GitHub Check: Security Check

Comment thread apps/e2e/tests/backend/endpoints/api/v1/internal-metrics.test.ts
@BilalG1 BilalG1 requested a review from N2D4 September 24, 2025 22:12
@BilalG1 BilalG1 assigned N2D4 and unassigned BilalG1 Sep 24, 2025
@github-actions github-actions Bot assigned BilalG1 and unassigned N2D4 Sep 24, 2025
@BilalG1 BilalG1 merged commit e256be8 into dev Sep 24, 2025
31 checks passed
@BilalG1 BilalG1 deleted the fix-flaky-metrics-test branch September 24, 2025 22:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants