From 5ab4b036a8dd18dc2d25a3d3ee49ed66ef3ec9d3 Mon Sep 17 00:00:00 2001 From: Ed Morley <501702+edmorley@users.noreply.github.com> Date: Tue, 7 Nov 2023 12:52:07 +0000 Subject: [PATCH] Fix error messages for buildpack packaging failures (#720) After #666, incorrect error messages were shown for failures that occurred during buildpack compilation packaging, since the handling in `TestRunner::build_internal` used the wrong message text, plus didn't include the error struct in the `panic!` string. These have been fixed, plus display representations added to all of the new error variants. In addition, the usages of `assert!` and `.unwrap()` in functions that return `Result` have been replaced with new error enum variants. The change in output can be seen in the new tests added by #717. There is one last scenario that doesn't yet have a test - the testing of the cross-compilation output itself. However, I'll add that separately, since it's going to be more complex to test / there are a few different options that we'll need to choose between. Fixes #704. Fixes #709. Fixes #710. GUS-W-14395170. --- CHANGELOG.md | 4 ++ libcnb-test/Cargo.toml | 1 + libcnb-test/src/build.rs | 55 +++++++++++++++------------ libcnb-test/src/test_runner.rs | 12 ++++-- libcnb-test/tests/integration_test.rs | 26 ++++++------- 5 files changed, 56 insertions(+), 42 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 82610c45..127d7510 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Fixed + +- `libcnb-test`: + - Fixed incorrect error messages being shown for buildpack compilation/packaging failures. ([#720](https://github.com/heroku/libcnb.rs/pull/720)) ## [0.15.0] - 2023-09-25 diff --git a/libcnb-test/Cargo.toml b/libcnb-test/Cargo.toml index 3e568da8..011b60c2 100644 --- a/libcnb-test/Cargo.toml +++ b/libcnb-test/Cargo.toml @@ -18,6 +18,7 @@ libcnb-common.workspace = true libcnb-data.workspace = true libcnb-package.workspace = true tempfile = "3.8.0" +thiserror = "1.0.48" [dev-dependencies] indoc = "2.0.4" diff --git a/libcnb-test/src/build.rs b/libcnb-test/src/build.rs index 4ad4e61f..98846b0a 100644 --- a/libcnb-test/src/build.rs +++ b/libcnb-test/src/build.rs @@ -8,8 +8,8 @@ use libcnb_package::dependency_graph::{get_dependencies, GetDependenciesError}; use libcnb_package::output::create_packaged_buildpack_dir_resolver; use libcnb_package::{find_cargo_workspace_root_dir, CargoProfile, FindCargoWorkspaceRootError}; use std::collections::BTreeMap; -use std::fs; use std::path::{Path, PathBuf}; +use std::{fs, io}; /// Packages the current crate as a buildpack into the provided directory. // TODO: Convert the `assert!` usages to an additional `PackageBuildpackError` variant instead: @@ -23,11 +23,11 @@ pub(crate) fn package_crate_buildpack( ) -> Result { let buildpack_toml = cargo_manifest_dir.join("buildpack.toml"); - assert!( - buildpack_toml.exists(), - "Could not package directory as buildpack! No `buildpack.toml` file exists at {}", - cargo_manifest_dir.display() - ); + if !buildpack_toml.exists() { + return Err(PackageBuildpackError::BuildpackDescriptorNotFound( + buildpack_toml, + )); + } let buildpack_descriptor: BuildpackDescriptor = read_toml_file(buildpack_toml) .map_err(PackageBuildpackError::CannotReadBuildpackDescriptor)?; @@ -53,7 +53,7 @@ pub(crate) fn package_buildpack( ) -> Result { let cargo_build_env = match cross_compile_assistance(target_triple.as_ref()) { CrossCompileAssistance::HelpText(help_text) => { - return Err(PackageBuildpackError::CrossCompileConfigurationError( + return Err(PackageBuildpackError::CrossCompileToolchainNotFound( help_text, )); } @@ -75,28 +75,21 @@ pub(crate) fn package_buildpack( let root_node = buildpack_dependency_graph .node_weights() - .find(|node| node.buildpack_id == buildpack_id.clone()); + .find(|node| &node.buildpack_id == buildpack_id) + .ok_or_else(|| { + PackageBuildpackError::BuildpackIdNotFound(buildpack_id.clone(), workspace_root_path) + })?; - assert!( - root_node.is_some(), - "Could not package directory as buildpack! No buildpack with id `{buildpack_id}` exists in the workspace at {}", - workspace_root_path.display() - ); - - let build_order = get_dependencies( - &buildpack_dependency_graph, - &[root_node.expect("The root node should exist")], - ) - .map_err(PackageBuildpackError::GetDependencies)?; + let build_order = get_dependencies(&buildpack_dependency_graph, &[root_node]) + .map_err(PackageBuildpackError::GetDependencies)?; let mut packaged_buildpack_dirs = BTreeMap::new(); for node in &build_order { let buildpack_destination_dir = buildpack_dir_resolver(&node.buildpack_id); - // TODO: Convert the `unwrap()` to an additional `PackageBuildpackError` variant instead: - // https://github.com/heroku/libcnb.rs/issues/710 - #[allow(clippy::unwrap_used)] - fs::create_dir_all(&buildpack_destination_dir).unwrap(); + fs::create_dir_all(&buildpack_destination_dir).map_err(|error| { + PackageBuildpackError::CannotCreateDirectory(buildpack_destination_dir.clone(), error) + })?; libcnb_package::package::package_buildpack( &node.path, @@ -114,12 +107,24 @@ pub(crate) fn package_buildpack( Ok(buildpack_dir_resolver(buildpack_id)) } -#[derive(Debug)] +#[derive(thiserror::Error, Debug)] pub(crate) enum PackageBuildpackError { + #[error("Couldn't find a buildpack.toml file at {0}")] + BuildpackDescriptorNotFound(PathBuf), + #[error("Couldn't find a buildpack with ID '{0}' in the workspace at {1}")] + BuildpackIdNotFound(BuildpackId, PathBuf), + #[error("I/O error creating directory {0}: {1}")] + CannotCreateDirectory(PathBuf, io::Error), + #[error("Couldn't read buildpack.toml: {0}")] CannotReadBuildpackDescriptor(TomlFileError), + #[error("Couldn't calculate buildpack dependency graph: {0}")] BuildBuildpackDependencyGraph(BuildBuildpackDependencyGraphError), - CrossCompileConfigurationError(String), + #[error("Couldn't find cross-compilation toolchain.\n\n{0}")] + CrossCompileToolchainNotFound(String), + #[error("Couldn't find Cargo workspace root: {0}")] FindCargoWorkspaceRoot(FindCargoWorkspaceRootError), + #[error("Couldn't get buildpack dependencies: {0}")] GetDependencies(GetDependenciesError), + #[error(transparent)] PackageBuildpack(libcnb_package::package::PackageBuildpackError), } diff --git a/libcnb-test/src/test_runner.rs b/libcnb-test/src/test_runner.rs index e4947801..80fe3318 100644 --- a/libcnb-test/src/test_runner.rs +++ b/libcnb-test/src/test_runner.rs @@ -107,7 +107,10 @@ impl TestRunner { &config.target_triple, &cargo_manifest_dir, buildpacks_target_dir.path(), - ).expect("Test references crate buildpack, but crate wasn't packaged as a buildpack. This is an internal libcnb-test error, please report any occurrences"); + ) + .unwrap_or_else(|error| { + panic!("Error packaging current crate as buildpack: {error}") + }); pack_command.buildpack(crate_buildpack_dir); } @@ -117,8 +120,11 @@ impl TestRunner { config.cargo_profile, &config.target_triple, &cargo_manifest_dir, - buildpacks_target_dir.path() - ).unwrap_or_else(|_| panic!("Test references buildpack `{buildpack_id}`, but this directory wasn't packaged as a buildpack. This is an internal libcnb-test error, please report any occurrences")); + buildpacks_target_dir.path(), + ) + .unwrap_or_else(|error| { + panic!("Error packaging buildpack '{buildpack_id}': {error}") + }); pack_command.buildpack(buildpack_dir); } diff --git a/libcnb-test/tests/integration_test.rs b/libcnb-test/tests/integration_test.rs index 640acead..f1588a61 100644 --- a/libcnb-test/tests/integration_test.rs +++ b/libcnb-test/tests/integration_test.rs @@ -146,8 +146,12 @@ fn packaging_failure_missing_buildpack_toml() { assert_eq!( err.downcast_ref::().unwrap(), &format!( - "Could not package directory as buildpack! No `buildpack.toml` file exists at {}", - env::var("CARGO_MANIFEST_DIR").unwrap() + "Error packaging current crate as buildpack: Couldn't find a buildpack.toml file at {}", + env::var("CARGO_MANIFEST_DIR") + .map(PathBuf::from) + .unwrap() + .join("buildpack.toml") + .display() ) ); } @@ -157,7 +161,7 @@ fn packaging_failure_missing_buildpack_toml() { // TODO: Fix the implementation to return the correct error message (that the buildpack.toml doesn't match the schema): // https://github.com/heroku/libcnb.rs/issues/708 #[should_panic( - expected = "Could not package directory as buildpack! No buildpack with id `libcnb-test/invalid-buildpack-toml` exists in the workspace at" + expected = "Error packaging buildpack 'libcnb-test/invalid-buildpack-toml': Couldn't find a buildpack with ID 'libcnb-test/invalid-buildpack-toml' in the workspace at" )] fn packaging_failure_invalid_buildpack_toml() { TestRunner::default().build( @@ -174,10 +178,8 @@ fn packaging_failure_invalid_buildpack_toml() { #[test] #[ignore = "integration test"] -// TODO: Fix the implementation to return the correct error message. Has the same cause as: -// https://github.com/heroku/libcnb.rs/issues/704 #[should_panic( - expected = "Test references buildpack `libcnb-test/composite-missing-package-toml`, but this directory wasn't packaged as a buildpack. This is an internal libcnb-test error, please report any occurrences" + expected = "Error packaging buildpack 'libcnb-test/composite-missing-package-toml': Could not read package.toml: IO error while reading/writing TOML file: No such file or directory (os error 2)" )] fn packaging_failure_composite_buildpack_missing_package_toml() { TestRunner::default().build( @@ -194,10 +196,8 @@ fn packaging_failure_composite_buildpack_missing_package_toml() { #[test] #[ignore = "integration test"] -// TODO: Fix the implementation to return the correct error message. Has the same cause as: -// https://github.com/heroku/libcnb.rs/issues/704 #[should_panic( - expected = "Test references buildpack `libcnb-test/invalid-cargo-toml`, but this directory wasn't packaged as a buildpack. This is an internal libcnb-test error, please report any occurrences" + expected = "Error packaging buildpack 'libcnb-test/invalid-cargo-toml': Obtaining Cargo metadata failed: `cargo metadata` exited with an error: " )] fn packaging_failure_invalid_cargo_toml() { TestRunner::default().build( @@ -212,10 +212,8 @@ fn packaging_failure_invalid_cargo_toml() { #[test] #[ignore = "integration test"] -// TODO: Fix the implementation to return the correct error message. Has the same cause as: -// https://github.com/heroku/libcnb.rs/issues/704 #[should_panic( - expected = "Test references buildpack `libcnb-test/compile-error`, but this directory wasn't packaged as a buildpack. This is an internal libcnb-test error, please report any occurrences" + expected = "Error packaging buildpack 'libcnb-test/compile-error': Building buildpack binaries failed: Failed to build binary target compile-error: Cargo unexpectedly exited with status exit status: 101" )] fn packaging_failure_compile_error() { TestRunner::default().build( @@ -246,7 +244,7 @@ fn packaging_failure_non_existent_workspace_buildpack() { assert_eq!( err.downcast_ref::().unwrap(), &format!( - "Could not package directory as buildpack! No buildpack with id `non-existent` exists in the workspace at {}", + "Error packaging buildpack 'non-existent': Couldn't find a buildpack with ID 'non-existent' in the workspace at {}", // There is currently no env var for determining the workspace root directly: // https://github.com/rust-lang/cargo/issues/3946 env::var("CARGO_MANIFEST_DIR") @@ -320,7 +318,7 @@ fn expected_pack_failure() { #[test] #[ignore = "integration test"] #[should_panic( - expected = "Could not package directory as buildpack! No `buildpack.toml` file exists at" + expected = "Error packaging current crate as buildpack: Couldn't find a buildpack.toml file" )] fn expected_pack_failure_still_panics_for_non_pack_failure() { TestRunner::default().build(