test(gui-client): multi-process smoke test for GUI + IPC service (#5672)

```[tasklist]
### Tasks
- [x] Check the GUI saves its settings file
- [x] Check the IPC service writes the device ID to disk
- [x] Check the GUI writes a log file (skipped - we already check if the exported zip has any files in it)
- [x] Run the crash file through `minidump-stackwalk`
- [x] Reach feature parity with the original smoke tests
- [x] Ready for review
- [x] Finish #5452
- [ ] Start on #5453 
```
This commit is contained in:
Reactor Scram
2024-07-04 21:10:31 +00:00
committed by GitHub
parent 4037a7bdd3
commit d0f68fc133
14 changed files with 350 additions and 215 deletions

View File

@@ -22,7 +22,7 @@ outputs:
value: ${{
(runner.os == 'Linux' && '--workspace') ||
(runner.os == 'macOS' && '-p connlib-client-apple -p connlib-client-shared -p firezone-tunnel -p snownet') ||
(runner.os == 'Windows' && '-p connlib-client-shared -p firezone-headless-client -p firezone-gui-client -p firezone-tunnel -p snownet') }}
(runner.os == 'Windows' && '-p connlib-client-shared -p firezone-headless-client -p firezone-gui-client -p firezone-tunnel -p gui-smoke-test -p snownet') }}
runs:
using: "composite"

View File

@@ -74,7 +74,7 @@ jobs:
name: "cargo test"
shell: bash
# Runs the Windows client smoke test, built in debug mode. We can't run it in release
# Runs the Tauri client smoke test, built in debug mode. We can't run it in release
# mode because of a known issue: <https://github.com/firezone/firezone/blob/456e044f882c2bb314e19cc44c0d19c5ad817b7c/rust/windows-client/src-tauri/src/client.rs#L162-L164>
gui-smoke-test:
name: gui-smoke-test-${{ matrix.runs-on }}
@@ -114,12 +114,9 @@ jobs:
- uses: taiki-e/install-action@v2
with:
tool: dump_syms,minidump-stackwalk
- name: Run smoke tests (Linux)
if: ${{ runner.os == 'Linux' }}
run: bash ../../scripts/tests/smoke-test-gui-linux.sh
- name: Run smoke tests (Windows)
if: ${{ runner.os == 'Windows' }}
run: bash ../../scripts/tests/smoke-test-gui-windows.sh
- name: Run smoke test
working-directory: ./rust
run: cargo run -p gui-smoke-test
headless-client:
name: headless-client-${{ matrix.test }}-${{ matrix.runs-on }}

21
rust/Cargo.lock generated
View File

@@ -2623,6 +2623,17 @@ dependencies = [
"syn 1.0.109",
]
[[package]]
name = "gui-smoke-test"
version = "0.1.0"
dependencies = [
"anyhow",
"libc",
"subprocess",
"tracing",
"tracing-subscriber",
]
[[package]]
name = "h2"
version = "0.3.26"
@@ -5956,6 +5967,16 @@ dependencies = [
"trackable 1.3.0",
]
[[package]]
name = "subprocess"
version = "0.2.9"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "0c2e86926081dda636c546d8c5e641661049d7562a68f5488be4a1f7f66f6086"
dependencies = [
"libc",
"winapi",
]
[[package]]
name = "subtle"
version = "2.5.0"

View File

@@ -8,6 +8,7 @@ members = [
"connlib/snownet",
"gateway",
"firezone-cli-utils",
"gui-smoke-test",
"headless-client",
"snownet-tests",
"phoenix-channel",

View File

@@ -238,13 +238,7 @@ pub enum Cmd {
},
Elevated,
OpenDeepLink(DeepLink),
/// SmokeTest gets its own subcommand because elevating would start a new process and trash the exit code
///
/// We could solve that by keeping the un-elevated process around, blocking on the elevated
/// child process, but then we'd always have an extra process hanging around.
///
/// It's also invalid for release builds, because we build the exe as a GUI app,
/// so Windows won't give us a valid exit code, it'll just detach from the terminal instantly.
/// SmokeTest gets its own subcommand for historical reasons.
SmokeTest,
}

View File

