test: Retry all RPC commands to fix MacOS CI jobs #5171

ximinez · 2024-10-30T23:40:05Z

High Level Overview of Change

Retry all failed RPC connections / commands in unit tests.

Context of Change

Follow up to #5120 (23991c9), which added a retry for submit commands. It improved MacOS test reliability, but other tests are failing now.

Type of Change

Bug fix (non-breaking change which fixes an issue)
Tests (you added tests for code that already exists, or your new feature included in this PR)

codecov · 2024-10-31T00:09:18Z

Codecov Report

Attention: Patch coverage is 0% with 2 lines in your changes missing coverage. Please review.

Project coverage is 77.9%. Comparing base (838978b) to head (34d918a).

Files with missing lines	Patch %	Lines
include/xrpl/server/detail/Door.h	0.0%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff            @@
##           develop   #5171     +/-   ##
=========================================
- Coverage     77.9%   77.9%   -0.0%     
=========================================
  Files          782     782             
  Lines        66621   66619      -2     
  Branches      8161    8158      -3     
=========================================
- Hits         51902   51873     -29     
- Misses       14719   14746     +27

Files with missing lines	Coverage Δ
include/xrpl/server/detail/Door.h	`79.3% <0.0%> (ø)`

... and 7 files with indirect coverage changes

* Follow up to XRPLF#5120 (23991c9), which added a retry for submit commands. It improved MacOS test reliability, but other tests are failing now. * Retry all failed RPC connections / commands in unit tests.

* upstream/develop: Add hubs.xrpkuwait.com to bootstrap (5169)

* upstream/develop: Add AMMClawback Transaction (XLS-0073d) (5142)

* upstream/develop: Fix unity build (5179)

thejohnfreeman · 2024-11-08T01:15:58Z

src/test/jtx/Env.h

@@ -292,6 +292,17 @@ class Env
        The command is examined and used to build
        the correct JSON as per the arguments.
    */
+    using rpcCallback = std::function<bool(Json::Value const&)>;


Should we capitalize the name (RpcCallback)?

Please add a comment like "Given the return value of an RPC, returns true if it is acceptable."

Yes. Fixed.

thejohnfreeman · 2024-11-08T01:22:48Z

src/test/jtx/impl/Env.cpp

+        static auto const defaultCallback = [](Json::Value const& jr) {
+            auto const parsedResult = parseResult(jr, false);
+            return parsedResult.rpcCode != rpcINTERNAL;
+        };
+        cb = defaultCallback;


Can we move this out of the function, into the definition of useDefaultCallback (either inline or extern), and rename useDefaultCallback to something like rejectInternalError? If you prefer to frame the predicate names as accept{Condition} instead of reject{Condition}, then I'd be fine with that too.

Yes. Fixed.

thejohnfreeman · 2024-11-08T01:23:43Z

src/test/jtx/Env.h

@@ -292,6 +292,17 @@ class Env
        The command is examined and used to build
        the correct JSON as per the arguments.
    */
