Sponsored Content
Skip to content

test: Fix flaky subscribe tests - #6510

Merged
bthomee merged 9 commits into
XRPLF:developfrom
kuznetsss:6399_Another_attempt_to_fix_flaky_test
Mar 11, 2026
Merged

test: Fix flaky subscribe tests#6510
bthomee merged 9 commits into
XRPLF:developfrom
kuznetsss:6399_Another_attempt_to_fix_flaky_test

Conversation

@kuznetsss

@kuznetsss kuznetsss commented Mar 9, 2026

Copy link
Copy Markdown
Contributor

High Level Overview of Change

This PR hopefully fixes flaky Subscribe tests.
Fixes #6399.

Context of Change

Subscribe tests have a problem that there is no way to synchronise application running in background threads and test's thread. Threads are communicating by websocket messages. When the code is compiled in debug mode with code coverage it executes quite slow, so receiving websocket messages by the client in subscribe tests may time out.
This fix does 2 things to fix the problem:

  • increases timeout for receiving a websocket message
  • decreases the number of tests running in parallel

While testing the fix for subscribe test another flaky test in ledger replay was found. I left a comment in the test describing what was wrong in the flaky ledger replay test.

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)

@kuznetsss
kuznetsss marked this pull request as ready for review March 9, 2026 17:19
@kuznetsss
kuznetsss force-pushed the 6399_Another_attempt_to_fix_flaky_test branch from b4a4566 to 2dcbc95 Compare March 9, 2026 17:53
Comment thread .github/workflows/reusable-build-test-config.yml Outdated
@kuznetsss

kuznetsss commented Mar 10, 2026

Copy link
Copy Markdown
Contributor Author

CI failed after running tests 15 times in debug with code coverage. But the other test turned out to be flaky:

19> #212 failed: LedgerReplay_test.cpp(1133)

https://github.com/XRPLF/rippled/actions/runs/22867192993/job/66337167696#step:20:52692

I've added a fix for that. Let's see how it goes now.

@kuznetsss

kuznetsss commented Mar 10, 2026

Copy link
Copy Markdown
Contributor Author

With the fix for LedgerReplay test, CI has successfully run tests 7 times and then the runner lost communication.
https://github.com/XRPLF/rippled/actions/runs/22905275224/job/66462220709?pr=6510

Let's try again.

@kuznetsss

Copy link
Copy Markdown
Contributor Author

Operation got cancelled in CI. But it've run tests successfully 11 more times. So I think the fix is fine. Overall we have:

  • The fix for subscribe test was checked 34 times
  • The fix for ledger replay test was checked 18 times

I think this should be enough to say that both fixes are good.

@kuznetsss
kuznetsss force-pushed the 6399_Another_attempt_to_fix_flaky_test branch from 7ff6b41 to 968c155 Compare March 10, 2026 17:38
@kuznetsss
kuznetsss requested a review from a1q123456 March 10, 2026 17:44
@codecov

codecov Bot commented Mar 10, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 79.8%. Comparing base (7b3724b) to head (b6b0dce).
⚠️ Report is 1 commits behind head on develop.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff            @@
##           develop   #6510     +/-   ##
=========================================
- Coverage     79.8%   79.8%   -0.0%     
=========================================
  Files          862     862             
  Lines        67883   67883             
  Branches      7551    7549      -2     
=========================================
- Hits         54199   54196      -3     
- Misses       13684   13687      +3     

see 6 files with indirect coverage changes

Impacted file tree graph

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@kuznetsss
kuznetsss requested a review from bthomee March 11, 2026 12:05

@a1q123456 a1q123456 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It's an attempt anyway so it's fine as long as we don't try to cover the issue. i.e. skip the checks if we receive no message. Also I did want to test if NPROC is contributing to the issue. Let's see if things are improved!

Comment thread .github/workflows/reusable-build-test-config.yml Outdated
Comment thread src/test/rpc/Subscribe_test.cpp
@kuznetsss 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 11, 2026
Comment thread .github/workflows/reusable-build-test-config.yml Outdated
@kuznetsss
kuznetsss force-pushed the 6399_Another_attempt_to_fix_flaky_test branch 4 times, most recently from 0ffa880 to 0fcd01b Compare March 11, 2026 15:12
@kuznetsss
kuznetsss force-pushed the 6399_Another_attempt_to_fix_flaky_test branch from 0fcd01b to b6b0dce Compare March 11, 2026 15:14
@bthomee bthomee changed the title test: Another attempt to fix flaky subscribe tests test: Fix flaky subscribe tests Mar 11, 2026
@bthomee
bthomee enabled auto-merge (squash) March 11, 2026 15:33
@bthomee
bthomee merged commit c791cae into XRPLF:develop Mar 11, 2026
3 checks passed
@kuznetsss
kuznetsss deleted the 6399_Another_attempt_to_fix_flaky_test branch March 12, 2026 11:27
@mvadari mvadari added this to the 3.2.0 milestone May 20, 2026
beartec-jpg pushed a commit to beartec-jpg/FalconLedger that referenced this pull request Jun 1, 2026
Subscribe tests have a problem that there is no way to synchronize application running in background threads and test threads. Threads are communicating via websocket messages. When the code is compiled in debug mode with code coverage enabled it executes quite slow, so receiving websocket messages by the client in subscribe tests may time out.

This change does 2 things to fix the problem:
* Increases timeout for receiving a websocket message.
* Decreases the number of tests running in parallel.

While testing the fix for subscribe test another flaky test in ledger replay was found, which has also been addressed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Ready to merge *PR author* thinks it's ready to merge. Has passed code review. Perf sign-off may still be required.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Flaky subscribe test

4 participants