tests: Improve stability of Subscribe tests by kuznetsss · Pull Request #6420 · XRPLF/rippled

kuznetsss · 2026-02-25T17:40:44Z

High Level Overview of Change

This PR improves stability of Subscribe tests.

Context of Change

Subscribe tests were flaky because each test performs some operations (e.g. sends transactions) and waits for messages to appear in subscription with 100ms timeout. If tests are slow (because compiled in debug or the machine is slow) some of them could fail. This PR adds an attempt to synchronise background Env's thread and test's thread by ensuring that all the scheduled operations are started before test's thread starts to wait for a websocket message. This is done by limiting IO threads of the app inside Env to 1 and adding a synchronisation barrier after closing ledger.

Type of Change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Refactor (non-breaking change that only restructures code)
Performance (increase or change in throughput and/or latency)
Tests (you added tests for code that already exists, or your new feature included in this PR)
Documentation update
Chore (no impact to binary, e.g. .gitignore, formatting, dropping support for older tooling)
Release

API Impact

Public API: New feature (new methods and/or new fields)
Public API: Breaking change (in general, breaking changes should only impact the next api_version)
libxrpl change (any change that may affect libxrpl or dependents of libxrpl)
Peer protocol change (must be backward compatible or bump the peer protocol version)

Copilot

Pull request overview

This PR addresses flaky Subscribe tests by introducing a synchronization mechanism to ensure WebSocket subscription messages have been initiated before tests check for them. The issue (#6399) was caused by race conditions where tests would check for messages before the server's background thread had posted the async write operations.

Changes:

Added syncClose() method to test Env class that closes a ledger and synchronizes with the server's io_context
Replaced all env.close() calls in Subscribe_test.cpp with BEAST_EXPECT(env.syncClose()) to ensure synchronization
Added lambda capture of this in sendPayments helper to support BEAST_EXPECT usage

Reviewed changes

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

File	Description
src/test/jtx/Env.h	Added syncClose() method with comprehensive documentation that closes a ledger and waits for all posted async operations to start
src/test/rpc/Subscribe_test.cpp	Replaced env.close() with env.syncClose() throughout and fixed lambda capture to enable test assertions

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

src/test/jtx/Env.h

codecov · 2026-02-25T18:22:58Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 79.8%. Comparing base (1a7f824) to head (d9c46d5).

Additional details and impacted files

@@            Coverage Diff            @@
##           develop   #6420     +/-   ##
=========================================
- Coverage     79.8%   79.8%   -0.0%     
=========================================
  Files          858     858             
  Lines        67757   67761      +4     
  Branches      7562    7553      -9     
=========================================
- Hits         54081   54079      -2     
- Misses       13676   13682      +6

Files with missing lines	Coverage Δ
src/xrpld/app/main/Application.cpp	`70.4% <100.0%> (+0.4%)`	⬆️
src/xrpld/app/main/BasicApp.h	`100.0% <100.0%> (ø)`

... and 2 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copilot

Pull request overview

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

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

godexsoft

Seems like this relies on the fact we have one thread in tests. If it changes there may be issues but for now this is best we can do by the looks of it 👍

kuznetsss · 2026-02-27T13:33:18Z

@godexsoft, you are right and it turned out that the application was not single threaded. I've added an assert into syncClose() and enforced the application to have only 1 IO thread in these tests.

Copilot

Pull request overview

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

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

src/test/jtx/Env.h

src/xrpld/app/main/Application.h

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Copilot

Pull request overview

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

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

src/xrpld/app/main/BasicApp.h

src/test/jtx/Env.h

src/xrpld/app/main/Application.h

src/test/jtx/impl/envconfig.cpp

a1q123456

May be a good approach. We can merge it and see if it works.

tests: Add syncClose() for async operation synchronisation in tests

33c0d68

Copilot AI review requested due to automatic review settings February 25, 2026 17:40

Copilot started reviewing on behalf of kuznetsss February 25, 2026 17:41 View session

Copilot AI reviewed Feb 25, 2026

View reviewed changes

src/test/jtx/Env.h Show resolved Hide resolved

src/test/jtx/Env.h Outdated Show resolved Hide resolved

Add missing include

2c660b6

kuznetsss requested review from a1q123456 and godexsoft February 25, 2026 18:00

Merge branch 'develop' into 6399_Fix_flaky_subscribe_test

3280979

Copilot AI review requested due to automatic review settings February 26, 2026 12:10

Copilot started reviewing on behalf of kuznetsss February 26, 2026 12:10 View session

Copilot AI reviewed Feb 26, 2026

View reviewed changes

godexsoft approved these changes Feb 26, 2026

View reviewed changes

Make the app single threaded

bec3aea

kuznetsss requested review from Copilot and godexsoft February 27, 2026 13:35

Copilot started reviewing on behalf of kuznetsss February 27, 2026 13:35 View session

Copilot AI reviewed Feb 27, 2026

View reviewed changes

src/test/jtx/Env.h Outdated Show resolved Hide resolved

src/xrpld/app/main/Application.h Outdated Show resolved Hide resolved

Update src/xrpld/app/main/Application.h

ce226bd

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Copilot AI review requested due to automatic review settings February 27, 2026 14:34

Copilot started reviewing on behalf of kuznetsss February 27, 2026 14:35 View session

Copilot AI reviewed Feb 27, 2026

View reviewed changes

kuznetsss added 2 commits February 27, 2026 14:48

Fix race condition

0739050

Merge branch 'develop' into 6399_Fix_flaky_subscribe_test

4663e6d

Copilot AI review requested due to automatic review settings February 27, 2026 15:14

Copilot started reviewing on behalf of kuznetsss February 27, 2026 15:15 View session

Copilot AI reviewed Feb 27, 2026

View reviewed changes

src/xrpld/app/main/BasicApp.h Show resolved Hide resolved

src/test/jtx/Env.h Show resolved Hide resolved

src/test/jtx/Env.h Show resolved Hide resolved

src/xrpld/app/main/Application.h Show resolved Hide resolved

src/test/jtx/impl/envconfig.cpp Show resolved Hide resolved

a1q123456 approved these changes Mar 2, 2026

View reviewed changes

Merge branch 'develop' into 6399_Fix_flaky_subscribe_test

d9c46d5

kuznetsss added the Ready to merge *PR author* thinks it's ready to merge. Has passed code review. Perf sign-off may still be required. label Mar 2, 2026

Conversation

kuznetsss commented Feb 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

High Level Overview of Change

Context of Change

Type of Change

API Impact

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

codecov bot commented Feb 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

godexsoft left a comment

Choose a reason for hiding this comment

Uh oh!

kuznetsss commented Feb 27, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

a1q123456 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

kuznetsss commented Feb 25, 2026 •

edited

Loading

codecov bot commented Feb 25, 2026 •

edited

Loading