+    using rpcCallback = std::function<bool(Json::Value const&)>;
+    const rpcCallback useDefaultCallback{};
+    const rpcCallback expectInternalRPCError = [](Json::Value const&) {


Can we rename this to rejectNever and add a comment explaining that it is meant to be used for tests that expect an internal error and do not want to trigger a retry?

Yes. Fixed.

thejohnfreeman · 2024-11-08T01:24:52Z

src/test/jtx/Env.h

+    getInternalFailureCallback(bool shouldFail)
+    {
+        return shouldFail ? expectInternalRPCError : useDefaultCallback;
+    }


Can we rename this to rejectInternalErrorIf?

thejohnfreeman · 2024-11-08T01:26:26Z

src/test/jtx/Env.h

@@ -514,7 +536,7 @@ class Env
    /** Gets the TER result and `didApply` flag from a RPC Json result object.
     */
    static ParsedResult
-    parseResult(Json::Value const& jr);
+    parseResult(Json::Value const& jr, bool checkTER);


I don't quite understand what this parameter is supposed to be doing. Can we add an explanation in the docstring?

A few more checks, and I was able to get rid of checkTER entirely. The intention was to flag whether the request was related to submitting a transaction, and thus a TER would be expected in the result. There is no TER in the result if there isn't a transaction.

* upstream/develop: Set version to 2.3.0-rc1 Replace Uint192 with Hash192 in server_definitions response (5177) Fix potential deadlock (5124) Introduce Credentials support (XLS-70d): (5103) Fix token comparison in Payment (5172) Add fixAMMv1_2 amendment (5176)

* Rename the RpcCallback type, and the default variables of that type * Move the "default callback" out of the retry logic, and into the variable definition. * Get rid of the checkTER parameter to parseResult.

* upstream/develop: fix: include `index` in `server_definitions` RPC (5190) Fix ledger_entry crash on invalid credentials request (5189)

ximinez · 2024-11-13T21:48:29Z

@thejohnfreeman @vlntb I want to bring your attention to draft PR #5180. It builds on top of this PR, but adds three commits:

The MacOS CI jobs have 4 configurations: The combinations of release vs debug, and unity vs non-unity.
The Env tests cycle through a range of ports so that if the OS is slow to close a port when one test ends, the next test doesn't immediately try to open it. A general retry operation is added so that lower-level network issues can also be retried, not just failed RPC commands.
The MacOS CI jobs use parallel runners! (It only uses half the available CPU threads, because using all was still not reliable, but it's better than nothing!)

I would like to roll those changes into this PR so we can get the MacOS jobs fixed all in one shot. Do you think that's a good idea, or should that be a separate PR to be merged separately sometime after this one?

* upstream/develop: Set version to 2.3.0-rc2

thejohnfreeman · 2024-11-13T22:00:04Z

I don't see any approvals in here yet, so I say roll them into this PR.

* Add a lot more retries across test network operations. * Add some network error logging details.

ximinez force-pushed the test_retry branch from 654b649 to 8024b5d Compare October 30, 2024 23:52

test: Retry all RPC commands to fix MacOS CI jobs

1150626

* Follow up to XRPLF#5120 (23991c9), which added a retry for submit commands. It improved MacOS test reliability, but other tests are failing now. * Retry all failed RPC connections / commands in unit tests.

ximinez force-pushed the test_retry branch from 8024b5d to 1150626 Compare October 31, 2024 01:37

ximinez changed the title ~~DRAFT: test: Retry all RPC commands to fix MacOS CI jobs~~ test: Retry all RPC commands to fix MacOS CI jobs Oct 31, 2024

ximinez marked this pull request as ready for review October 31, 2024 22:11

ximinez requested review from thejohnfreeman and vlntb October 31, 2024 23:04

ximinez assigned thejohnfreeman and vlntb Oct 31, 2024

ximinez added Tech Debt Non-urgent improvements Perf impact not expected Change is not expected to improve nor harm performance. labels Oct 31, 2024

ximinez added 4 commits November 1, 2024 18:54

Merge remote-tracking branch 'upstream/develop' into test_retry

38a290d

* upstream/develop: Add hubs.xrpkuwait.com to bootstrap (5169)

Merge remote-tracking branch 'upstream/develop' into test_retry

97a5be0

* upstream/develop: Add AMMClawback Transaction (XLS-0073d) (5142)

Merge remote-tracking branch 'upstream/develop' into test_retry

75130e9

* upstream/develop: Fix unity build (5179)

Also handle timeout exceptions

205fe37

thejohnfreeman requested changes Nov 8, 2024

View reviewed changes

ximinez added 3 commits November 8, 2024 14:26

[FOLD] Review feedback from @thejohnfreeman:

63768ce

* Rename the RpcCallback type, and the default variables of that type * Move the "default callback" out of the retry logic, and into the variable definition. * Get rid of the checkTER parameter to parseResult.

Merge remote-tracking branch 'upstream/develop' into test_retry

3f9a7ad

* upstream/develop: fix: include `index` in `server_definitions` RPC (5190) Fix ledger_entry crash on invalid credentials request (5189)

Merge remote-tracking branch 'upstream/develop' into test_retry

58e2652

* upstream/develop: Set version to 2.3.0-rc2

ximinez added 3 commits November 13, 2024 18:33

Add MacOS CI configurations: Debug, unity

e66d2e1

Cycle through more test ports to avoid slow close collisions:

315b7dd

* Add a lot more retries across test network operations. * Add some network error logging details.

Run MacOS tests with multiple jobs

7f74c46

This was referenced Nov 13, 2024

DRAFT Wip/5171 #5180

Closed

Try to fix macos unit tests #5192

Draft

Include endpoint info in retry messages

34d918a

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: Retry all RPC commands to fix MacOS CI jobs #5171

test: Retry all RPC commands to fix MacOS CI jobs #5171

ximinez commented Oct 30, 2024 •

edited

Loading

codecov bot commented Oct 31, 2024 •

edited

Loading

thejohnfreeman Nov 8, 2024

ximinez Nov 12, 2024

thejohnfreeman Nov 8, 2024

ximinez Nov 12, 2024

thejohnfreeman Nov 8, 2024

ximinez Nov 12, 2024

thejohnfreeman Nov 8, 2024

thejohnfreeman Nov 8, 2024

ximinez Nov 12, 2024

ximinez commented Nov 13, 2024

thejohnfreeman commented Nov 13, 2024

test: Retry all RPC commands to fix MacOS CI jobs #5171

Are you sure you want to change the base?

test: Retry all RPC commands to fix MacOS CI jobs #5171

Conversation

ximinez commented Oct 30, 2024 • edited Loading

High Level Overview of Change

Context of Change

Type of Change

codecov bot commented Oct 31, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ximinez commented Nov 13, 2024

thejohnfreeman commented Nov 13, 2024

ximinez commented Oct 30, 2024 •

edited

Loading

codecov bot commented Oct 31, 2024 •

edited

Loading