From 9f0fa146448cc3b1e35fa9572b5e837a787d907b Mon Sep 17 00:00:00 2001 From: J Robert Ray Date: Thu, 4 Aug 2022 23:26:10 -0700 Subject: [PATCH 1/3] Reduce code duplication in cmd_test After the tester is configured, the code is the same. Use a trait to handle the different tester types. Reduce some of the duplicate code across the three test flavors, specifically targeting the code that generates IO errors. Signed-off-by: J Robert Ray --- crates/spk-cli/cmd-test/src/cmd_test.rs | 128 +++++++++++--------- crates/spk-cli/cmd-test/src/test/build.rs | 56 +++------ crates/spk-cli/cmd-test/src/test/install.rs | 55 +++------ crates/spk-cli/cmd-test/src/test/mod.rs | 54 +++++++++ crates/spk-cli/cmd-test/src/test/sources.rs | 55 +++------ 5 files changed, 183 insertions(+), 165 deletions(-) diff --git a/crates/spk-cli/cmd-test/src/cmd_test.rs b/crates/spk-cli/cmd-test/src/cmd_test.rs index d8df231b02..eacb4fd6ad 100644 --- a/crates/spk-cli/cmd-test/src/cmd_test.rs +++ b/crates/spk-cli/cmd-test/src/cmd_test.rs @@ -15,6 +15,8 @@ use spk_schema::foundation::spec_ops::RecipeOps; use spk_schema::ident::parse_ident; use spk_schema::{Recipe, Template, TestStage}; +use crate::test::{PackageBuildTester, PackageInstallTester, PackageSourceTester, Tester}; + #[cfg(test)] #[path = "./cmd_test_test.rs"] mod cmd_test_test; @@ -146,26 +148,6 @@ impl Run for Test { continue; } - let mut selected = false; - for selector in test.selectors.iter() { - let mut selected_opts = opts.clone(); - selected_opts.extend(selector.clone()); - if selected_opts.digest() == digest { - selected = true; - } - } - if !selected && !test.selectors.is_empty() { - tracing::info!( - "SKIP #{index}: variant not selected: {}", - opts.format_option_map() - ); - continue; - } - tracing::info!( - "Running test #{index} variant={}", - opts.format_option_map() - ); - let mut builder = self.formatter_settings.get_formatter_builder(self.verbose); let src_formatter = builder.with_header("Source Resolver ").build(); @@ -175,58 +157,90 @@ impl Run for Test { let install_formatter = builder.with_header("Install Env Resolver ").build(); - match stage { + let mut tester: Box = match stage { TestStage::Sources => { - super::test::PackageSourceTester::new( + let mut tester = PackageSourceTester::new( (*recipe).clone(), test.script.join("\n"), - ) - .with_options(opts.clone()) - .with_repositories(repos.iter().cloned()) - .with_requirements(test.requirements.clone()) - .with_source(source.clone()) - .watch_environment_resolve(&src_formatter) - .test() - .await? + ); + + tester + .with_options(opts.clone()) + .with_repositories(repos.iter().cloned()) + .with_requirements(test.requirements.clone()) + .with_source(source.clone()) + .watch_environment_resolve(&src_formatter); + + Box::new(tester) } TestStage::Build => { - super::test::PackageBuildTester::new( + let mut tester = PackageBuildTester::new( (*recipe).clone(), test.script.join("\n"), - ) - .with_options(opts.clone()) - .with_repositories(repos.iter().cloned()) - .with_requirements(test.requirements.clone()) - .with_source( - source.clone().map(BuildSource::LocalPath).unwrap_or_else( - || { - BuildSource::SourcePackage( - recipe.to_ident().into_build(Build::Source).into(), - ) - }, - ), - ) - .with_source_resolver(&build_src_formatter) - .with_build_resolver(&build_formatter) - .test() - .await? + ); + + tester + .with_options(opts.clone()) + .with_repositories(repos.iter().cloned()) + .with_requirements(test.requirements.clone()) + .with_source( + source.clone().map(BuildSource::LocalPath).unwrap_or_else( + || { + BuildSource::SourcePackage( + recipe + .to_ident() + .into_build(Build::Source) + .into(), + ) + }, + ), + ) + .with_source_resolver(&build_src_formatter) + .with_build_resolver(&build_formatter); + + Box::new(tester) } TestStage::Install => { - super::test::PackageInstallTester::new( + let mut tester = PackageInstallTester::new( (*recipe).clone(), test.script.join("\n"), - ) - .with_options(opts.clone()) - .with_repositories(repos.iter().cloned()) - .with_requirements(test.requirements.clone()) - .with_source(source.clone()) - .watch_environment_resolve(&install_formatter) - .test() - .await? + ); + + tester + .with_options(opts.clone()) + .with_repositories(repos.iter().cloned()) + .with_requirements(test.requirements.clone()) + .with_source(source.clone()) + .watch_environment_resolve(&install_formatter); + + Box::new(tester) + } + }; + + let mut selected = false; + for selector in test.selectors.iter() { + let mut selected_opts = opts.clone(); + selected_opts.extend(selector.clone()); + if selected_opts.digest() == digest { + selected = true; } } + if !selected && !test.selectors.is_empty() { + tracing::info!( + "SKIP #{index}: variant not selected: {}", + opts.format_option_map() + ); + continue; + } + + tracing::info!( + "Running test #{index} variant={}", + opts.format_option_map() + ); + + tester.test().await? } } } diff --git a/crates/spk-cli/cmd-test/src/test/build.rs b/crates/spk-cli/cmd-test/src/test/build.rs index e58d912d7b..724a00a614 100644 --- a/crates/spk-cli/cmd-test/src/test/build.rs +++ b/crates/spk-cli/cmd-test/src/test/build.rs @@ -2,13 +2,12 @@ // SPDX-License-Identifier: Apache-2.0 // https://github.com/imageworks/spk -use std::io::Write; -use std::path::PathBuf; +use std::convert::TryInto; +use std::path::{Path, PathBuf}; use std::sync::Arc; -use std::{convert::TryInto, ffi::OsString}; use spk_build::{source_package_path, BuildSource}; -use spk_cli_common::{Error, Result, TestError}; +use spk_cli_common::Result; use spk_exec::resolve_runtime_layers; use spk_schema::foundation::ident_build::Build; use spk_schema::foundation::ident_component::Component; @@ -21,6 +20,8 @@ use spk_solve::solution::Solution; use spk_solve::{BoxedResolverCallback, DefaultResolver, ResolverCallback, Solver}; use spk_storage::{self as storage}; +use super::Tester; + pub struct PackageBuildTester<'a> { prefix: PathBuf, recipe: SpecRecipe, @@ -143,16 +144,7 @@ impl<'a> PackageBuildTester<'a> { .recipe .generate_binary_build(&self.options, &solution)?; - let mut env = solution.to_environment(Some(std::env::vars())); - env.insert( - "PREFIX".to_string(), - self.prefix - .to_str() - .ok_or_else(|| { - Error::String("Test prefix must be a valid unicode string".to_string()) - })? - .to_string(), - ); + let env = solution.to_environment(Some(std::env::vars())); let source_dir = match &self.source { BuildSource::SourcePackage(source) => { @@ -161,28 +153,7 @@ impl<'a> PackageBuildTester<'a> { BuildSource::LocalPath(path) => path.clone(), }; - let tmpdir = tempfile::Builder::new().prefix("spk-test").tempdir()?; - let script_path = tmpdir.path().join("test.sh"); - let mut script_file = std::fs::File::create(&script_path)?; - script_file.write_all(self.script.as_bytes())?; - script_file.sync_data()?; - // TODO: this should be more easily configurable on the spfs side - std::env::set_var("SHELL", "bash"); - let cmd = spfs::build_shell_initialized_command( - &rt, - OsString::from("bash"), - &[OsString::from("-ex"), script_path.into_os_string()], - )?; - let mut cmd = cmd.into_std(); - let status = cmd.envs(env).current_dir(source_dir).status()?; - if !status.success() { - Err(TestError::new_error(format!( - "Test script returned non-zero exit status: {}", - status.code().unwrap_or(1) - ))) - } else { - Ok(()) - } + self.execute_test(&source_dir, env, &rt) } async fn resolve_source_package(&mut self, package: &Ident) -> Result { @@ -213,3 +184,16 @@ impl<'a> PackageBuildTester<'a> { Ok(solution?) } } + +#[async_trait::async_trait] +impl<'a> Tester for PackageBuildTester<'a> { + async fn test(&mut self) -> Result<()> { + self.test().await + } + fn prefix(&self) -> &Path { + &self.prefix + } + fn script(&self) -> &String { + &self.script + } +} diff --git a/crates/spk-cli/cmd-test/src/test/install.rs b/crates/spk-cli/cmd-test/src/test/install.rs index 75a1938a35..524854dfb6 100644 --- a/crates/spk-cli/cmd-test/src/test/install.rs +++ b/crates/spk-cli/cmd-test/src/test/install.rs @@ -2,12 +2,11 @@ // SPDX-License-Identifier: Apache-2.0 // https://github.com/imageworks/spk -use std::ffi::OsString; -use std::io::Write; +use std::path::Path; use std::path::PathBuf; use std::sync::Arc; -use spk_cli_common::{Error, Result, TestError}; +use spk_cli_common::Result; use spk_exec::resolve_runtime_layers; use spk_schema::foundation::ident_component::Component; use spk_schema::foundation::option_map::OptionMap; @@ -18,6 +17,8 @@ use spk_solve::graph::Graph; use spk_solve::{BoxedResolverCallback, DefaultResolver, ResolverCallback, Solver}; use spk_storage::{self as storage}; +use super::Tester; + pub struct PackageInstallTester<'a> { prefix: PathBuf, recipe: SpecRecipe, @@ -118,44 +119,26 @@ impl<'a> PackageInstallTester<'a> { rt.save_state_to_storage().await?; spfs::remount_runtime(&rt).await?; - let mut env = solution.to_environment(Some(std::env::vars())); - env.insert( - "PREFIX".to_string(), - self.prefix - .to_str() - .ok_or_else(|| { - Error::String("Test prefix must be a valid unicode string".to_string()) - })? - .to_string(), - ); + let env = solution.to_environment(Some(std::env::vars())); let source_dir = match &self.source { Some(source) => source.clone(), None => PathBuf::from("."), }; - let tmpdir = tempfile::Builder::new().prefix("spk-test").tempdir()?; - let script_path = tmpdir.path().join("test.sh"); - let mut script_file = std::fs::File::create(&script_path)?; - script_file.write_all(self.script.as_bytes())?; - script_file.sync_data()?; - - // TODO: this should be more easily configurable on the spfs side - std::env::set_var("SHELL", "bash"); - let cmd = spfs::build_shell_initialized_command( - &rt, - OsString::from("bash"), - &[OsString::from("-ex"), script_path.into_os_string()], - )?; - let mut cmd = cmd.into_std(); - let status = cmd.envs(env).current_dir(source_dir).status()?; - if !status.success() { - Err(TestError::new_error(format!( - "Test script returned non-zero exit status: {}", - status.code().unwrap_or(1) - ))) - } else { - Ok(()) - } + self.execute_test(&source_dir, env, &rt) + } +} + +#[async_trait::async_trait] +impl<'a> Tester for PackageInstallTester<'a> { + async fn test(&mut self) -> Result<()> { + self.test().await + } + fn prefix(&self) -> &Path { + &self.prefix + } + fn script(&self) -> &String { + &self.script } } diff --git a/crates/spk-cli/cmd-test/src/test/mod.rs b/crates/spk-cli/cmd-test/src/test/mod.rs index 026412f8d0..375ee2375a 100644 --- a/crates/spk-cli/cmd-test/src/test/mod.rs +++ b/crates/spk-cli/cmd-test/src/test/mod.rs @@ -6,6 +6,60 @@ mod build; mod install; mod sources; +use std::{collections::HashMap, ffi::OsString, io::Write, path::Path}; + pub use build::PackageBuildTester; pub use install::PackageInstallTester; pub use sources::PackageSourceTester; +use spfs::runtime::Runtime; +use spk_cli_common::{Error, Result, TestError}; + +/// Common code and logic for all test flavors. +#[async_trait::async_trait] +pub trait Tester: Send { + async fn test(&mut self) -> Result<()>; + + fn execute_test( + &self, + source_dir: &Path, + mut env: HashMap, + rt: &Runtime, + ) -> Result<()> { + env.insert( + "PREFIX".to_string(), + self.prefix() + .to_str() + .ok_or_else(|| { + Error::String("Test prefix must be a valid unicode string".to_string()) + })? + .to_string(), + ); + + let tmpdir = tempfile::Builder::new().prefix("spk-test").tempdir()?; + let script_path = tmpdir.path().join("test.sh"); + let mut script_file = std::fs::File::create(&script_path)?; + script_file.write_all(self.script().as_bytes())?; + script_file.sync_data()?; + // TODO: this should be more easily configurable on the spfs side + std::env::set_var("SHELL", "bash"); + let cmd = spfs::build_shell_initialized_command( + rt, + OsString::from("bash"), + &[OsString::from("-ex"), script_path.into_os_string()], + )?; + let mut cmd = cmd.into_std(); + let status = cmd.envs(env).current_dir(source_dir).status()?; + if !status.success() { + Err(TestError::new_error(format!( + "Test script returned non-zero exit status: {}", + status.code().unwrap_or(1) + ))) + } else { + Ok(()) + } + } + + fn prefix(&self) -> &Path; + + fn script(&self) -> &String; +} diff --git a/crates/spk-cli/cmd-test/src/test/sources.rs b/crates/spk-cli/cmd-test/src/test/sources.rs index 07db128c40..2b2dfaff0a 100644 --- a/crates/spk-cli/cmd-test/src/test/sources.rs +++ b/crates/spk-cli/cmd-test/src/test/sources.rs @@ -2,13 +2,11 @@ // SPDX-License-Identifier: Apache-2.0 // https://github.com/imageworks/spk -use std::ffi::OsString; -use std::io::Write; -use std::path::PathBuf; +use std::path::{Path, PathBuf}; use std::sync::Arc; use spk_build::source_package_path; -use spk_cli_common::{Error, Result, TestError}; +use spk_cli_common::Result; use spk_exec::resolve_runtime_layers; use spk_schema::foundation::ident_build::Build; use spk_schema::foundation::ident_component::Component; @@ -20,6 +18,8 @@ use spk_solve::graph::Graph; use spk_solve::{BoxedResolverCallback, DefaultResolver, ResolverCallback, Solver}; use spk_storage::{self as storage}; +use super::Tester; + pub struct PackageSourceTester<'a> { prefix: PathBuf, recipe: SpecRecipe, @@ -128,16 +128,7 @@ impl<'a> PackageSourceTester<'a> { rt.save_state_to_storage().await?; spfs::remount_runtime(&rt).await?; - let mut env = solution.to_environment(Some(std::env::vars())); - env.insert( - "PREFIX".to_string(), - self.prefix - .to_str() - .ok_or_else(|| { - Error::String("Test prefix must be a valid unicode string".to_string()) - })? - .to_string(), - ); + let env = solution.to_environment(Some(std::env::vars())); let source_dir = match &self.source { Some(source) => source.clone(), @@ -145,27 +136,19 @@ impl<'a> PackageSourceTester<'a> { .to_path(&self.prefix), }; - let tmpdir = tempfile::Builder::new().prefix("spk-test").tempdir()?; - let script_path = tmpdir.path().join("test.sh"); - let mut script_file = std::fs::File::create(&script_path)?; - script_file.write_all(self.script.as_bytes())?; - script_file.sync_data()?; - // TODO: this should be more easily configurable on the spfs side - std::env::set_var("SHELL", "bash"); - let cmd = spfs::build_shell_initialized_command( - &rt, - OsString::from("bash"), - &[OsString::from("-ex"), script_path.into_os_string()], - )?; - let mut cmd = cmd.into_std(); - let status = cmd.envs(env).current_dir(source_dir).status()?; - if !status.success() { - Err(TestError::new_error(format!( - "Test script returned non-zero exit status: {}", - status.code().unwrap_or(1) - ))) - } else { - Ok(()) - } + self.execute_test(&source_dir, env, &rt) + } +} + +#[async_trait::async_trait] +impl<'a> Tester for PackageSourceTester<'a> { + async fn test(&mut self) -> Result<()> { + self.test().await + } + fn prefix(&self) -> &Path { + &self.prefix + } + fn script(&self) -> &String { + &self.script } } From 0655e3416b9b0b25da92be2b79c7889da97d9f27 Mon Sep 17 00:00:00 2001 From: J Robert Ray Date: Tue, 23 Aug 2022 22:33:16 -0700 Subject: [PATCH 2/3] Rename `execute_test` Signed-off-by: J Robert Ray --- crates/spk-cli/cmd-test/src/test/build.rs | 2 +- crates/spk-cli/cmd-test/src/test/install.rs | 2 +- crates/spk-cli/cmd-test/src/test/mod.rs | 2 +- crates/spk-cli/cmd-test/src/test/sources.rs | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/crates/spk-cli/cmd-test/src/test/build.rs b/crates/spk-cli/cmd-test/src/test/build.rs index 724a00a614..7bddaeed73 100644 --- a/crates/spk-cli/cmd-test/src/test/build.rs +++ b/crates/spk-cli/cmd-test/src/test/build.rs @@ -153,7 +153,7 @@ impl<'a> PackageBuildTester<'a> { BuildSource::LocalPath(path) => path.clone(), }; - self.execute_test(&source_dir, env, &rt) + self.execute_test_script(&source_dir, env, &rt) } async fn resolve_source_package(&mut self, package: &Ident) -> Result { diff --git a/crates/spk-cli/cmd-test/src/test/install.rs b/crates/spk-cli/cmd-test/src/test/install.rs index 524854dfb6..5206e0a9f3 100644 --- a/crates/spk-cli/cmd-test/src/test/install.rs +++ b/crates/spk-cli/cmd-test/src/test/install.rs @@ -126,7 +126,7 @@ impl<'a> PackageInstallTester<'a> { None => PathBuf::from("."), }; - self.execute_test(&source_dir, env, &rt) + self.execute_test_script(&source_dir, env, &rt) } } diff --git a/crates/spk-cli/cmd-test/src/test/mod.rs b/crates/spk-cli/cmd-test/src/test/mod.rs index 375ee2375a..76ff6e278c 100644 --- a/crates/spk-cli/cmd-test/src/test/mod.rs +++ b/crates/spk-cli/cmd-test/src/test/mod.rs @@ -19,7 +19,7 @@ use spk_cli_common::{Error, Result, TestError}; pub trait Tester: Send { async fn test(&mut self) -> Result<()>; - fn execute_test( + fn execute_test_script( &self, source_dir: &Path, mut env: HashMap, diff --git a/crates/spk-cli/cmd-test/src/test/sources.rs b/crates/spk-cli/cmd-test/src/test/sources.rs index 2b2dfaff0a..9ea7d03400 100644 --- a/crates/spk-cli/cmd-test/src/test/sources.rs +++ b/crates/spk-cli/cmd-test/src/test/sources.rs @@ -136,7 +136,7 @@ impl<'a> PackageSourceTester<'a> { .to_path(&self.prefix), }; - self.execute_test(&source_dir, env, &rt) + self.execute_test_script(&source_dir, env, &rt) } } From 0c671b82a878cc697fca46e3b0385d60a5f11b14 Mon Sep 17 00:00:00 2001 From: J Robert Ray Date: Tue, 23 Aug 2022 22:41:07 -0700 Subject: [PATCH 3/3] Move code out of mod.rs Document the methods of `Tester`. Signed-off-by: J Robert Ray --- crates/spk-cli/cmd-test/src/test/mod.rs | 56 +------------------ crates/spk-cli/cmd-test/src/test/tester.rs | 63 ++++++++++++++++++++++ 2 files changed, 65 insertions(+), 54 deletions(-) create mode 100644 crates/spk-cli/cmd-test/src/test/tester.rs diff --git a/crates/spk-cli/cmd-test/src/test/mod.rs b/crates/spk-cli/cmd-test/src/test/mod.rs index 76ff6e278c..b8e2e7b1b1 100644 --- a/crates/spk-cli/cmd-test/src/test/mod.rs +++ b/crates/spk-cli/cmd-test/src/test/mod.rs @@ -5,61 +5,9 @@ mod build; mod install; mod sources; - -use std::{collections::HashMap, ffi::OsString, io::Write, path::Path}; +mod tester; pub use build::PackageBuildTester; pub use install::PackageInstallTester; pub use sources::PackageSourceTester; -use spfs::runtime::Runtime; -use spk_cli_common::{Error, Result, TestError}; - -/// Common code and logic for all test flavors. -#[async_trait::async_trait] -pub trait Tester: Send { - async fn test(&mut self) -> Result<()>; - - fn execute_test_script( - &self, - source_dir: &Path, - mut env: HashMap, - rt: &Runtime, - ) -> Result<()> { - env.insert( - "PREFIX".to_string(), - self.prefix() - .to_str() - .ok_or_else(|| { - Error::String("Test prefix must be a valid unicode string".to_string()) - })? - .to_string(), - ); - - let tmpdir = tempfile::Builder::new().prefix("spk-test").tempdir()?; - let script_path = tmpdir.path().join("test.sh"); - let mut script_file = std::fs::File::create(&script_path)?; - script_file.write_all(self.script().as_bytes())?; - script_file.sync_data()?; - // TODO: this should be more easily configurable on the spfs side - std::env::set_var("SHELL", "bash"); - let cmd = spfs::build_shell_initialized_command( - rt, - OsString::from("bash"), - &[OsString::from("-ex"), script_path.into_os_string()], - )?; - let mut cmd = cmd.into_std(); - let status = cmd.envs(env).current_dir(source_dir).status()?; - if !status.success() { - Err(TestError::new_error(format!( - "Test script returned non-zero exit status: {}", - status.code().unwrap_or(1) - ))) - } else { - Ok(()) - } - } - - fn prefix(&self) -> &Path; - - fn script(&self) -> &String; -} +pub use tester::Tester; diff --git a/crates/spk-cli/cmd-test/src/test/tester.rs b/crates/spk-cli/cmd-test/src/test/tester.rs new file mode 100644 index 0000000000..585544823c --- /dev/null +++ b/crates/spk-cli/cmd-test/src/test/tester.rs @@ -0,0 +1,63 @@ +// Copyright (c) Sony Pictures Imageworks, et al. +// SPDX-License-Identifier: Apache-2.0 +// https://github.com/imageworks/spk + +use std::{collections::HashMap, ffi::OsString, io::Write, path::Path}; + +use spfs::runtime::Runtime; +use spk_cli_common::{Error, Result, TestError}; + +/// Common code and logic for all test flavors. +#[async_trait::async_trait] +pub trait Tester: Send { + /// Create the runtime environment for the defined test and then execute + /// the test. + async fn test(&mut self) -> Result<()>; + + /// Generate and invoke the test script defined in the recipe. + fn execute_test_script( + &self, + source_dir: &Path, + mut env: HashMap, + rt: &Runtime, + ) -> Result<()> { + env.insert( + "PREFIX".to_string(), + self.prefix() + .to_str() + .ok_or_else(|| { + Error::String("Test prefix must be a valid unicode string".to_string()) + })? + .to_string(), + ); + + let tmpdir = tempfile::Builder::new().prefix("spk-test").tempdir()?; + let script_path = tmpdir.path().join("test.sh"); + let mut script_file = std::fs::File::create(&script_path)?; + script_file.write_all(self.script().as_bytes())?; + script_file.sync_data()?; + // TODO: this should be more easily configurable on the spfs side + std::env::set_var("SHELL", "bash"); + let cmd = spfs::build_shell_initialized_command( + rt, + OsString::from("bash"), + &[OsString::from("-ex"), script_path.into_os_string()], + )?; + let mut cmd = cmd.into_std(); + let status = cmd.envs(env).current_dir(source_dir).status()?; + if !status.success() { + Err(TestError::new_error(format!( + "Test script returned non-zero exit status: {}", + status.code().unwrap_or(1) + ))) + } else { + Ok(()) + } + } + + /// Return the root path of the overlayfs + fn prefix(&self) -> &Path; + + /// Return the text of the test script. + fn script(&self) -> &String; +}