@@ -271,10 +271,16 @@ pub(crate) fn run(
Ok(())
}
#[cfg(not(debug_assertions))]
async fn smoke_test(_: CtlrTx) -> Result<()> {
bail!("Smoke test is not built for release binaries.");
}
/// Runs a smoke test and then asks Controller to exit gracefully
///
/// You can purposely fail this test by deleting the exported zip file during
/// the 10-second sleep.
#[cfg(debug_assertions)]
async fn smoke_test(ctlr_tx: CtlrTx) -> Result<()> {
let delay = 10;
tracing::info!("Will quit on purpose in {delay} seconds as part of the smoke test.");
@@ -323,6 +329,9 @@ async fn smoke_test(ctlr_tx: CtlrTx) -> Result<()> {
.context("Failed to remove zip file")?;
tracing::info!(?path, ?zip_len, "Exported log zip looks okay");
// Check that settings file and at least one log file were written
anyhow::ensure!(tokio::fs::try_exists(settings::advanced_settings_path()?).await?);
tracing::info!("Quitting on purpose because of `smoke-test` subcommand");
ctlr_tx
.send(ControllerRequest::SystemTrayMenu(TrayMenuEvent::Quit))

View File

@@ -6,7 +6,8 @@ use connlib_client_shared::file_logger;
use firezone_headless_client::known_dirs;
use serde::Serialize;
use std::{
fs, io,
fs,
io::{self, ErrorKind::NotFound},
path::{Path, PathBuf},
result::Result as StdResult,
};
@@ -111,7 +112,17 @@ pub(crate) async fn clear_logs_inner() -> Result<()> {
let mut result = Ok(());
for log_path in log_paths()?.into_iter().map(|x| x.src) {
let mut dir = tokio::fs::read_dir(log_path).await?;
let mut dir = match tokio::fs::read_dir(log_path).await {
Ok(x) => x,
Err(error) => {
if matches!(error.kind(), NotFound) {
// In smoke tests, the IPC service runs in debug mode, so it won't write any logs to disk. If the IPC service's log dir doesn't exist, we shouldn't crash, it's correct to simply not delete the non-existent files
return Ok(());
}
// But any other error like permissions errors, should bubble.
return Err(error.into());
}
};
while let Some(entry) = dir.next_entry().await? {
let path = entry.path();
if let Err(error) = tokio::fs::remove_file(&path).await {
@@ -191,7 +202,18 @@ fn add_dir_to_zip(
dst_stem: &Path,
) -> Result<()> {
let options = zip::write::SimpleFileOptions::default();
for entry in fs::read_dir(src_dir).context("Failed to `read_dir` log dir")? {
let dir = match fs::read_dir(src_dir) {
Ok(x) => x,
Err(error) => {
if matches!(error.kind(), NotFound) {
// In smoke tests, the IPC service runs in debug mode, so it won't write any logs to disk. If the IPC service's log dir doesn't exist, we shouldn't crash, it's correct to simply not add any files to the zip
return Ok(());
}
// But any other error like permissions errors, should bubble.
return Err(error.into());
}
};
for entry in dir {
let entry = entry.context("Got bad entry from `read_dir`")?;
let Some(path) = dst_stem
.join(entry.file_name())

View File

@@ -39,7 +39,7 @@ impl Default for AdvancedSettings {
}
}
fn advanced_settings_path() -> Result<PathBuf> {
pub(crate) fn advanced_settings_path() -> Result<PathBuf> {
Ok(known_dirs::settings()
.context("`known_dirs::settings` failed")?
.join("advanced_settings.json"))

View File

@@ -0,0 +1,11 @@
[package]
name = "gui-smoke-test"
version = "0.1.0"
edition = "2021"
[dependencies]
anyhow = { version = "1.0" }
libc = "0.2.155"
subprocess = "0.2.9"
tracing = { workspace = true }
tracing-subscriber = { version = "0.3.17", features = ["env-filter"] }

View File

@@ -0,0 +1,229 @@
// Invoke with `cargo run --bin gui-smoke-test`
//
// Starts up the IPC service and GUI app and lets them run for a bit
use anyhow::{bail, Context as _, Result};
use std::{
ffi::OsStr,
path::{Path, PathBuf},
};
use subprocess::Exec;
#[cfg(target_os = "linux")]
const FZ_GROUP: &str = "firezone-client";
const GUI_NAME: &str = "firezone-gui-client";
const IPC_NAME: &str = "firezone-client-ipc";
#[cfg(target_os = "linux")]
const EXE_EXTENSION: &str = "";
#[cfg(target_os = "windows")]
const EXE_EXTENSION: &str = "exe";
fn main() -> Result<()> {
tracing_subscriber::fmt::init();
let app = App::new()?;
// Run `cargo build`
build_binary(GUI_NAME)?;
build_binary(IPC_NAME)?;
dump_syms()?;
// Run normal smoke test
let mut ipc_service = ipc_service_command().arg("run-smoke-test").popen()?;
let mut gui = app
.gui_command(&["--no-deep-links", "smoke-test"])? // Disable deep links because they don't work in the headless CI environment
.popen()?;
gui.wait()?.fz_exit_ok().context("GUI process")?;
ipc_service.wait()?.fz_exit_ok().context("IPC service")?;
// Force the GUI to crash and then try to read the crash dump
let mut ipc_service = ipc_service_command().arg("run-smoke-test").popen()?;
let mut gui = app.gui_command(&["--no-deep-links", "--crash"])?.popen()?;
// Ignore exit status here since we asked the GUI to crash on purpose
gui.wait()?;
ipc_service.wait()?.fz_exit_ok().context("IPC service")?;
app.check_crash_dump()?;
Ok(())
}
struct App {
#[cfg(target_os = "linux")]
username: String,
}
impl App {
fn check_crash_dump(&self) -> Result<()> {
Exec::cmd("minidump-stackwalk")
.args(&[
OsStr::new("--symbols-path"),
syms_path().as_os_str(),
self.crash_dump_path().as_os_str(),
])
.join()?
.fz_exit_ok()?;
Ok(())
}
}
#[cfg(target_os = "linux")]
impl App {
fn new() -> Result<Self> {
// Needed to manipulate the group membership inside CI
let username = std::env::var("USER")?;
// Create the firezone group if needed
Exec::cmd("sudo")
.args(&[
"groupadd", "--force", // Exit with success if the group already exists
FZ_GROUP,
])
.join()?
.fz_exit_ok()?;
// Add ourself to the firezone group
Exec::cmd("sudo")
.args(&["usermod", "--append", "--groups", FZ_GROUP, &username])
.join()?
.fz_exit_ok()?;
Ok(Self { username })
}
fn crash_dump_path(&self) -> PathBuf {
Path::new("/home")
.join(&self.username)
.join(".cache/dev.firezone.client/data/logs/last_crash.dmp")
}
// `args` can't just be appended because of the `xvfb-run` wrapper
fn gui_command(&self, args: &[&str]) -> Result<Exec> {
let gui_path = gui_path().canonicalize()?;
let args: Vec<_> = [
"--auto-servernum",
gui_path
.to_str()
.context("Should be able to convert Path to &str")?, // For some reason `xvfb-run` doesn't just use our current working dir
]
.into_iter()
.chain(args.iter().copied())
.collect();
let xvfb = Exec::cmd("xvfb-run").args(&args).to_cmdline_lossy();
tracing::debug!(?xvfb);
let cmd = Exec::cmd("sudo") // We need `sudo` to run `su`
.args(&[
"--preserve-env",
"su", // We need `su` to get a login shell as ourself
"--login", // And we need a login shell so that the group membership will take effect immediately
"--whitelist-environment=XDG_RUNTIME_DIR",
&self.username,
"--command",
&xvfb,
])
.env("WEBKIT_DISABLE_COMPOSITING_MODE", "1"); // Might help with CI
Ok(cmd)
}
}
#[cfg(target_os = "windows")]
impl App {
fn new() -> Result<Self> {
Ok(Self {})
}
fn crash_dump_path(&self) -> PathBuf {
let app_data = std::env::var("LOCALAPPDATA").expect("$LOCALAPPDATA should be set");
PathBuf::from(app_data)
.join("dev.firezone.client/data/logs")
.join("last_crash.dmp")
}
// Strange signature needed to match Linux
fn gui_command(&self, args: &[&str]) -> Result<Exec> {
Ok(Exec::cmd(gui_path()).args(args))
}
}
fn build_binary(name: &str) -> Result<()> {
Exec::cmd("cargo")
.args(&["build", "--bin", name])
.join()?
.fz_exit_ok()?;
Ok(())
}
// Get debug symbols from the exe / pdb
fn dump_syms() -> Result<()> {
Exec::cmd("dump_syms")
.args(&[
debug_db_path().as_os_str(),
gui_path().as_os_str(),
OsStr::new("--output"),
syms_path().as_os_str(),
])
.join()?
.fz_exit_ok()?;
Ok(())
}
#[cfg(target_os = "linux")]
fn debug_db_path() -> PathBuf {
Path::new("target").join("debug").join(GUI_NAME)
}
#[cfg(target_os = "windows")]
fn debug_db_path() -> PathBuf {
Path::new("target")
.join("debug")
.join("firezone_gui_client.pdb")
}
#[cfg(target_os = "linux")]
fn ipc_service_command() -> Exec {
Exec::cmd("sudo").args(&[OsStr::new("--preserve-env"), ipc_path().as_os_str()])
}
#[cfg(target_os = "windows")]
fn ipc_service_command() -> Exec {
Exec::cmd(ipc_path())
}
// `ExitStatus::exit_ok` is nightly, so we add an equivalent here
trait ExitStatusExt {
fn fz_exit_ok(&self) -> Result<()>;
}
impl ExitStatusExt for subprocess::ExitStatus {
fn fz_exit_ok(&self) -> Result<()> {
if !self.success() {
bail!("Subprocess should exit with success");
}
Ok(())
}
}
fn gui_path() -> PathBuf {
Path::new("target")
.join("debug")
.join(GUI_NAME)
.with_extension(EXE_EXTENSION)
}
fn ipc_path() -> PathBuf {
Path::new("target")
.join("debug")
.join(IPC_NAME)
.with_extension(EXE_EXTENSION)
}
fn syms_path() -> PathBuf {
gui_path().with_extension("syms")
}

View File

@@ -1,11 +1,26 @@
use anyhow::{Context as _, Result};
use atomicwrites::{AtomicFile, OverwriteBehavior};
use std::{fs, io::Write, path::Path};
use std::{
fs,
io::Write,
path::{Path, PathBuf},
};
pub(crate) struct DeviceId {
pub(crate) id: String,
}
/// Returns the path of the randomly-generated Firezone device ID
///
/// e.g. `C:\ProgramData\dev.firezone.client/firezone-id.json` or
/// `/var/lib/dev.firezone.client/config/firezone-id.json`.
pub(crate) fn path() -> Result<PathBuf> {
let path = crate::known_dirs::ipc_service_config()
.context("Failed to compute path for firezone-id file")?
.join("firezone-id.json");
Ok(path)
}
/// Returns the device ID, generating it and saving it to disk if needed.
///
/// Per <https://github.com/firezone/firezone/issues/2697> and <https://github.com/firezone/firezone/issues/2711>,
@@ -15,20 +30,20 @@ pub(crate) struct DeviceId {
///
/// Errors: If the disk is unwritable when initially generating the ID, or unwritable when re-generating an invalid ID.
pub(crate) fn get_or_create() -> Result<DeviceId> {
let dir = crate::known_dirs::ipc_service_config()
.context("Failed to compute path for firezone-id file")?;
let path = path()?;
let dir = path
.parent()
.context("Device ID path should always have a parent")?;
// Make sure the dir exists, and fix its permissions so the GUI can write the
// log filter file
fs::create_dir_all(&dir).context("Failed to create dir for firezone-id")?;
set_permissions(&dir).with_context(|| {
fs::create_dir_all(dir).context("Failed to create dir for firezone-id")?;
set_permissions(dir).with_context(|| {
format!(
"Couldn't set permissions on IPC service config dir `{}`",
dir.display()
)
})?;
let path = dir.join("firezone-id.json");
// Try to read it from the disk
if let Some(j) = fs::read_to_string(&path)
.ok()

View File

@@ -51,6 +51,7 @@ enum Cmd {
Install,
Run,
RunDebug,
RunSmokeTest,
}
impl Default for Cmd {
@@ -84,6 +85,7 @@ pub fn run_only_ipc_service() -> Result<()> {
Cmd::Install => platform::install_ipc_service(),
Cmd::Run => platform::run_ipc_service(cli.common),
Cmd::RunDebug => run_debug_ipc_service(),
Cmd::RunSmokeTest => run_smoke_test(),
}
}
@@ -111,6 +113,29 @@ fn run_debug_ipc_service() -> Result<()> {
})
}
#[cfg(not(debug_assertions))]
fn run_smoke_test() -> Result<()> {
anyhow::bail!("Smoke test is not built for release binaries.");
}
#[cfg(debug_assertions)]
fn run_smoke_test() -> Result<()> {
crate::setup_stdout_logging()?;
let rt = tokio::runtime::Runtime::new()?;
let _guard = rt.enter();
// Couldn't get the loop to work here yet, so SIGHUP is not implemented
rt.block_on(async {
if let Ok(result) = tokio::time::timeout(Duration::from_secs(12), ipc_listen()).await {
return Err(result.unwrap_err());
}
tracing::info!("IPC service smoke test timed out as expected");
anyhow::ensure!(tokio::fs::try_exists(crate::device_id::path()?).await?);
Ok(())
})?;
Ok(())
}
async fn ipc_listen() -> Result<std::convert::Infallible> {
// Create the device ID and IPC service config dir if needed
// This also gives the GUI a safe place to put the log filter config

View File

@@ -1,111 +0,0 @@
#!/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.
# <https://github.com/firezone/firezone/actions/runs/9602401296/job/26483176628#step:11:12>
set -euox pipefail
BUNDLE_ID="dev.firezone.client"
FZ_GROUP="firezone-client"
LOGS_PATH="$HOME/.cache/$BUNDLE_ID/data/logs"
DUMP_PATH="$LOGS_PATH/last_crash.dmp"
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
export RUST_LOG=firezone_gui_client=debug,warn
export WEBKIT_DISABLE_COMPOSITING_MODE=1
# The dwp doesn't actually do anything if the exe already has all the debug info
# Getting this to coordinate between Linux and Windows is tricky
dump_syms ../target/debug/firezone-gui-client --output "$SYMS_PATH"
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
sudo --preserve-env \
su --login "$USER" --command \
"xvfb-run --auto-servernum $PWD/../target/debug/$PACKAGE $*"
}
function smoke_test() {
# Make sure the files we want to check don't exist on the system yet
stat "$LOGS_PATH" && exit 1
stat "$SETTINGS_PATH" && exit 1
# TODO: The device ID will be written by the tunnel, not the GUI, so we can't check that.
# stat "$DEVICE_ID_PATH" && exit 1
stat "$RAN_BEFORE_PATH" && exit 1
# Run the smoke test normally
if ! run_fz_gui --no-deep-links smoke-test
then
minidump-stackwalk --symbols-path "$SYMS_PATH" "$DUMP_PATH"
exit 1
fi
# Note the device ID
# DEVICE_ID_1=$(cat "$DEVICE_ID_PATH")
# 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
# 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
stat "$RAN_BEFORE_PATH" && exit 1
# Run the test again and make sure the device ID is not changed
run_fz_gui --no-deep-links 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
# Clean up the files but not the folders
rm -rf "$LOGS_PATH"
rm "$SETTINGS_PATH"
# rm "$DEVICE_ID_PATH"
rm -f "$RAN_BEFORE_PATH"
}
function crash_test() {
# Delete the crash file if present
rm -f "$DUMP_PATH"
# Fail if it returns success, this is supposed to crash
run_fz_gui --crash --no-deep-links && exit 1
# Fail if the crash file wasn't written
stat "$DUMP_PATH"
}
function get_stacktrace() {
minidump-stackwalk --symbols-path "$SYMS_PATH" "$DUMP_PATH"
}
# Run the tests twice to make sure it's okay for the directories to stay intact
smoke_test
smoke_test
crash_test
crash_test
get_stacktrace
# Clean up
rm "$DUMP_PATH"

View File

@@ -1,78 +0,0 @@
#!/usr/bin/env bash
# Usage: This is made for CI, so it will change system-wide files without asking.
# Read it before running on a dev system.
# This script must run from an elevated shell so that Firezone won't try to elevate.
set -euox pipefail
# This prevents a `shellcheck` lint warning about using an unset CamelCase var
if [[ -z "$ProgramData" ]]; then
echo "The env var \$ProgramData should be set to \`C:\ProgramData\` or similar"
exit 1
fi
BUNDLE_ID="dev.firezone.client"
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"
)
# Make sure the files we want to check don't exist on the system yet
# I'm leaning on ChatGPT and `shellcheck` for the syntax here.
# Maybe this is about ready to be translated into Python or Rust.
for file in "${files[@]}"
do
stat "$file" && exit 1
done
# Run the smoke test normally
$PWD/../target/debug/$PACKAGE smoke-test
# Make sure the files were written in the right paths
for file in "${files[@]}"
do
stat "$file"
done
# Clean up so the test can be cycled
for file in "${files[@]}"
do
rm "$file"
done
}
function crash_test() {
# Delete the crash file if present
rm -f "$DUMP_PATH"
# Fail if it returns success, this is supposed to crash
$PWD/../target/debug/$PACKAGE --crash && exit 1
# Fail if the crash file wasn't written
stat "$DUMP_PATH"
}
function get_stacktrace() {
# Per `crash_handling.rs`
SYMS_PATH="../target/debug/firezone-gui-client.syms"
dump_syms ../target/debug/firezone_gui_client.pdb ../target/debug/firezone-gui-client.exe --output "$SYMS_PATH"
ls -lash ../target/debug
minidump-stackwalk --symbols-path "$SYMS_PATH" "$DUMP_PATH"
}
smoke_test
smoke_test
crash_test
get_stacktrace
# Clean up
rm "$DUMP_PATH"