From 9e7cee8a003c37c513469a6fe87890a612acee96 Mon Sep 17 00:00:00 2001 From: Manuel Fuchs Date: Fri, 24 Jun 2022 12:31:26 +0200 Subject: [PATCH 1/3] Remove TestContext::app_dir --- libcnb-test/CHANGELOG.md | 1 + libcnb-test/src/test_context.rs | 6 ------ libcnb-test/src/test_runner.rs | 1 - 3 files changed, 1 insertion(+), 7 deletions(-) diff --git a/libcnb-test/CHANGELOG.md b/libcnb-test/CHANGELOG.md index 89a271bf..02f6e491 100644 --- a/libcnb-test/CHANGELOG.md +++ b/libcnb-test/CHANGELOG.md @@ -15,6 +15,7 @@ - Add `TestContext::run_test`, allowing you to run subsequent integration tests with the image from a previous test. These functions allow testing of subsequent builds, including caching logic and buildpack behaviour when build environment variables change, stacks are upgraded and more. ([#422](https://github.com/heroku/libcnb.rs/pull/422)). - Add `TestConfig::expected_pack_result`. When set to `PackResult::Failure`, it allows testing of build failure scenarios. ([#429](https://github.com/heroku/libcnb.rs/pull/429)). - Add `TestConfig::app_dir` which is handy in cases where `TestConfig` values are shared and only the `app_dir` needs to be different. ([#430](https://github.com/heroku/libcnb.rs/pull/430)). +- Remove `TestContext::app_dir` to encourage the use of `TestContext::app_dir_preprocessor`. ([#431](https://github.com/heroku/libcnb.rs/pull/431)). ## [0.3.1] 2022-04-12 diff --git a/libcnb-test/src/test_context.rs b/libcnb-test/src/test_context.rs index c5accf7c..1abb2209 100644 --- a/libcnb-test/src/test_context.rs +++ b/libcnb-test/src/test_context.rs @@ -1,7 +1,6 @@ use crate::{PrepareContainerContext, TestConfig, TestRunner}; use bollard::image::RemoveImageOptions; use std::borrow::Borrow; -use std::path::PathBuf; /// Context for a currently executing test. pub struct TestContext<'a> { @@ -9,11 +8,6 @@ pub struct TestContext<'a> { pub pack_stdout: String, /// Standard error of `pack`, interpreted as an UTF-8 string. pub pack_stderr: String, - /// The directory of the app this integration test uses. - /// - /// This is a copy of the `app_dir` directory passed to [`TestConfig::new`] and unique to - /// this integration test run. It is safe to modify the directory contents inside the test. - pub app_dir: PathBuf, /// The configuration used for this integration test. pub config: TestConfig, diff --git a/libcnb-test/src/test_runner.rs b/libcnb-test/src/test_runner.rs index 99729a75..1335bbb0 100644 --- a/libcnb-test/src/test_runner.rs +++ b/libcnb-test/src/test_runner.rs @@ -192,7 +192,6 @@ impl TestRunner { let test_context = TestContext { pack_stdout: String::from_utf8_lossy(&output.stdout).into_owned(), pack_stderr: String::from_utf8_lossy(&output.stderr).into_owned(), - app_dir: PathBuf::from(temp_app_dir.path()), image_name, config: config.clone(), runner: self, From debff598fdd1cb4bb2a951623bd38009f2337dcd Mon Sep 17 00:00:00 2001 From: Manuel Fuchs Date: Fri, 24 Jun 2022 12:52:23 +0200 Subject: [PATCH 2/3] Only copy app_dir when a preprocessor is configured --- libcnb-test/CHANGELOG.md | 3 ++- libcnb-test/src/app.rs | 41 ++++++++++++++++++++++++++++++---- libcnb-test/src/test_runner.rs | 36 +++++++++++++++++------------ 3 files changed, 61 insertions(+), 19 deletions(-) diff --git a/libcnb-test/CHANGELOG.md b/libcnb-test/CHANGELOG.md index 02f6e491..e694b72f 100644 --- a/libcnb-test/CHANGELOG.md +++ b/libcnb-test/CHANGELOG.md @@ -15,7 +15,8 @@ - Add `TestContext::run_test`, allowing you to run subsequent integration tests with the image from a previous test. These functions allow testing of subsequent builds, including caching logic and buildpack behaviour when build environment variables change, stacks are upgraded and more. ([#422](https://github.com/heroku/libcnb.rs/pull/422)). - Add `TestConfig::expected_pack_result`. When set to `PackResult::Failure`, it allows testing of build failure scenarios. ([#429](https://github.com/heroku/libcnb.rs/pull/429)). - Add `TestConfig::app_dir` which is handy in cases where `TestConfig` values are shared and only the `app_dir` needs to be different. ([#430](https://github.com/heroku/libcnb.rs/pull/430)). -- Remove `TestContext::app_dir` to encourage the use of `TestContext::app_dir_preprocessor`. ([#431](https://github.com/heroku/libcnb.rs/pull/431)). +- Remove `TestContext::app_dir` to encourage the use of `TestConfig::app_dir_preprocessor`. ([#431](https://github.com/heroku/libcnb.rs/pull/431)). +- Improve performance when no `TestConfig::app_dir_preprocessor` is configured by skipping application directory copying. ([#431](https://github.com/heroku/libcnb.rs/pull/431)). ## [0.3.1] 2022-04-12 diff --git a/libcnb-test/src/app.rs b/libcnb-test/src/app.rs index 50b08fb1..42bbd36f 100644 --- a/libcnb-test/src/app.rs +++ b/libcnb-test/src/app.rs @@ -1,9 +1,10 @@ use fs_extra::dir::CopyOptions; -use std::path::Path; +use std::ffi::OsStr; +use std::path::{Path, PathBuf}; use tempfile::{tempdir, TempDir}; /// Copies an application directory to a temporary location. -pub(crate) fn copy_app(app_dir: impl AsRef) -> Result { +pub(crate) fn copy_app(app_dir: impl AsRef) -> Result { tempdir() .map_err(PrepareAppError::CreateTempDirError) .and_then(|temp_app_dir| { @@ -16,7 +17,7 @@ pub(crate) fn copy_app(app_dir: impl AsRef) -> Result &Path { + match self { + AppDir::Temporary(temp_dir) => temp_dir.path(), + AppDir::Unmanaged(path) => path, + } + } +} + +impl AsRef for AppDir { + fn as_ref(&self) -> &OsStr { + self.as_path().as_os_str() + } +} + +impl From for AppDir { + fn from(value: PathBuf) -> Self { + AppDir::Unmanaged(value) + } +} + +impl From for AppDir { + fn from(value: TempDir) -> Self { + AppDir::Temporary(value) + } +} + #[cfg(test)] mod tests { use std::collections::HashMap; @@ -59,7 +92,7 @@ mod tests { let temp_app_dir = super::copy_app(source_app_dir.path()).unwrap(); for (path, contents) in files { - let absolute_path = temp_app_dir.path().join(path); + let absolute_path = temp_app_dir.as_path().join(path); assert_eq!(std::fs::read_to_string(absolute_path).unwrap(), contents); } diff --git a/libcnb-test/src/test_runner.rs b/libcnb-test/src/test_runner.rs index 1335bbb0..116dfc7d 100644 --- a/libcnb-test/src/test_runner.rs +++ b/libcnb-test/src/test_runner.rs @@ -134,21 +134,29 @@ impl TestRunner { ) { let config = config.borrow(); - let app_dir = if config.app_dir.is_relative() { - env::var("CARGO_MANIFEST_DIR") - .map(PathBuf::from) - .expect("Could not determine Cargo manifest directory") - .join(&config.app_dir) - } else { - config.app_dir.clone() - }; + let app_dir = { + let normalized_app_dir_path = if config.app_dir.is_relative() { + env::var("CARGO_MANIFEST_DIR") + .map(PathBuf::from) + .expect("Could not determine Cargo manifest directory") + .join(&config.app_dir) + } else { + config.app_dir.clone() + }; - let temp_app_dir = - app::copy_app(&app_dir).expect("Could not copy app to temporary location"); + // Copy the app to a temporary directory if an app_dir_preprocessor is specified and run the + // preprocessor. Skip app copying if no changes to the app will be made. + if let Some(app_dir_preprocessor) = &config.app_dir_preprocessor { + let app_dir = app::copy_app(&normalized_app_dir_path) + .expect("Could not copy app to temporary location"); - if let Some(app_dir_preprocessor) = &config.app_dir_preprocessor { - (app_dir_preprocessor)(PathBuf::from(temp_app_dir.path())); - } + (app_dir_preprocessor)(app_dir.as_path().to_owned()); + + app_dir + } else { + normalized_app_dir_path.into() + } + }; let temp_crate_buildpack_dir = config @@ -161,7 +169,7 @@ impl TestRunner { let mut pack_command = PackBuildCommand::new( &config.builder_name, - temp_app_dir.path(), + &app_dir, &image_name, // Prevent redundant image-pulling, which slows tests and risks hitting registry rate limits. PullPolicy::IfNotPresent, From 6d53175544ab91e177c9b64cb495572c4060cf18 Mon Sep 17 00:00:00 2001 From: Manuel Fuchs Date: Fri, 24 Jun 2022 13:45:55 +0200 Subject: [PATCH 3/3] Rename app_dir to temporary_app_dir --- libcnb-test/src/test_runner.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/libcnb-test/src/test_runner.rs b/libcnb-test/src/test_runner.rs index 116dfc7d..a4656d9f 100644 --- a/libcnb-test/src/test_runner.rs +++ b/libcnb-test/src/test_runner.rs @@ -147,12 +147,12 @@ impl TestRunner { // Copy the app to a temporary directory if an app_dir_preprocessor is specified and run the // preprocessor. Skip app copying if no changes to the app will be made. if let Some(app_dir_preprocessor) = &config.app_dir_preprocessor { - let app_dir = app::copy_app(&normalized_app_dir_path) + let temporary_app_dir = app::copy_app(&normalized_app_dir_path) .expect("Could not copy app to temporary location"); - (app_dir_preprocessor)(app_dir.as_path().to_owned()); + (app_dir_preprocessor)(temporary_app_dir.as_path().to_owned()); - app_dir + temporary_app_dir } else { normalized_app_dir_path.into() }