Introduce TestEnvBuilder structure

[mina86]

Replace TestEnv constructors with a TestEnvBuilder structure instead.
Motivation for this change are twofold. First of all, with Rust’s
lack of default or keyword arguments constructors of TestEnv would
just continue to multiply with more and more arguments. Second of
all, with the current design whoever uses TestEnv structure must
adhere to test{} AccountId format which may not always be desired.

With this commit also change three instances where
TestEnv::new_with_runtime_and_network_adapter was used by the use of
the builder. Other places where TestEnv constructors are used are
left for now and will be dealt in another commit.

Issue: #4835

[matklad]

Delightful!

[matklad]

Just String?

[mina86]

Yeah. Admittedly this is a vestige of the earlier version of the code. The idea is to let users provide custom formatters (though currently that feature is not available) and in that case it seems that the typical use case will be to call format! which returns a String. I therefore moved converting the string to AccountId inside of the builder. This could also by anything implementing TryInto<AccountId> but that adds more generic types which I didn’t care to define.

[mina86]

Ah, wait, you just meant to use String i/o std::string::String. Will do in a follow up commit.

[matklad]

We always use default_formatter, so I'd just YAGNI this.

If the user wants to customize it, i'd let them provide the actual names, rather than a formatter.

[mina86]

In early version of this code there was a clients_with_formatter function which took number of clients and a formatter function. While it’s true that with clients method where the vector can be provided directly support for custom formatters is unnecessary but it seems that the most common case will be to to simply use clients_count(number) and the second most common will be to use clients_with_formatter(number, |id| format("blah{}", id). Perhaps it’s clients method that we’re not gonna need?

[matklad]

the second most common will be to use clients_with_formatter(number, |id| format("blah{}", id)

Why would this be common? It seems that there shouldn't be a need to customize the names, outside of the special cases like #4771, where overriding the whole thing should be enough.

I feel clients is needed because it's the most general -- even if we call it zero times, it's important that we can customize this behavior when we suddenly need this for some future investigation. This avoids midlayer mistake -- providing convenience API without exposing the full capabilities of the underlying platform.

Playing around this ideas, I got the following:

diff --git a/chain/client/src/test_utils.rs b/chain/client/src/test_utils.rs
index 723584675..9d97d3ece 100644
--- a/chain/client/src/test_utils.rs
+++ b/chain/client/src/test_utils.rs
@@ -1117,11 +1117,13 @@ impl TestEnvBuilder {
     fn new(chain_genesis: ChainGenesis) -> Self {
         Self {
             chain_genesis,
-            clients: Self::make_accounts(1, Self::default_formatter),
-            validators: Self::make_accounts(1, Self::default_formatter),
+            clients: Vec::new(),
+            validators: Vec::new(),
             runtime_adapters: None,
             network_adapters: None,
         }
+        .clients_count(1)
+        .validator_seats(1)
     }
 
     /// Sets list of client `AccountId`s to the one provided.  Panics if the
@@ -1132,10 +1134,11 @@ impl TestEnvBuilder {
         self
     }
 
-    /// Sets number of clients to given one.  Each client will use `AccountId`
-    /// in the form `test{}` where `{}` will count from zero.
+    /// Sets number of clients to given one.
     pub fn clients_count(self, num: usize) -> Self {
-        self.clients(Self::make_accounts(num, Self::default_formatter))
+        let clients =
+            (0..num).map(|i| AccountId::try_from(format!("test{}", i)).unwrap()).collect();
+        self.clients(clients)
     }
 
     /// Sets list of validator `AccountId`s to the one provided.  Panics if the
@@ -1146,10 +1149,10 @@ impl TestEnvBuilder {
         self
     }
 
-    /// Sets number of validator seats to given one.  Each validator will use
-    /// `AccountId` in the form `test{}` where `{}` will count from zero.
+    /// Sets the first `num` accounts as validators.
     pub fn validator_seats(self, num: usize) -> Self {
-        self.validators(Self::make_accounts(num, Self::default_formatter))
+        let validators = self.clients[..num].to_vec();
+        self.validators(validators)
     }
 
     /// Specifies custom runtime adaptors for each client.  This allows us to
@@ -1229,17 +1232,6 @@ impl TestEnvBuilder {
 
         TestEnv { chain_genesis, validators, network_adapters, clients }
     }
-
-    fn default_formatter(id: usize) -> std::string::String {
-        format!("test{}", id)
-    }
-
-    fn make_accounts<F>(count: usize, formatter: F) -> Vec<AccountId>
-    where
-        F: Fn(usize) -> std::string::String,
-    {
-        (0..count).map(|i| AccountId::try_from(formatter(i)).unwrap()).collect()
-    }
 }
 
 impl TestEnv {

I've almo made a couple observations while playing with it:

• there's slight naming inconsistensy where we use count, num, and seats
• not sure if we should document the exact format of test account naming. Seems better to provide accessor to resulting AccountId structs. Reliance on specific names is exactly what caused the original issues with different names not working.

[mina86]

You’re not wrong and my assertion isn’t really supported by hard data. On the other hand, we have no examples of tests where we use account ids which don’t follow simple pattern while we have an example of test which wants to change the pattern. ;)

I’m also not exactly convinced that we should be worried about the midlayer mistake. We can have both clients and clients_with_formatter. While that ‘bloats’ the builder, I think it’s acceptable for a helper test code whose whole purpose is to provide convenience.

there's slight naming inconsistensy where we use count, num, and seats

I was going back and forth between validator_seats and validators_count. I don’t really have any good reasons for either to be honest. I first went with count but then saw bunch of code use seats. Don’t really have an opinion.

Seems better to provide accessor to resulting AccountId structs.

Yes, I've been looking into that as well though I think such accessor belongs to TestEnv rather than the builder.

PS. Note that there are tests where number of validator seats is greater than number of clients, see test-utils/state-viewer/src/state_dump.rs so validators = self.clients[..num].to_vec(); is sadly not valid.