From 1150626511fc748c2e63996037ea6d90a9c375bb Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Wed, 30 Oct 2024 18:08:45 -0400 Subject: [PATCH] test: Retry all RPC commands to fix MacOS CI jobs * Follow up to #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. --- include/xrpl/protocol/TxFlags.h | 2 +- src/test/app/MultiSign_test.cpp | 1 + src/test/jtx/Env.h | 49 ++++++++++++++++++- src/test/jtx/impl/Env.cpp | 66 ++++++++++++++++++-------- src/test/rpc/LedgerRPC_test.cpp | 35 +++++++++++--- src/test/rpc/LedgerRequestRPC_test.cpp | 3 +- src/test/rpc/ValidatorInfo_test.cpp | 3 +- src/test/rpc/ValidatorRPC_test.cpp | 3 +- 8 files changed, 130 insertions(+), 32 deletions(-) diff --git a/include/xrpl/protocol/TxFlags.h b/include/xrpl/protocol/TxFlags.h index 4894f48a7f9..c83f2a964a8 100644 --- a/include/xrpl/protocol/TxFlags.h +++ b/include/xrpl/protocol/TxFlags.h @@ -134,7 +134,7 @@ constexpr std::uint32_t const tfTrustLine = 0x00000004; constexpr std::uint32_t const tfTransferable = 0x00000008; // MPTokenIssuanceCreate flags: -// NOTE - there is intentionally no flag here for lsfMPTLocked, which this transaction cannot mutate. +// NOTE - there is intentionally no flag here for lsfMPTLocked, which this transaction cannot mutate. constexpr std::uint32_t const tfMPTCanLock = lsfMPTCanLock; constexpr std::uint32_t const tfMPTRequireAuth = lsfMPTRequireAuth; constexpr std::uint32_t const tfMPTCanEscrow = lsfMPTCanEscrow; diff --git a/src/test/app/MultiSign_test.cpp b/src/test/app/MultiSign_test.cpp index 77d85d9011b..94d3d3ee11d 100644 --- a/src/test/app/MultiSign_test.cpp +++ b/src/test/app/MultiSign_test.cpp @@ -644,6 +644,7 @@ class MultiSign_test : public beast::unit_test::suite Json::Value jv_submit; jv_submit[jss::tx_json] = jrr[jss::tx_json]; jrr = env.rpc( + env.expectInternalRPCError, "json", "submit_multisigned", to_string(jv_submit))[jss::result]; diff --git a/src/test/jtx/Env.h b/src/test/jtx/Env.h index d90d2bc1228..e971cecc166 100644 --- a/src/test/jtx/Env.h +++ b/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; + const rpcCallback useDefaultCallback{}; + const rpcCallback expectInternalRPCError = [](Json::Value const&) { + return true; + }; + rpcCallback + getInternalFailureCallback(bool shouldFail) + { + return shouldFail ? expectInternalRPCError : useDefaultCallback; + } + template Json::Value rpc(unsigned apiVersion, @@ -303,12 +314,23 @@ class Env Json::Value rpc(unsigned apiVersion, std::string const& cmd, Args&&... args); + template + Json::Value + rpc(rpcCallback cb, + std::unordered_map const& headers, + std::string const& cmd, + Args&&... args); + template Json::Value rpc(std::unordered_map const& headers, std::string const& cmd, Args&&... args); + template + Json::Value + rpc(rpcCallback cb, std::string const& cmd, Args&&... args); + template Json::Value rpc(std::string const& cmd, Args&&... args); @@ -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); /** Submit an existing JTx. This calls postconditions. @@ -696,6 +718,7 @@ class Env Json::Value do_rpc( + rpcCallback cb, unsigned apiVersion, std::vector const& args, std::unordered_map const& headers = {}); @@ -746,6 +769,7 @@ Env::rpc( Args&&... args) { return do_rpc( + useDefaultCallback, apiVersion, std::vector{cmd, std::forward(args)...}, headers); @@ -765,16 +789,39 @@ Env::rpc(unsigned apiVersion, std::string const& cmd, Args&&... args) template Json::Value Env::rpc( + rpcCallback cb, std::unordered_map const& headers, std::string const& cmd, Args&&... args) { return do_rpc( + cb, RPC::apiCommandLineVersion, std::vector{cmd, std::forward(args)...}, headers); } +template +Json::Value +Env::rpc( + std::unordered_map const& headers, + std::string const& cmd, + Args&&... args) +{ + return rpc(useDefaultCallback, headers, cmd, std::forward(args)...); +} + +template +Json::Value +Env::rpc(rpcCallback cb, std::string const& cmd, Args&&... args) +{ + return rpc( + cb, + std::unordered_map(), + cmd, + std::forward(args)...); +} + template Json::Value Env::rpc(std::string const& cmd, Args&&... args) diff --git a/src/test/jtx/impl/Env.cpp b/src/test/jtx/impl/Env.cpp index ef5a2124e24..ba1c697ce2b 100644 --- a/src/test/jtx/impl/Env.cpp +++ b/src/test/jtx/impl/Env.cpp @@ -273,7 +273,7 @@ Env::trust(STAmount const& amount, Account const& account) } Env::ParsedResult -Env::parseResult(Json::Value const& jr) +Env::parseResult(Json::Value const& jr, bool checkTER) { auto error = [](ParsedResult& parsed, Json::Value const& object) { // Use an error code that is not used anywhere in the transaction @@ -296,14 +296,19 @@ Env::parseResult(Json::Value const& jr) if (jr.isObject() && jr.isMember(jss::result)) { auto const& result = jr[jss::result]; - if (result.isMember(jss::engine_result_code)) + if (checkTER && result.isMember(jss::engine_result_code)) { parsed.ter = TER::fromInt(result[jss::engine_result_code].asInt()); parsed.rpcCode.emplace(rpcSUCCESS); } + else if (!checkTER && !result.isMember(jss::error)) + parsed.rpcCode.emplace(rpcSUCCESS); else error(parsed, result); } + else if ( + jr.isObject() && jr.isMember(jss::error) && jr[jss::error].isObject()) + error(parsed, jr[jss::error]); else error(parsed, jr); @@ -317,24 +322,20 @@ Env::submit(JTx const& jt) auto const jr = [&]() { if (jt.stx) { - // We shouldn't need to retry, but it fixes the test on macOS for - // the moment. - int retries = 3; - do - { - txid_ = jt.stx->getTransactionID(); - Serializer s; - jt.stx->add(s); - auto const jr = rpc("submit", strHex(s.slice())); - - parsedResult = parseResult(jr); + txid_ = jt.stx->getTransactionID(); + Serializer s; + jt.stx->add(s); + auto const cb = [&](Json::Value const& jr) { + parsedResult = parseResult(jr, true); test.expect(parsedResult.ter, "ter uninitialized!"); ter_ = parsedResult.ter.value_or(telENV_RPC_FAILED); - if (ter_ != telENV_RPC_FAILED || + return ( + ter_ != telENV_RPC_FAILED || parsedResult.rpcCode != rpcINTERNAL || - jt.ter == telENV_RPC_FAILED || --retries <= 0) - return jr; - } while (true); + jt.ter == telENV_RPC_FAILED); + }; + // rpc() will call cb(), which does all the parsing + return rpc(cb, "submit", strHex(s.slice())); } else { @@ -379,7 +380,7 @@ Env::sign_and_submit(JTx const& jt, Json::Value params) if (!txid_.parseHex(jr[jss::result][jss::tx_json][jss::hash].asString())) txid_.zero(); - ParsedResult const parsedResult = parseResult(jr); + ParsedResult const parsedResult = parseResult(jr, true); test.expect(parsedResult.ter, "ter uninitialized!"); ter_ = parsedResult.ter.value_or(telENV_RPC_FAILED); @@ -562,12 +563,37 @@ Env::ust(JTx const& jt) Json::Value Env::do_rpc( + rpcCallback cb, unsigned apiVersion, std::vector const& args, std::unordered_map const& headers) { - return rpcClient(args, app().config(), app().logs(), apiVersion, headers) - .second; + // We shouldn't need to retry, but it fixes the test on macOS for + // the moment. + if (!cb) + { + static auto const defaultCallback = [](Json::Value const& jr) { + auto const parsedResult = parseResult(jr, false); + return parsedResult.rpcCode != rpcINTERNAL; + }; + cb = defaultCallback; + } + int retries = 3; + do + { + auto const ret = + rpcClient(args, app().config(), app().logs(), apiVersion, headers) + .second; + if (cb(ret) || --retries <= 0) + return ret; + std::stringstream ss; + for (auto const& arg : args) + { + ss << arg << ", "; + } + test.log << "RPC failed: " << ss.str() << " -> " << to_string(ret) + << std::endl; + } while (true); } void diff --git a/src/test/rpc/LedgerRPC_test.cpp b/src/test/rpc/LedgerRPC_test.cpp index 792da88b5bc..37ed82985db 100644 --- a/src/test/rpc/LedgerRPC_test.cpp +++ b/src/test/rpc/LedgerRPC_test.cpp @@ -1638,7 +1638,10 @@ class LedgerRPC_test : public beast::unit_test::suite }); Json::Value const jrr = env.rpc( - "json", "ledger_entry", to_string(jvParams))[jss::result]; + env.getInternalFailureCallback(apiVersion < 2u), + "json", + "ledger_entry", + to_string(jvParams))[jss::result]; if (apiVersion < 2u) checkErrorValue(jrr, "internal", "Internal error."); @@ -1675,7 +1678,10 @@ class LedgerRPC_test : public beast::unit_test::suite }); Json::Value const jrr = env.rpc( - "json", "ledger_entry", to_string(jvParams))[jss::result]; + env.getInternalFailureCallback(apiVersion < 2u), + "json", + "ledger_entry", + to_string(jvParams))[jss::result]; if (apiVersion < 2u) checkErrorValue(jrr, "internal", "Internal error."); @@ -1691,7 +1697,10 @@ class LedgerRPC_test : public beast::unit_test::suite }); Json::Value const jrr = env.rpc( - "json", "ledger_entry", to_string(jvParams))[jss::result]; + env.getInternalFailureCallback(apiVersion < 2u), + "json", + "ledger_entry", + to_string(jvParams))[jss::result]; if (apiVersion < 2u) checkErrorValue(jrr, "internal", "Internal error."); @@ -1707,7 +1716,10 @@ class LedgerRPC_test : public beast::unit_test::suite }); Json::Value const jrr = env.rpc( - "json", "ledger_entry", to_string(jvParams))[jss::result]; + env.getInternalFailureCallback(apiVersion < 2u), + "json", + "ledger_entry", + to_string(jvParams))[jss::result]; if (apiVersion < 2u) checkErrorValue(jrr, "internal", "Internal error."); @@ -1723,7 +1735,10 @@ class LedgerRPC_test : public beast::unit_test::suite }); Json::Value const jrr = env.rpc( - "json", "ledger_entry", to_string(jvParams))[jss::result]; + env.getInternalFailureCallback(apiVersion < 2u), + "json", + "ledger_entry", + to_string(jvParams))[jss::result]; if (apiVersion < 2u) checkErrorValue(jrr, "internal", "Internal error."); @@ -1746,7 +1761,10 @@ class LedgerRPC_test : public beast::unit_test::suite }); Json::Value const jrr = env.rpc( - "json", "ledger_entry", to_string(jvParams))[jss::result]; + env.getInternalFailureCallback(apiVersion < 2u), + "json", + "ledger_entry", + to_string(jvParams))[jss::result]; if (apiVersion < 2u) checkErrorValue(jrr, "internal", "Internal error."); @@ -1762,7 +1780,10 @@ class LedgerRPC_test : public beast::unit_test::suite }); Json::Value const jrr = env.rpc( - "json", "ledger_entry", to_string(jvParams))[jss::result]; + env.getInternalFailureCallback(apiVersion < 2u), + "json", + "ledger_entry", + to_string(jvParams))[jss::result]; if (apiVersion < 2u) checkErrorValue(jrr, "internal", "Internal error."); diff --git a/src/test/rpc/LedgerRequestRPC_test.cpp b/src/test/rpc/LedgerRequestRPC_test.cpp index 8922cd38386..689447df817 100644 --- a/src/test/rpc/LedgerRequestRPC_test.cpp +++ b/src/test/rpc/LedgerRequestRPC_test.cpp @@ -347,7 +347,8 @@ class LedgerRequestRPC_test : public beast::unit_test::suite auto const USD = gw["USD"]; env.fund(XRP(100000), gw); - auto const result = env.rpc("ledger_request", "1")[jss::result]; + auto const result = env.rpc( + env.expectInternalRPCError, "ledger_request", "1")[jss::result]; // The current HTTP/S ServerHandler returns an HTTP 403 error code here // rather than a noPermission JSON error. The JSONRPCClient just eats // that error and returns an null result. diff --git a/src/test/rpc/ValidatorInfo_test.cpp b/src/test/rpc/ValidatorInfo_test.cpp index 603a0ad9d23..1b94e4d9812 100644 --- a/src/test/rpc/ValidatorInfo_test.cpp +++ b/src/test/rpc/ValidatorInfo_test.cpp @@ -50,7 +50,8 @@ class ValidatorInfo_test : public beast::unit_test::suite { using namespace test::jtx; Env env{*this, envconfig(no_admin)}; - auto const info = env.rpc("validator_info")[jss::result]; + auto const info = + env.rpc(env.expectInternalRPCError, "validator_info")[jss::result]; BEAST_EXPECT(info.isNull()); } diff --git a/src/test/rpc/ValidatorRPC_test.cpp b/src/test/rpc/ValidatorRPC_test.cpp index 2bd4b69c37b..0c842d54dc2 100644 --- a/src/test/rpc/ValidatorRPC_test.cpp +++ b/src/test/rpc/ValidatorRPC_test.cpp @@ -49,7 +49,8 @@ class ValidatorRPC_test : public beast::unit_test::suite for (std::string cmd : {"validators", "validator_list_sites"}) { Env env{*this, isAdmin ? envconfig() : envconfig(no_admin)}; - auto const jrr = env.rpc(cmd)[jss::result]; + auto const jrr = env.rpc( + env.getInternalFailureCallback(!isAdmin), cmd)[jss::result]; if (isAdmin) { BEAST_EXPECT(!jrr.isMember(jss::error));