Fix flaky CI test (use-ink#263)

* Add fix

* Revert "Add fix"

This reverts commit 5c81fee.

* Generate unique contract names for tests

* Fix test

* Implement comments

* Implement comments

src/cmd/build.rs

@@ -590,8 +590,8 @@ pub(crate) fn execute(
 mod tests_ci_only {
     use super::{assert_compatible_ink_dependencies, check_wasm_opt_version_compatibility};
     use crate::{
-        cmd::{self, BuildCommand},
-        util::tests::with_tmp_dir,
+        cmd::BuildCommand,
+        util::tests::{with_new_contract_project, with_tmp_dir},
         workspace::Manifest,
         BuildArtifacts, ManifestPath, OptimizationPasses, UnstableFlags, UnstableOptions,
         Verbosity, VerbosityFlags,
@@ -638,10 +638,7 @@ mod tests_ci_only {
 
     #[test]
     fn build_code_only() {
-        with_tmp_dir(|path| {
-            cmd::new::execute("new_project", Some(path)).expect("new project creation failed");
-            let manifest_path =
-                ManifestPath::new(&path.join("new_project").join("Cargo.toml")).unwrap();
+        with_new_contract_project(|manifest_path| {
             let res = super::execute(
                 &manifest_path,
                 Verbosity::Default,
@@ -676,11 +673,9 @@ mod tests_ci_only {
 
     #[test]
     fn check_must_not_output_contract_artifacts_in_project_dir() {
-        with_tmp_dir(|path| {
+        with_new_contract_project(|manifest_path| {
             // given
-            cmd::new::execute("new_project", Some(path)).expect("new project creation failed");
-            let project_dir = path.join("new_project");
-            let manifest_path = ManifestPath::new(&project_dir.join("Cargo.toml")).unwrap();
+            let project_dir = manifest_path.directory().expect("directory must exist");
 
             // when
             super::execute(
@@ -707,13 +702,14 @@ mod tests_ci_only {
 
     #[test]
     fn optimization_passes_from_cli_must_take_precedence_over_profile() {
-        with_tmp_dir(|path| {
+        with_new_contract_project(|manifest_path| {
             // given
-            cmd::new::execute("new_project", Some(path)).expect("new project creation failed");
-            let cargo_toml_path = path.join("new_project").join("Cargo.toml");
-            write_optimization_passes_into_manifest(&cargo_toml_path, OptimizationPasses::Three);
+            write_optimization_passes_into_manifest(
+                &manifest_path.clone().into(),
+                OptimizationPasses::Three,
+            );
             let cmd = BuildCommand {
-                manifest_path: Some(cargo_toml_path),
+                manifest_path: Some(manifest_path.into()),
                 build_artifact: BuildArtifacts::All,
                 verbosity: VerbosityFlags::default(),
                 unstable_options: UnstableOptions::default(),
@@ -746,13 +742,14 @@ mod tests_ci_only {
 
     #[test]
     fn optimization_passes_from_profile_must_be_used() {
-        with_tmp_dir(|path| {
+        with_new_contract_project(|manifest_path| {
             // given
-            cmd::new::execute("new_project", Some(path)).expect("new project creation failed");
-            let cargo_toml_path = path.join("new_project").join("Cargo.toml");
-            write_optimization_passes_into_manifest(&cargo_toml_path, OptimizationPasses::Three);
+            write_optimization_passes_into_manifest(
+                &manifest_path.clone().into(),
+                OptimizationPasses::Three,
+            );
             let cmd = BuildCommand {
-                manifest_path: Some(cargo_toml_path),
+                manifest_path: Some(manifest_path.into()),
                 build_artifact: BuildArtifacts::All,
                 verbosity: VerbosityFlags::default(),
                 unstable_options: UnstableOptions::default(),
@@ -785,12 +782,9 @@ mod tests_ci_only {
 
     #[test]
     fn project_template_dependencies_must_be_ink_compatible() {
-        with_tmp_dir(|path| {
+        with_new_contract_project(|manifest_path| {
             // given
-            cmd::new::execute("new_project", Some(path)).expect("new project creation failed");
-            let cargo_toml_path = path.join("new_project").join("Cargo.toml");
-            let manifest_path =
-                ManifestPath::new(&cargo_toml_path).expect("manifest path creation failed");
+            // the manifest path
 
             // when
             let res = assert_compatible_ink_dependencies(&manifest_path, Verbosity::Default);
@@ -803,12 +797,9 @@ mod tests_ci_only {
 
     #[test]
     fn detect_mismatching_parity_scale_codec_dependencies() {
-        with_tmp_dir(|path| {
+        with_new_contract_project(|manifest_path| {
             // given
-            cmd::new::execute("new_project", Some(path)).expect("new project creation failed");
-            let cargo_toml_path = path.join("new_project").join("Cargo.toml");
-            let manifest_path =
-                ManifestPath::new(&cargo_toml_path).expect("manifest path creation failed");
+            // the manifest path
 
             // at the time of writing this test ink! already uses `parity-scale-codec`
             // in a version > 2, hence 1 is an incompatible version.

src/cmd/metadata.rs

@@ -225,7 +225,7 @@ fn blake2_hash(code: &[u8]) -> CodeHash {
 mod tests {
     use crate::cmd::metadata::blake2_hash;
     use crate::{
-        cmd, crate_metadata::CrateMetadata, util::tests::with_tmp_dir, BuildArtifacts,
+        cmd, crate_metadata::CrateMetadata, util::tests::with_new_contract_project, BuildArtifacts,
         ManifestPath, OptimizationPasses, UnstableFlags, Verbosity,
     };
     use contract_metadata::*;
@@ -299,11 +299,7 @@ mod tests {
     #[test]
     fn generate_metadata() {
         env_logger::try_init().ok();
-        with_tmp_dir(|path| {
-            cmd::new::execute("new_project", Some(path)).expect("new project creation failed");
-            let working_dir = path.join("new_project");
-            let manifest_path = ManifestPath::new(working_dir.join("Cargo.toml"))?;
-
+        with_new_contract_project(|manifest_path| {
             // add optional metadata fields
             let mut test_manifest = TestContractManifest::new(manifest_path)?;
             test_manifest.add_package_value("description", "contract description".into())?;

src/cmd/new.rs

@@ -108,12 +108,12 @@ where
 #[cfg(test)]
 mod tests {
     use super::*;
-    use crate::util::tests::with_tmp_dir;
+    use crate::util::tests::{with_new_contract_project, with_tmp_dir};
 
     #[test]
     fn rejects_hyphenated_name() {
-        with_tmp_dir(|path| {
-            let result = execute("rejects-hyphenated-name", Some(path));
+        with_new_contract_project(|manifest_path| {
+            let result = execute("rejects-hyphenated-name", Some(manifest_path));
             assert!(result.is_err(), "Should fail");
             assert_eq!(
                 result.err().unwrap().to_string(),
@@ -125,8 +125,8 @@ mod tests {
 
     #[test]
     fn rejects_name_with_period() {
-        with_tmp_dir(|path| {
-            let result = execute("../xxx", Some(path));
+        with_new_contract_project(|manifest_path| {
+            let result = execute("../xxx", Some(manifest_path));
             assert!(result.is_err(), "Should fail");
             assert_eq!(
                 result.err().unwrap().to_string(),
@@ -138,8 +138,8 @@ mod tests {
 
     #[test]
     fn rejects_name_beginning_with_number() {
-        with_tmp_dir(|path| {
-            let result = execute("1xxx", Some(path));
+        with_new_contract_project(|manifest_path| {
+            let result = execute("1xxx", Some(manifest_path));
             assert!(result.is_err(), "Should fail");
             assert_eq!(
                 result.err().unwrap().to_string(),

src/util.rs

@@ -104,8 +104,12 @@ macro_rules! maybe_println {
 
 #[cfg(test)]
 pub mod tests {
+    use crate::ManifestPath;
     use std::path::Path;
+    use std::sync::atomic::{AtomicU32, Ordering};
 
+    /// Creates a temporary directory and passes the `tmp_dir` path to `f`.
+    /// Panics if `f` returns an `Err`.
     pub fn with_tmp_dir<F>(f: F)
     where
         F: FnOnce(&Path) -> anyhow::Result<()>,
@@ -118,4 +122,42 @@ pub mod tests {
         // catch test panics in order to clean up temp dir which will be very large
         f(tmp_dir.path()).expect("Error executing test with tmp dir")
     }
+
+    /// Global counter to generate unique contract names in `with_new_contract_project`.
+    ///
+    /// We typically use `with_tmp_dir` to generate temporary folders to build contracts
+    /// in. But for caching purposes our CI uses `CARGO_TARGET_DIR` to overwrite the
+    /// target directory of any contract build -- it is set to a fixed cache directory
+    /// instead.
+    /// This poses a problem since we still want to ensure that each test builds to its
+    /// own, unique target directory -- without interfering with the target directory of
+    /// other tests. In the past this has been a problem when a test tried to create a
+    /// contract with the same contract name as another test -- both were then build
+    /// into the same target directory, sometimes causing test failures for strange reasons.
+    ///
+    /// The fix we decided on is to append a unique number to each contract name which
+    /// is created. This `COUNTER` provides a global counter which is accessed by each test
+    /// (in each thread) to get the current `COUNTER` number and increase it afterwards.
+    ///
+    /// We decided to go for this counter instead of hashing (with e.g. the temp dir) to
+    /// prevent an infinite number of contract artifacts being created in the cache directory.
+    static COUNTER: AtomicU32 = AtomicU32::new(0);
+
+    /// Creates a new contract into a temporary directory. The contract's
+    /// `ManifestPath` is passed into `f`.
+    pub fn with_new_contract_project<F>(f: F)
+    where
+        F: FnOnce(ManifestPath) -> anyhow::Result<()>,
+    {
+        with_tmp_dir(|tmp_dir| {
+            let unique_name = format!("new_project_{}", COUNTER.fetch_add(1, Ordering::SeqCst));
+
+            crate::cmd::new::execute(&unique_name, Some(tmp_dir))
+                .expect("new project creation failed");
+            let working_dir = tmp_dir.join(unique_name);
+            let manifest_path = ManifestPath::new(working_dir.join("Cargo.toml"))?;
+
+            f(manifest_path)
+        })
+    }
 }