From a9985c00dc31338e38fe398177630f529cd4019d Mon Sep 17 00:00:00 2001 From: Ed Morley <501702+edmorley@users.noreply.github.com> Date: Tue, 18 Oct 2022 10:55:35 +0100 Subject: [PATCH] libcnb-test: Check the exit code of `pack sbom download` Previously if the `pack sbom download` command exited non-zero, `TestContext::download_sbom_files` would ignore the error and return an `SbomFiles` referencing an empty directory. Now, such errors will result in a test panic showing the stdout/stderr from pack - like already occurs for errors during `pack build`. I tried writing an integration test for this, however wasn't able to find an easy way to make `pack sbom` fail (it appears to succeed even for images that don't have any SBOMs). However the implementation is already tested via the `pack build` consumer, so this could be worse. GUS-W-11924792. --- CHANGELOG.md | 4 ++++ libcnb-test/src/pack.rs | 42 ++++++++++++++++++++++++--------- libcnb-test/src/test_context.rs | 4 ++-- libcnb-test/src/test_runner.rs | 21 +++-------------- 4 files changed, 40 insertions(+), 31 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c75879d2..1d7c102b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,10 @@ separate changelogs for each crate were used. If you need to refer to these old ## [Unreleased] +### Fixed + +- libcnb-test: `TestContext::download_sbom_files` now checks the exit code of the `pack sbom download` command it runs. ([#520](https://github.com/heroku/libcnb.rs/pull/520)) + ### Changed - libcnb: Drop the use of the `stacker` crate when recursively removing layer directories. ([#517](https://github.com/heroku/libcnb.rs/pull/517)) diff --git a/libcnb-test/src/pack.rs b/libcnb-test/src/pack.rs index a3b9fda9..d70886f4 100644 --- a/libcnb-test/src/pack.rs +++ b/libcnb-test/src/pack.rs @@ -1,6 +1,7 @@ +use crate::PackResult; use std::collections::BTreeMap; use std::path::PathBuf; -use std::process::{Command, Output, Stdio}; +use std::process::{Command, Output}; use tempfile::TempDir; /// Represents a `pack build` command. @@ -175,22 +176,41 @@ impl From for Command { } } -/// Runs the given pack command, panicking with user-friendly error messages when errors occur and -/// pipes through stdout and stderr. -pub(crate) fn run_pack_command>(command: C) -> Output { - command.into() - .stdout(Stdio::piped()) - .stderr(Stdio::piped()) - .spawn() +/// Runs the given pack command, panicking with user-friendly error messages when errors occur. +pub(crate) fn run_pack_command>( + command: C, + expected_result: &PackResult, +) -> Output { + let output = command.into() + .output() .unwrap_or_else(|io_error| { if io_error.kind() == std::io::ErrorKind::NotFound { panic!("External `pack` command not found. Install Pack CLI and ensure it is on PATH: https://buildpacks.io/docs/install-pack"); } else { panic!("Could not spawn external `pack` process: {io_error}"); }; - }) - .wait_with_output() - .expect("Error while waiting on external `pack` process") + }); + + if (expected_result == &PackResult::Success && !output.status.success()) + || (expected_result == &PackResult::Failure && output.status.success()) + { + panic!( + "pack command unexpectedly {} with exit-code {}!\n\npack stdout:\n{}\n\npack stderr:\n{}", + if output.status.success() { + "succeeded" + } else { + "failed" + }, + output + .status + .code() + .map_or(String::from(""), |exit_code| exit_code.to_string()), + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr) + ); + } + + output } #[cfg(test)] diff --git a/libcnb-test/src/test_context.rs b/libcnb-test/src/test_context.rs index 41a49d98..0afa900f 100644 --- a/libcnb-test/src/test_context.rs +++ b/libcnb-test/src/test_context.rs @@ -1,7 +1,7 @@ use crate::pack::{run_pack_command, PackSbomDownloadCommand}; use crate::{ container_port_mapping, util, BuildConfig, ContainerConfig, ContainerContext, LogOutput, - TestRunner, + PackResult, TestRunner, }; use bollard::container::{Config, CreateContainerOptions, StartContainerOptions}; use bollard::image::RemoveImageOptions; @@ -202,7 +202,7 @@ impl<'a> TestContext<'a> { let mut command = PackSbomDownloadCommand::new(&self.image_name); command.output_dir(temp_dir.path()); - run_pack_command(command); + run_pack_command(command, &PackResult::Success); f(SbomFiles { sbom_files_directory: temp_dir.path().into(), diff --git a/libcnb-test/src/test_runner.rs b/libcnb-test/src/test_runner.rs index 096d1b77..76128623 100644 --- a/libcnb-test/src/test_runner.rs +++ b/libcnb-test/src/test_runner.rs @@ -1,5 +1,5 @@ use crate::pack::{run_pack_command, PackBuildCommand}; -use crate::{app, build, util, BuildConfig, BuildpackReference, PackResult, TestContext}; +use crate::{app, build, util, BuildConfig, BuildpackReference, TestContext}; use bollard::Docker; use std::borrow::Borrow; use std::env; @@ -156,7 +156,7 @@ impl TestRunner { }; } - let output = run_pack_command(pack_command); + let output = run_pack_command(pack_command, &config.expected_pack_result); let test_context = TestContext { pack_stdout: String::from_utf8_lossy(&output.stdout).into_owned(), @@ -166,21 +166,6 @@ impl TestRunner { runner: self, }; - if (config.expected_pack_result == PackResult::Failure && output.status.success()) - || (config.expected_pack_result == PackResult::Success && !output.status.success()) - { - panic!( - "pack command unexpectedly {} with exit-code {}!\n\npack stdout:\n{}\n\npack stderr:\n{}", - if output.status.success() { "succeeded" } else { "failed" }, - output - .status - .code() - .map_or(String::from(""), |exit_code| exit_code.to_string()), - test_context.pack_stdout, - test_context.pack_stderr - ); - } else { - f(test_context); - } + f(test_context); } }