Skip to content

Commit

Permalink
test: Retry all RPC commands to fix MacOS CI jobs
Browse files Browse the repository at this point in the history 
* 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.
  • Loading branch information
@ximinez
ximinez committed Oct 31, 2024
1 parent 0d887ad commit 1150626
Show file tree
Hide file tree
Showing 8 changed files with 130 additions and 32 deletions.
2 changes: 1 addition & 1 deletion include/xrpl/protocol/TxFlags.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
1 change: 1 addition & 0 deletions src/test/app/MultiSign_test.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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];
Expand Down
49 changes: 48 additions & 1 deletion src/test/jtx/Env.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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&) {
return true;
};
rpcCallback
getInternalFailureCallback(bool shouldFail)
{
return shouldFail ? expectInternalRPCError : useDefaultCallback;
}

template <class... Args>
Json::Value
rpc(unsigned apiVersion,
Expand All @@ -303,12 +314,23 @@ class Env
Json::Value
rpc(unsigned apiVersion, std::string const& cmd, Args&&... args);

template <class... Args>
Json::Value
rpc(rpcCallback cb,
std::unordered_map<std::string, std::string> const& headers,
std::string const& cmd,
Args&&... args);

template <class... Args>
Json::Value
rpc(std::unordered_map<std::string, std::string> const& headers,
std::string const& cmd,
Args&&... args);

template <class... Args>
Json::Value
rpc(rpcCallback cb, std::string const& cmd, Args&&... args);

template <class... Args>
Json::Value
rpc(std::string const& cmd, Args&&... args);
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -696,6 +718,7 @@ class Env

Json::Value
do_rpc(
rpcCallback cb,
unsigned apiVersion,
std::vector<std::string> const& args,
std::unordered_map<std::string, std::string> const& headers = {});
Expand Down Expand Up @@ -746,6 +769,7 @@ Env::rpc(
Args&&... args)
{
return do_rpc(
useDefaultCallback,
apiVersion,
std::vector<std::string>{cmd, std::forward<Args>(args)...},
headers);
Expand All @@ -765,16 +789,39 @@ Env::rpc(unsigned apiVersion, std::string const& cmd, Args&&... args)
template <class... Args>
Json::Value
Env::rpc(
rpcCallback cb,
std::unordered_map<std::string, std::string> const& headers,
std::string const& cmd,
Args&&... args)
{
return do_rpc(
cb,
RPC::apiCommandLineVersion,
std::vector<std::string>{cmd, std::forward<Args>(args)...},
headers);
}

template <class... Args>
Json::Value
Env::rpc(
std::unordered_map<std::string, std::string> const& headers,
std::string const& cmd,
Args&&... args)
{
return rpc(useDefaultCallback, headers, cmd, std::forward<Args>(args)...);
}

template <class... Args>
Json::Value
Env::rpc(rpcCallback cb, std::string const& cmd, Args&&... args)
{
return rpc(
cb,
std::unordered_map<std::string, std::string>(),
cmd,
std::forward<Args>(args)...);
}

template <class... Args>
Json::Value
Env::rpc(std::string const& cmd, Args&&... args)
Expand Down
66 changes: 46 additions & 20 deletions src/test/jtx/impl/Env.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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);

Expand All @@ -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
{
Expand Down Expand Up @@ -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);

Expand Down Expand Up @@ -562,12 +563,37 @@ Env::ust(JTx const& jt)

Json::Value
Env::do_rpc(
rpcCallback cb,
unsigned apiVersion,
std::vector<std::string> const& args,
std::unordered_map<std::string, std::string> 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
Expand Down
35 changes: 28 additions & 7 deletions src/test/rpc/LedgerRPC_test.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.");
Expand Down Expand Up @@ -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.");
Expand All @@ -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.");
Expand All @@ -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.");
Expand All @@ -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.");
Expand All @@ -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.");
Expand All @@ -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.");
Expand Down
3 changes: 2 additions & 1 deletion src/test/rpc/LedgerRequestRPC_test.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
3 changes: 2 additions & 1 deletion src/test/rpc/ValidatorInfo_test.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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());
}

Expand Down
3 changes: 2 additions & 1 deletion src/test/rpc/ValidatorRPC_test.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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));
Expand Down

0 comments on commit 1150626

Please sign in to comment.