Remove TestContext::app_dir, only copy app_dir when preprocessor is configured

libcnb-test/CHANGELOG.md

@@ -15,6 +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 `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
 

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<Path>) -> Result<TempDir, PrepareAppError> {
+pub(crate) fn copy_app(app_dir: impl AsRef<Path>) -> Result<AppDir, PrepareAppError> {
     tempdir()
         .map_err(PrepareAppError::CreateTempDirError)
         .and_then(|temp_app_dir| {
@@ -16,7 +17,7 @@ pub(crate) fn copy_app(app_dir: impl AsRef<Path>) -> Result<TempDir, PrepareAppE
                 },
             )
             .map_err(PrepareAppError::CopyAppError)
-            .map(|_| temp_app_dir)
+            .map(|_| temp_app_dir.into())
         })
 }
 
@@ -26,6 +27,38 @@ pub enum PrepareAppError {
     CopyAppError(fs_extra::error::Error),
 }
 
+pub(crate) enum AppDir {
+    Temporary(TempDir),
+    Unmanaged(PathBuf),
+}
+
+impl AppDir {
+    pub fn as_path(&self) -> &Path {
+        match self {
+            AppDir::Temporary(temp_dir) => temp_dir.path(),
+            AppDir::Unmanaged(path) => path,
+        }
+    }
+}
+
+impl AsRef<OsStr> for AppDir {
+    fn as_ref(&self) -> &OsStr {
+        self.as_path().as_os_str()
+    }
+}
+
+impl From<PathBuf> for AppDir {
+    fn from(value: PathBuf) -> Self {
+        AppDir::Unmanaged(value)
+    }
+}
+
+impl From<TempDir> 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);
         }

libcnb-test/src/test_context.rs

@@ -1,19 +1,13 @@
 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> {
     /// Standard output of `pack`, interpreted as an UTF-8 string.
     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,
 

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,
@@ -192,7 +200,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,