Fix CI unit tests (#5196)

- Add retries for rpc client
- Add dynamic port allocation for rpc servers

src/test/app/LedgerReplay_test.cpp

@@ -583,10 +583,7 @@ public:
         PeerSetBehavior behavior = PeerSetBehavior::Good,
         InboundLedgersBehavior inboundBhvr = InboundLedgersBehavior::Good,
         PeerFeature peerFeature = PeerFeature::LedgerReplayEnabled)
-        : env(suite,
-              jtx::envconfig(jtx::port_increment, 3),
-              nullptr,
-              beast::severities::kDisabled)
+        : env(suite, jtx::envconfig(), nullptr, beast::severities::kDisabled)
         , app(env.app())
         , ledgerMaster(env.app().getLedgerMaster())
         , inboundLedgers(

src/test/app/XChain_test.cpp

@@ -201,11 +201,7 @@ struct SEnv
 template <class T>
 struct XEnv : public jtx::XChainBridgeObjects, public SEnv<T>
 {
-    XEnv(T& s, bool side = false)
-        : SEnv<T>(
-              s,
-              jtx::envconfig(jtx::port_increment, side ? 3 : 0),
-              features)
+    XEnv(T& s, bool side = false) : SEnv<T>(s, jtx::envconfig(), features)
     {
         using namespace jtx;
         STAmount xrp_funds{XRP(10000)};

src/test/core/Config_test.cpp

@@ -24,10 +24,13 @@
 #include <xrpl/basics/contract.h>
 #include <xrpl/beast/unit_test/suite.h>
 #include <xrpl/server/Port.h>
+
 #include <boost/filesystem.hpp>
 #include <boost/format.hpp>
+
 #include <fstream>
 #include <iostream>
+#include <regex>
 
 namespace ripple {
 namespace detail {
@@ -140,12 +143,15 @@ public:
         path subDir,
         path const& dbPath,
         path const& validatorsFile,
-        bool useCounter = true)
+        bool useCounter = true,
+        std::string confContents = "")
         : FileDirGuard(
               test,
               std::move(subDir),
               path(Config::configFileName),
-              configContents(dbPath.string(), validatorsFile.string()),
+              confContents.empty()
+                  ? configContents(dbPath.string(), validatorsFile.string())
+                  : confContents,
               useCounter)
         , dataDir_(dbPath)
     {
@@ -1068,6 +1074,27 @@ trustthesevalidators.gov
         BEAST_EXPECT(wss.admin_nets_v4.size() + wss.admin_nets_v6.size() == 1);
     }
 
+    void
+    testZeroPort()
+    {
+        auto const contents = std::regex_replace(
+            detail::configContents("", ""),
+            std::regex("port\\s*=\\s*\\d+"),
+            "port = 0");
+
+        try
+        {
+            detail::RippledCfgGuard const cfg(
+                *this, "testPort", "", "", true, contents);
+            BEAST_EXPECT(false);
+        }
+        catch (std::exception const& ex)
+        {
+            BEAST_EXPECT(std::string_view(ex.what()).starts_with(
+                "Invalid value '0' for key 'port'"));
+        }
+    }
+
     void
     testWhitespace()
     {
@@ -1478,6 +1505,7 @@ r.ripple.com:51235
         testSetup(false);
         testSetup(true);
         testPort();
+        testZeroPort();
         testWhitespace();
         testColons();
         testComments();

src/test/jtx/Env.h

@@ -419,6 +419,20 @@ public:
         app().checkSigs(false);
     }
 
+    // set rpc retries
+    void
+    set_retries(unsigned r = 5)
+    {
+        retries_ = r;
+    }
+
+    // get rpc retries
+    unsigned
+    retries() const
+    {
+        return retries_;
+    }
+
     /** Associate AccountID with account. */
     void
     memoize(Account const& account);
@@ -700,6 +714,7 @@ protected:
     uint256 txid_;
     TER ter_ = tesSUCCESS;
     bool parseFailureExpected_ = false;
+    unsigned retries_ = 5;
 
     Json::Value
     do_rpc(

src/test/jtx/envconfig.h

@@ -106,19 +106,6 @@ std::unique_ptr<Config> secure_gateway_localnet(std::unique_ptr<Config>);
 std::unique_ptr<Config>
 validator(std::unique_ptr<Config>, std::string const&);
 
-/// @brief adjust the default configured server ports by a specified value
-///
-/// This is intended for use with envconfig, as in
-/// envconfig(port_increment, 5)
-///
-/// @param cfg config instance to be modified
-/// @param int amount by which to increment the existing server port
-/// values in the config
-///
-/// @return unique_ptr to Config instance
-std::unique_ptr<Config>
-port_increment(std::unique_ptr<Config>, int);
-
 /// @brief add a grpc address and port to config
 ///
 /// This is intended for use with envconfig, for tests that require a grpc

src/test/jtx/impl/Env.cpp

@@ -326,24 +326,16 @@ 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()));
+            txid_ = jt.stx->getTransactionID();
+            Serializer s;
+            jt.stx->add(s);
+            auto const jr = rpc("submit", strHex(s.slice()));
 
-                parsedResult = parseResult(jr);
-                test.expect(parsedResult.ter, "ter uninitialized!");
-                ter_ = parsedResult.ter.value_or(telENV_RPC_FAILED);
-                if (ter_ != telENV_RPC_FAILED ||
-                    parsedResult.rpcCode != rpcINTERNAL ||
-                    jt.ter == telENV_RPC_FAILED || --retries <= 0)
-                    return jr;
-            } while (true);
+            parsedResult = parseResult(jr);
+            test.expect(parsedResult.ter, "ter uninitialized!");
+            ter_ = parsedResult.ter.value_or(telENV_RPC_FAILED);
+
+            return jr;
         }
         else
         {
@@ -576,8 +568,21 @@ Env::do_rpc(
     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;
+    auto response =
+        rpcClient(args, app().config(), app().logs(), apiVersion, headers);
+
+    for (unsigned ctr = 0; (ctr < retries_) and (response.first == rpcINTERNAL);
+         ++ctr)
+    {
+        JLOG(journal.error())
+            << "Env::do_rpc error, retrying, attempt #" << ctr + 1 << " ...";
+        std::this_thread::sleep_for(std::chrono::milliseconds(500));
+
+        response =
+            rpcClient(args, app().config(), app().logs(), apiVersion, headers);
+    }
+
+    return response.second;
 }
 
 void

src/test/jtx/impl/JSONRPCClient.cpp

@@ -52,6 +52,10 @@ class JSONRPCClient : public AbstractClient
             if (pp.ip && pp.ip->is_unspecified())
                 *pp.ip = pp.ip->is_v6() ? address{address_v6::loopback()}
                                         : address{address_v4::loopback()};
+
+            if (!pp.port)
+                Throw<std::runtime_error>("Use fixConfigPorts with auto ports");
+
             return {*pp.ip, *pp.port};
         }
         Throw<std::runtime_error>("Missing HTTP port");

