From b8f92ed8129c3ded4e31a0f813b7d3feb344607e Mon Sep 17 00:00:00 2001 From: Reactor Scram Date: Fri, 21 Jun 2024 14:05:26 +0000 Subject: [PATCH] test(gui-client): fix zip file export in smoke tests (#5465) Closes #5464 These were silently broken, it was exporting an empty zip and passing the test anyway. So this PR will cause the test to fail if the zip wasn't fully exported, and then it will fix the export. --- rust/gui-client/src-tauri/src/client/gui.rs | 7 +++--- .../src-tauri/src/client/logging.rs | 9 ++++--- scripts/tests/smoke-test-gui-linux.sh | 15 ++++++++--- scripts/tests/smoke-test-gui-windows.sh | 25 ++++++------------- 4 files changed, 28 insertions(+), 28 deletions(-) diff --git a/rust/gui-client/src-tauri/src/client/gui.rs b/rust/gui-client/src-tauri/src/client/gui.rs index bb08e3037..a0100dcbd 100644 --- a/rust/gui-client/src-tauri/src/client/gui.rs +++ b/rust/gui-client/src-tauri/src/client/gui.rs @@ -170,8 +170,7 @@ pub(crate) fn run( let ctlr_tx = ctlr_tx.clone(); tokio::spawn(async move { if let Err(error) = smoke_test(ctlr_tx).await { - tracing::error!(?error, "Error during smoke test"); - tracing::error!("Crashing on purpose so a dev can see our stacktraces"); + tracing::error!(?error, "Error during smoke test, crashing on purpose so a dev can see our stacktraces"); unsafe { sadness_generator::raise_segfault() } } }); @@ -324,8 +323,8 @@ async fn smoke_test(ctlr_tx: CtlrTx) -> Result<()> { .await .context("Failed to get zip file metadata")? .len(); - if zip_len == 0 { - bail!("Exported log zip has 0 bytes"); + if zip_len <= 22 { + bail!("Exported log zip just has the file header"); } tokio::fs::remove_file(&path) .await diff --git a/rust/gui-client/src-tauri/src/client/logging.rs b/rust/gui-client/src-tauri/src/client/logging.rs index 767003516..502231f7e 100644 --- a/rust/gui-client/src-tauri/src/client/logging.rs +++ b/rust/gui-client/src-tauri/src/client/logging.rs @@ -82,7 +82,7 @@ pub(crate) async fn clear_logs() -> StdResult<(), String> { #[tauri::command] pub(crate) async fn export_logs(managed: tauri::State<'_, Managed>) -> StdResult<(), String> { - export_logs_inner(managed.ctlr_tx.clone()).map_err(|e| e.to_string()) + show_export_dialog(managed.ctlr_tx.clone()).map_err(|e| e.to_string()) } #[derive(Clone, Default, Serialize)] @@ -112,7 +112,7 @@ pub(crate) async fn clear_logs_inner() -> Result<()> { } /// Pops up the "Save File" dialog -pub(crate) fn export_logs_inner(ctlr_tx: CtlrTx) -> Result<()> { +fn show_export_dialog(ctlr_tx: CtlrTx) -> Result<()> { let now = chrono::Local::now(); let datetime_string = now.format("%Y_%m_%d-%H-%M"); let stem = PathBuf::from(format!("firezone_logs_{datetime_string}")); @@ -144,15 +144,18 @@ pub(crate) fn export_logs_inner(ctlr_tx: CtlrTx) -> Result<()> { /// * `stem` - A directory containing all the log files inside the zip archive, to avoid creating a ["tar bomb"](https://www.linfo.org/tarbomb.html). This comes from the automatically-generated name of the archive, even if the user changes it to e.g. `logs.zip` pub(crate) async fn export_logs_to(path: PathBuf, stem: PathBuf) -> Result<()> { tracing::info!("Exporting logs to {path:?}"); + // Use a temp path so that if the export fails we don't end up with half a zip file + let temp_path = path.with_extension(".zip-partial"); // TODO: Consider https://github.com/Majored/rs-async-zip/issues instead of `spawn_blocking` spawn_blocking(move || { - let f = fs::File::create(path).context("Failed to create zip file")?; + let f = fs::File::create(&temp_path).context("Failed to create zip file")?; let mut zip = zip::ZipWriter::new(f); for log_path in log_paths().context("Can't compute log paths")? { add_dir_to_zip(&mut zip, &log_path.src, &stem.join(log_path.dst))?; } zip.finish().context("Failed to finish zip file")?; + fs::rename(&temp_path, &path)?; Ok::<_, anyhow::Error>(()) }) .await diff --git a/scripts/tests/smoke-test-gui-linux.sh b/scripts/tests/smoke-test-gui-linux.sh index dfb2fc5f3..40a3f3a39 100755 --- a/scripts/tests/smoke-test-gui-linux.sh +++ b/scripts/tests/smoke-test-gui-linux.sh @@ -1,15 +1,20 @@ #!/usr/bin/env bash +# Put `set -euox` in the top-level scripts directly. If it's only sourced, +# and the source path is wrong, it will not throw an error and it'll be hard +# to debug. +# set -euox pipefail BUNDLE_ID="dev.firezone.client" FZ_GROUP="firezone-client" -#DEVICE_ID_PATH="/var/lib/$BUNDLE_ID/config/firezone-id.json" LOGS_PATH="$HOME/.cache/$BUNDLE_ID/data/logs" + DUMP_PATH="$LOGS_PATH/last_crash.dmp" -SETTINGS_PATH="$HOME/.config/$BUNDLE_ID/config/advanced_settings.json" +IPC_LOGS_PATH="/var/log/$BUNDLE_ID" RAN_BEFORE_PATH="$HOME/.local/share/$BUNDLE_ID/data/ran_before.txt" +SETTINGS_PATH="$HOME/.config/$BUNDLE_ID/config/advanced_settings.json" SYMS_PATH="../target/debug/firezone-gui-client.syms" PACKAGE=firezone-gui-client @@ -26,6 +31,9 @@ ls -lash ../target/debug sudo groupadd --force "$FZ_GROUP" sudo adduser "$USER" "$FZ_GROUP" +# Make the IPC log dir so that the zip export doesn't bail out +sudo mkdir -p "$IPC_LOGS_PATH" + function run_fz_gui() { pwd # Does what it says @@ -55,7 +63,8 @@ function smoke_test() { # Make sure the files were written in the right paths # TODO: Inject some bogus sign-in sequence to test the actor_name file # https://stackoverflow.com/questions/41321092 - bash -c "stat \"${LOGS_PATH}/\"connlib*log" + # TODO: Smoke test the IPC service + # bash -c "stat \"${LOGS_PATH}/\"connlib*log" stat "$SETTINGS_PATH" # stat "$DEVICE_ID_PATH" # `ran_before` is now only written after a successful sign-in diff --git a/scripts/tests/smoke-test-gui-windows.sh b/scripts/tests/smoke-test-gui-windows.sh index 0d9bd24d1..9fd9e8d02 100755 --- a/scripts/tests/smoke-test-gui-windows.sh +++ b/scripts/tests/smoke-test-gui-windows.sh @@ -3,7 +3,7 @@ # Read it before running on a dev system. # This script must run from an elevated shell so that Firezone won't try to elevate. -source "./scripts/tests/lib.sh" +set -euox pipefail # This prevents a `shellcheck` lint warning about using an unset CamelCase var if [[ -z "$ProgramData" ]]; then @@ -12,15 +12,18 @@ if [[ -z "$ProgramData" ]]; then fi BUNDLE_ID="dev.firezone.client" -DEVICE_ID_PATH="$ProgramData/$BUNDLE_ID/config/firezone-id.json" DUMP_PATH="$LOCALAPPDATA/$BUNDLE_ID/data/logs/last_crash.dmp" +IPC_LOGS_PATH="$ProgramData/$BUNDLE_ID/data/logs" PACKAGE=firezone-gui-client +# Make the IPC log dir so that the zip export doesn't bail out +mkdir -p "$IPC_LOGS_PATH" + function smoke_test() { + # This array used to have more items + # TODO: Smoke-test the IPC service files=( "$LOCALAPPDATA/$BUNDLE_ID/config/advanced_settings.json" - "$LOCALAPPDATA/$BUNDLE_ID/data/wintun.dll" - "$DEVICE_ID_PATH" ) # Make sure the files we want to check don't exist on the system yet @@ -34,25 +37,11 @@ function smoke_test() { # Run the smoke test normally cargo run --bin "$PACKAGE" -- smoke-test - # Note the device ID - DEVICE_ID_1=$(cat "$DEVICE_ID_PATH") - - # Run the test again and make sure the device ID is not changed - cargo run --bin "$PACKAGE" -- smoke-test - DEVICE_ID_2=$(cat "$DEVICE_ID_PATH") - - if [ "$DEVICE_ID_1" != "$DEVICE_ID_2" ] - then - echo "The device ID should not change if the file is intact between runs" - exit 1 - fi - # Make sure the files were written in the right paths for file in "${files[@]}" do stat "$file" done - stat "$LOCALAPPDATA/$BUNDLE_ID/data/logs/"connlib*log # Clean up so the test can be cycled for file in "${files[@]}"