Skip to content

Conversation

@mvadari
Copy link
Collaborator

@mvadari mvadari commented Oct 20, 2025

High Level Overview of Change

This PR shrinks timeouts to make connections.test.ts run faster. This PR shrinks it from taking 24 seconds to 10.

These changes only appear to be locally, not in CI for some reason, though.

Context of Change

connections.test.ts is by far the longest running test in the test suite.

Type of Change

  • Tests (You added tests for code that already exists, or your new feature included in this PR)

Did you update HISTORY.md?

  • No, this change does not impact library users

Test Plan

CI passes.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 20, 2025

Walkthrough

The changes introduce configurable connection timeout options to test setup utilities and adjust test timing parameters. A new CONNECTION_TIMEOUT constant is added and referenced in error assertions, timeout delays in tests are shortened across multiple locations, and ClientOptions parameter support is added to setup helper functions.

Changes

Cohort / File(s) Summary
Test Connection Timeout Configuration
packages/xrpl/test/connection.test.ts
Introduces CONNECTION_TIMEOUT constant and passes it via clientOptions to setupClient. Replaces hard-coded timeout value in error message assertion. Reduces test delay timeouts from 5000 ms to 500 ms and 2000 ms to 1500 ms in multiple locations.
Setup Client ClientOptions Support
packages/xrpl/test/setupClient.ts
Adds ClientOptions import and wires it into function signatures. setupMockRippledConnection and setupClient now accept optional clientOptions parameter and forward it to Client constructor. Defaults to empty object when no options provided.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

🐰 Hops with delight at tests refined,
Timeouts now tuned to the right mind,
ConnectionOptions flow through the code,
Shorter delays lighten the load,
Setup utilities dance in new light!

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.
Description Check ❓ Inconclusive The pull request description follows the required template structure and addresses all major sections: High Level Overview of Change, Context of Change, Type of Change (Tests selected), HISTORY.md update status, and Test Plan. However, the description lacks specificity and detail in several areas. The High Level Overview vaguely refers to "shrinking timeouts" without clearly articulating the specific changes (such as introducing CONNECTION_TIMEOUT constant, updating function signatures to accept clientOptions, and specific timeout value modifications). The Test Plan section is minimal, providing only "CI passes" without describing what tests were run or how changes were validated. While the structure is correct, the vague and generic nature of the content makes it difficult to fully understand the scope and impact of the changes from the PR description alone. To improve this PR description, please provide more detailed information about the specific changes made (such as introducing the CONNECTION_TIMEOUT constant, updating setupClient and setupMockRippledConnection function signatures, and listing the specific timeout reductions made). Additionally, expand the Test Plan section to describe which tests were executed and what validation was performed beyond just stating "CI passes" to ensure reviewers have sufficient context for evaluation.
✅ Passed checks (1 passed)
Check name Status Explanation
Title Check ✅ Passed The pull request title "test: make connections.test.ts run faster" directly and clearly summarizes the main change in the PR. It uses a descriptive prefix ("test:") to indicate the category of change, specifies the affected test file, and articulates the primary improvement being made. The title is concise, specific, and contains no vague terminology or unnecessary noise.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix-connection-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.

@mvadari mvadari requested a review from Copilot October 20, 2025 22:08
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR optimizes the connections.test.ts test suite by reducing timeout values, cutting execution time from 24 seconds to 10 seconds. The changes introduce configurable client options through the test setup functions and adjust hardcoded timeouts to shorter durations.

Key Changes:

  • Added ClientOptions parameter support to setupClient and setupMockRippledConnection functions
  • Reduced connection timeout from default 5000ms to 1000ms for all tests
  • Shortened various setTimeout calls from 5000ms/2000ms to 500ms/1500ms

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
packages/xrpl/test/setupClient.ts Added optional clientOptions parameter to setup functions to allow configuring Client instances with custom options
packages/xrpl/test/connection.test.ts Introduced CONNECTION_TIMEOUT constant set to 1000ms and reduced hardcoded timeouts in tests to make suite run faster

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

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