Repository: paritytech/ink
Pull request #382 (2020-04-24 08:05:50)

Fix single threaded tests in off-chain environment
Currently some tests that rely on interaction with the off-chain test environment share some state when run in a single thread. This PR resolves these conflicts by conditionally resetting the off-chain test environment for every test incarnation if the off-chain test environment has already been initialized at that time.

Tests are normally run in parallel each in their own thread, however, some tools such as `miri` run all tests single threaded.

Tests that have been run using:
`cargo test -- --test-threads=1`

Robbepop: > From your description it is not obvious to me _why_ they share state in a single threaded environment. I guess because you rely on `thread_local!` to separate the state.
> 
> Maybe you can clarify that before merging.
> 
> When you expect every test to have a fresh environment why do not code that straightforward by creating a new instance in every test? I think even when running the tests per default it is not guaranteed that no thread is recycled. At least it is a assumption on something that is too much an implementation detail for my taste.

The problem is that the environment in ink! is a static entity. This is okay because ink! is normally run single threaded (as Wasm contract), however, in the off-chain environment is a bit more complicated.
On the one hand side we want to mimic semantics of the on-chain environment (being static, thus we opted into being `thread_local`) and on the other hand side it might be run in parallel.
Tests in ink! normally use the `run_test` API which now simply performs a reset of the underlying `thread_local` off-chain environment if it has been initialized already. So this is rather straight forward. Since each test is either run single threaded and in sequence of paralell and in their own thread this always works.
athei: > This shouldn't be a problem since then the `thread_local` off-chain environment would simply reset.

I know this is why I gave my approval. I was just wondering why it was done in this more complicated way:  Static instance that is rested instead of just spawning a new one on the stack for every test.

If you say there are good reasons for that it is fine by me.

Patch:
diff --git a/core/src/env/engine/off_chain/db/accounts.rs b/core/src/env/engine/off_chain/db/accounts.rs
index cde4306179..0f35324499 100644
--- a/core/src/env/engine/off_chain/db/accounts.rs
+++ b/core/src/env/engine/off_chain/db/accounts.rs
@@ -78,6 +78,11 @@ impl AccountsDb {
         }
     }
 
+    /// Resets the account DB to uninitialized state.
+    pub fn reset(&mut self) {
+        self.accounts.clear()
+    }
+
     /// Returns the account at the given account ID or creates it.
     pub fn get_or_create_account<T>(&mut self, at: &T::AccountId) -> &mut Account
     where
diff --git a/core/src/env/engine/off_chain/db/chain_spec.rs b/core/src/env/engine/off_chain/db/chain_spec.rs
index 9e98c939d1..aa5315d74c 100644
--- a/core/src/env/engine/off_chain/db/chain_spec.rs
+++ b/core/src/env/engine/off_chain/db/chain_spec.rs
@@ -42,6 +42,14 @@ impl ChainSpec {
         }
     }
 
+    /// Resets the chain spec to uninitialized state.
+    pub fn reset(&mut self) {
+        self.gas_price = OffBalance::uninitialized();
+        self.minimum_balance = OffBalance::uninitialized();
+        self.tombstone_deposit = OffBalance::uninitialized();
+        self.block_time = OffTimestamp::uninitialized();
+    }
+
     /// Default initialization for the off-chain specification.
     pub fn initialize_as_default<T>(&mut self) -> crate::env::Result<()>
     where
diff --git a/core/src/env/engine/off_chain/db/console.rs b/core/src/env/engine/off_chain/db/console.rs
index f19f2c9e14..95c9c813f9 100644
--- a/core/src/env/engine/off_chain/db/console.rs
+++ b/core/src/env/engine/off_chain/db/console.rs
@@ -28,6 +28,11 @@ impl Console {
         }
     }
 
+    /// Resets the console to uninitialized state.
+    pub fn reset(&mut self) {
+        self.past_prints.clear();
+    }
+
     /// Prints the contents to the actual console and stores them.
     pub fn println(&mut self, contents: &str) {
         self.past_prints.push(contents.to_string());
diff --git a/core/src/env/engine/off_chain/db/events.rs b/core/src/env/engine/off_chain/db/events.rs
index 1e652e8091..0774d05ee6 100644
--- a/core/src/env/engine/off_chain/db/events.rs
+++ b/core/src/env/engine/off_chain/db/events.rs
@@ -58,6 +58,11 @@ impl EmittedEventsRecorder {
         }
     }
 
+    /// Resets the emitted events to none.
+    pub fn reset(&mut self) {
+        self.emitted_events.clear();
+    }
+
     /// Records a new emitted event.
     pub fn record<T, E>(&mut self, new_event: E)
     where