src/test/jtx/impl/WSClient.cpp

@@ -69,6 +69,10 @@ class WSClientImpl : public WSClient
             if (pp.ip && pp.ip->is_unspecified())
                 *pp.ip = pp.ip->is_v6() ? address{address_v6::loopback()}
                                         : address{address_v4::loopback()};
+
+            if (!pp.port)
+                Throw<std::runtime_error>("Use fixConfigPorts with auto ports");
+
             return {*pp.ip, *pp.port};
         }
         Throw<std::runtime_error>("Missing WebSocket port");

src/test/jtx/impl/envconfig.cpp

@@ -25,22 +25,11 @@
 namespace ripple {
 namespace test {
 
-int port_base = 8000;
-void
-incPorts(int times)
-{
-    port_base += (4 * times);
-}
-
 std::atomic<bool> envUseIPv4{false};
 
 void
 setupConfigForUnitTests(Config& cfg)
 {
-    std::string const port_peer = std::to_string(port_base);
-    std::string port_rpc = std::to_string(port_base + 1);
-    std::string port_ws = std::to_string(port_base + 2);
-
     using namespace jtx;
     // Default fees to old values, so tests don't have to worry about changes in
     // Config.h
@@ -58,19 +47,25 @@ setupConfigForUnitTests(Config& cfg)
     cfg.setupControl(true, true, true);
     cfg["server"].append(PORT_PEER);
     cfg[PORT_PEER].set("ip", getEnvLocalhostAddr());
-    cfg[PORT_PEER].set("port", port_peer);
+
+    // Using port 0 asks the operating system to allocate an unused port, which
+    // can be obtained after a "bind" call.
+    // Works for all system (Linux, Windows, Unix, Mac).
+    // Check https://man7.org/linux/man-pages/man7/ip.7.html
+    // "ip_local_port_range" section for more info
+    cfg[PORT_PEER].set("port", "0");
     cfg[PORT_PEER].set("protocol", "peer");
 
     cfg["server"].append(PORT_RPC);
     cfg[PORT_RPC].set("ip", getEnvLocalhostAddr());
     cfg[PORT_RPC].set("admin", getEnvLocalhostAddr());
-    cfg[PORT_RPC].set("port", port_rpc);
+    cfg[PORT_RPC].set("port", "0");
     cfg[PORT_RPC].set("protocol", "http,ws2");
 
     cfg["server"].append(PORT_WS);
     cfg[PORT_WS].set("ip", getEnvLocalhostAddr());
     cfg[PORT_WS].set("admin", getEnvLocalhostAddr());
-    cfg[PORT_WS].set("port", port_ws);
+    cfg[PORT_WS].set("port", "0");
     cfg[PORT_WS].set("protocol", "ws");
     cfg.SSL_VERIFY = false;
 }
@@ -123,27 +118,11 @@ validator(std::unique_ptr<Config> cfg, std::string const& seed)
     return cfg;
 }
 
-std::unique_ptr<Config>
-port_increment(std::unique_ptr<Config> cfg, int increment)
-{
-    for (auto const sectionName : {PORT_PEER, PORT_RPC, PORT_WS})
-    {
-        Section& s = (*cfg)[sectionName];
-        auto const port = s.get<std::int32_t>("port");
-        if (port)
-        {
-            s.set("port", std::to_string(*port + increment));
-        }
-    }
-    return cfg;
-}
-
 std::unique_ptr<Config>
 addGrpcConfig(std::unique_ptr<Config> cfg)
 {
-    std::string port_grpc = std::to_string(port_base + 3);
     (*cfg)[SECTION_PORT_GRPC].set("ip", getEnvLocalhostAddr());
-    (*cfg)[SECTION_PORT_GRPC].set("port", port_grpc);
+    (*cfg)[SECTION_PORT_GRPC].set("port", "0");
     return cfg;
 }
 
@@ -152,9 +131,11 @@ addGrpcConfigWithSecureGateway(
     std::unique_ptr<Config> cfg,
     std::string const& secureGateway)
 {
-    std::string port_grpc = std::to_string(port_base + 3);
     (*cfg)[SECTION_PORT_GRPC].set("ip", getEnvLocalhostAddr());
-    (*cfg)[SECTION_PORT_GRPC].set("port", port_grpc);
+
+    // Check https://man7.org/linux/man-pages/man7/ip.7.html
+    // "ip_local_port_range" section for using 0 ports
+    (*cfg)[SECTION_PORT_GRPC].set("port", "0");
     (*cfg)[SECTION_PORT_GRPC].set("secure_gateway", secureGateway);
     return cfg;
 }

src/test/ledger/View_test.cpp

@@ -1012,11 +1012,7 @@ class View_test : public beast::unit_test::suite
 
         // The two Env's can't share the same ports, so modify the config
         // of the second Env to use higher port numbers
-        Env eB{
-            *this,
-            envconfig(port_increment, 3),
-            nullptr,
-            beast::severities::kDisabled};
+        Env eB{*this, envconfig(), nullptr, beast::severities::kDisabled};
 
         // Make ledgers that are incompatible with the first ledgers.  Note
         // that bob is funded before alice.

src/test/rpc/AccountObjects_test.cpp

@@ -759,7 +759,7 @@ public:
         {
             // Create a bridge
             test::jtx::XChainBridgeObjects x;
-            Env scEnv(*this, envconfig(port_increment, 3), features);
+            Env scEnv(*this, envconfig(), features);
             x.createScBridgeObjects(scEnv);
 
             auto scEnvAcctObjs = [&](Account const& acct, char const* type) {
@@ -800,7 +800,7 @@ public:
             // Alice and Bob create a xchain sequence number that we can look
             // for in the ledger.
             test::jtx::XChainBridgeObjects x;
-            Env scEnv(*this, envconfig(port_increment, 3), features);
+            Env scEnv(*this, envconfig(), features);
             x.createScBridgeObjects(scEnv);
 
             scEnv(
@@ -845,7 +845,7 @@ public:
         }
         {
             test::jtx::XChainBridgeObjects x;
-            Env scEnv(*this, envconfig(port_increment, 3), features);
+            Env scEnv(*this, envconfig(), features);
             x.createScBridgeObjects(scEnv);
             auto const amt = XRP(1000);
 

src/test/rpc/LedgerRPC_test.cpp

@@ -64,7 +64,7 @@ class LedgerRPC_XChain_test : public beast::unit_test::suite,
         using namespace test::jtx;
 
         Env mcEnv{*this, features};
-        Env scEnv(*this, envconfig(port_increment, 3), features);
+        Env scEnv(*this, envconfig(), features);
 
         createBridgeObjects(mcEnv, scEnv);
 
@@ -157,7 +157,7 @@ class LedgerRPC_XChain_test : public beast::unit_test::suite,
         using namespace test::jtx;
 
         Env mcEnv{*this, features};
-        Env scEnv(*this, envconfig(port_increment, 3), features);
+        Env scEnv(*this, envconfig(), features);
 
         createBridgeObjects(mcEnv, scEnv);
 
@@ -218,7 +218,7 @@ class LedgerRPC_XChain_test : public beast::unit_test::suite,
         using namespace test::jtx;
 
         Env mcEnv{*this, features};
-        Env scEnv(*this, envconfig(port_increment, 3), features);
+        Env scEnv(*this, envconfig(), features);
 
         // note: signers.size() and quorum are both 5 in createBridgeObjects
         createBridgeObjects(mcEnv, scEnv);

src/test/rpc/LedgerRequestRPC_test.cpp

@@ -347,6 +347,7 @@ public:
         auto const USD = gw["USD"];
         env.fund(XRP(100000), gw);
 
+        env.set_retries(0);
         auto const result = env.rpc("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

src/test/rpc/ServerInfo_test.cpp

@@ -104,17 +104,17 @@ admin = 127.0.0.1
         }
 
         {
-            auto config = makeValidatorConfig();
-            auto const rpc_port =
-                (*config)["port_rpc"].get<unsigned int>("port");
+            Env env(*this, makeValidatorConfig());
+            auto const& config = env.app().config();
+
+            auto const rpc_port = config["port_rpc"].get<unsigned int>("port");
             auto const grpc_port =
-                (*config)[SECTION_PORT_GRPC].get<unsigned int>("port");
-            auto const ws_port = (*config)["port_ws"].get<unsigned int>("port");
+                config[SECTION_PORT_GRPC].get<unsigned int>("port");
+            auto const ws_port = config["port_ws"].get<unsigned int>("port");
             BEAST_EXPECT(grpc_port);
             BEAST_EXPECT(rpc_port);
             BEAST_EXPECT(ws_port);
 
-            Env env(*this, std::move(config));
             auto const result = env.rpc("server_info");
             BEAST_EXPECT(!result[jss::result].isMember(jss::error));
             BEAST_EXPECT(result[jss::result][jss::status] == "success");

src/test/rpc/Subscribe_test.cpp

@@ -614,7 +614,7 @@ public:
         }
 
         {
-            Env env_nonadmin{*this, no_admin(envconfig(port_increment, 3))};
+            Env env_nonadmin{*this, no_admin(envconfig())};
             Json::Value jv;
             jv[jss::url] = "no-url";
             auto jr =

src/test/rpc/ValidatorInfo_test.cpp

@@ -50,6 +50,7 @@ public:
     {
         using namespace test::jtx;
         Env env{*this, envconfig(no_admin)};
+        env.set_retries(0);
         auto const info = env.rpc("validator_info")[jss::result];
         BEAST_EXPECT(info.isNull());
     }

src/test/rpc/ValidatorRPC_test.cpp

@@ -49,6 +49,7 @@ public:
             for (std::string cmd : {"validators", "validator_list_sites"})
             {
                 Env env{*this, isAdmin ? envconfig() : envconfig(no_admin)};
+                env.set_retries(isAdmin ? 5 : 0);
                 auto const jrr = env.rpc(cmd)[jss::result];
                 if (isAdmin)
                 {

src/test/server/Server_test.cpp

@@ -299,8 +299,8 @@ public:
         serverPort.back().port = 0;
         serverPort.back().protocol.insert("http");
         auto eps = s->ports(serverPort);
-        test_request(eps[0]);
-        test_keepalive(eps[0]);
+        test_request(eps.begin()->second);
+        test_keepalive(eps.begin()->second);
         // s->close();
         s = nullptr;
         pass();
@@ -423,7 +423,20 @@ public:
                 std::make_unique<CaptureLogs>(&messages)};
         });
         BEAST_EXPECT(
-            messages.find("Invalid value '0' for key 'port' in [port_rpc]") !=
+            messages.find("Invalid value '0' for key 'port' in [port_rpc]") ==
+            std::string::npos);
+
+        except([&] {
+            Env env{
+                *this,
+                envconfig([](std::unique_ptr<Config> cfg) {
+                    (*cfg)["server"].set("port", "0");
+                    return cfg;
+                }),
+                std::make_unique<CaptureLogs>(&messages)};
+        });
+        BEAST_EXPECT(
+            messages.find("Invalid value '0' for key 'port' in [server]") !=
             std::string::npos);
 
         except([&] {

src/test/unit_test/multi_runner.cpp

@@ -32,9 +32,6 @@
 namespace ripple {
 namespace test {
 
-extern void
-incPorts(int times);
-
 namespace detail {
 
 std::string
@@ -510,9 +507,6 @@ multi_runner_child::multi_runner_child(
     , quiet_{quiet}
     , print_log_{!quiet || print_log}
 {
-    // incPort twice (2*jobIndex_) because some tests need two envs
-    test::incPorts(2 * job_index_);
-
     if (num_jobs_ > 1)
     {
         keep_alive_thread_ = std::thread([this] {