diff --git a/src/cmd/build.rs b/src/cmd/build.rs index a0a19a76172331e10cfb89968ac0393111970f77..c29f590a3e741b6ebaaf60dfd38745bdb86943a6 100644 --- a/src/cmd/build.rs +++ b/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. diff --git a/src/cmd/metadata.rs b/src/cmd/metadata.rs index 4030e4460dba1f3e1e34745582787d3dabb11791..94cb4d104dcd70841267ff6391a0a055b65e2417 100644 --- a/src/cmd/metadata.rs +++ b/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())?; diff --git a/src/cmd/new.rs b/src/cmd/new.rs index 3c88c5b25ffee2aa2b441d6797e0d25534a93ba8..23eccd4096e71c70630d7564548e8a883c6eec37 100644 --- a/src/cmd/new.rs +++ b/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(), diff --git a/src/util.rs b/src/util.rs index c701f0abff4069fc4c8d35154bc39f9aad313246..71fcaa45848e154bd6407540f221dacdbb6257c3 100644 --- a/src/util.rs +++ b/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) 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) + 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) + }) + } }