diff --git a/core/src/env/engine/off_chain/mod.rs b/core/src/env/engine/off_chain/mod.rs
index ab91134431..ce6f40625d 100644
--- a/core/src/env/engine/off_chain/mod.rs
+++ b/core/src/env/engine/off_chain/mod.rs
@@ -110,6 +110,36 @@ impl EnvInstance {
         }
     }
 
+    /// Returns `true` if the off-chain environment is uninitialized.
+    pub fn is_initialized(&self) -> bool {
+        !self.exec_context.is_empty()
+    }
+
+    /// Either resets or initializes the off-chain environment to default values.
+    pub fn initialize_or_reset_as_default<T>(&mut self) -> crate::env::Result<()>
+    where
+        T: EnvTypes,
+        <T as EnvTypes>::AccountId: From<[u8; 32]>,
+    {
+        if self.is_initialized() {
+            self.reset()
+        }
+        self.initialize_as_default::<T>()?;
+        Ok(())
+    }
+
+    /// Resets the off-chain environment to unintialized state.
+    pub fn reset(&mut self) {
+        self.accounts.reset();
+        self.exec_context.clear();
+        self.chain_spec.reset();
+        self.blocks.clear();
+        self.console.reset();
+        self.runtime_storage.reset();
+        self.runtime_call_handler.reset();
+        self.emitted_events.reset();
+    }
+
     /// Initializes the whole off-chain environment.
     ///
     /// # Note
diff --git a/core/src/env/engine/off_chain/runtime_calls.rs b/core/src/env/engine/off_chain/runtime_calls.rs
index ec49f2c1e0..8296f4af41 100644
--- a/core/src/env/engine/off_chain/runtime_calls.rs
+++ b/core/src/env/engine/off_chain/runtime_calls.rs
@@ -37,6 +37,11 @@ impl RuntimeCallHandler {
         Self { registered: None }
     }
 
+    /// Resets the runtime call handler to uninitialized state.
+    pub fn reset(&mut self) {
+        self.registered = None;
+    }
+
     /// Register a runtime call handler.
     pub fn register<T, F>(&mut self, mut f: F)
     where
diff --git a/core/src/env/engine/off_chain/runtime_storage.rs b/core/src/env/engine/off_chain/runtime_storage.rs
index efa9d9ddae..5b4b3c17d5 100644
--- a/core/src/env/engine/off_chain/runtime_storage.rs
+++ b/core/src/env/engine/off_chain/runtime_storage.rs
@@ -34,6 +34,11 @@ impl RuntimeStorage {
         }
     }
 
+    /// Resets the runtime storage to uninitialized state.
+    pub fn reset(&mut self) {
+        self.entries.clear();
+    }
+
     /// Stores the value under the given key.
     pub fn store<T>(&mut self, key: Vec<u8>, value: T)
     where


Test patch:
diff --git a/core/src/env/engine/off_chain/test_api.rs b/core/src/env/engine/off_chain/test_api.rs
index 9a32ab6eff..997c6b34de 100644
--- a/core/src/env/engine/off_chain/test_api.rs
+++ b/core/src/env/engine/off_chain/test_api.rs
@@ -288,14 +288,13 @@ where
 ///
 /// - Initializes the off-chain environment with default values that fit most
 /// uses cases.
-/// - The off-chain environment _must_ be initialized before use.
-pub fn initialize_as_default<T>() -> Result<()>
+pub fn initialize_or_reset_as_default<T>() -> Result<()>
 where
     T: EnvTypes,
     <T as EnvTypes>::AccountId: From<[u8; 32]>,
 {
     <EnvInstance as OnInstance>::on_instance(|instance| {
-        instance.initialize_as_default::<T>()
+        instance.initialize_or_reset_as_default::<T>()
     })
 }
 
@@ -307,7 +306,7 @@ where
     F: FnOnce(DefaultAccounts<T>) -> Result<()>,
     <T as EnvTypes>::AccountId: From<[u8; 32]>,
 {
-    initialize_as_default::<T>()?;
+    initialize_or_reset_as_default::<T>()?;
     let default_accounts = default_accounts::<T>()?;
     f(default_accounts)
 }
diff --git a/lang/macro/src/codegen/testable.rs b/lang/macro/src/codegen/testable.rs
index fca7691c55..29bb580bc7 100644
--- a/lang/macro/src/codegen/testable.rs
+++ b/lang/macro/src/codegen/testable.rs
@@ -56,7 +56,7 @@ impl GenerateCode for TestWrapper<'_> {
                     type Wrapped = TestableStorage;
 
                     fn instantiate() -> Self::Wrapped {
-                        ink_core::env::test::initialize_as_default::<ink_core::env::DefaultEnvTypes>()
+                        ink_core::env::test::initialize_or_reset_as_default::<ink_core::env::DefaultEnvTypes>()
                             .expect("encountered already initialized off-chain environment");
                         let mut contract: Self = unsafe {
                             let mut alloc =