diff --git a/tests/suites/features/keycloak-provider-refresh/tests/provider_refresh.rs b/tests/suites/features/keycloak-provider-refresh/tests/provider_refresh.rs index fb57396901..d66caad361 100644 --- a/tests/suites/features/keycloak-provider-refresh/tests/provider_refresh.rs +++ b/tests/suites/features/keycloak-provider-refresh/tests/provider_refresh.rs @@ -27,108 +27,107 @@ async fn revoked_refresh_grant_requires_user_reauthorization(cx: &mut Context) - let profile = write_profile(&profile_id, kc.keycloak())?; let profile_path = profile.path().to_string_lossy().into_owned(); - let result = async { - kc.cli(["profile", "import", "--file", &profile_path]) - .await? - .success()?; - kc.cli_with_env( - &[(CREDENTIAL_KEY, &grant.access_token)], - [ - "provider", - "create", - "--name", - &provider, - "--type", - &profile_id, - "--credential", - CREDENTIAL_KEY, - ], - ) - .await? - .success()?; - let client_id = format!("client_id={}", kc.keycloak().client_id()); - kc.cli_with_env( - &[("KEYCLOAK_REFRESH_TOKEN", &grant.refresh_token)], - [ - "provider", - "refresh", - "configure", - &provider, - "--credential-key", - CREDENTIAL_KEY, - "--strategy", - "oauth2-refresh-token", - "--material", - &client_id, - "--secret-material-env", - "refresh_token=KEYCLOAK_REFRESH_TOKEN", - ], - ) + // Register each delete before its create, so cleanup also covers a create + // the gateway applied but the CLI reported as failed. Both deletes succeed + // when there is nothing to delete. + delete_on_cleanup(&kc, "profile", &profile_id); + kc.cli(["profile", "import", "--file", &profile_path]) .await? .success()?; + delete_on_cleanup(&kc, "provider", &provider); + kc.cli_with_env( + &[(CREDENTIAL_KEY, &grant.access_token)], + [ + "provider", + "create", + "--name", + &provider, + "--type", + &profile_id, + "--credential", + CREDENTIAL_KEY, + ], + ) + .await? + .success()?; + let client_id = format!("client_id={}", kc.keycloak().client_id()); + kc.cli_with_env( + &[("KEYCLOAK_REFRESH_TOKEN", &grant.refresh_token)], + [ + "provider", + "refresh", + "configure", + &provider, + "--credential-key", + CREDENTIAL_KEY, + "--strategy", + "oauth2-refresh-token", + "--material", + &client_id, + "--secret-material-env", + "refresh_token=KEYCLOAK_REFRESH_TOKEN", + ], + ) + .await? + .success()?; - let refresh = |verb: &'static str| { - [ - "provider", - "refresh", - verb, - provider.as_str(), - "--credential-key", - CREDENTIAL_KEY, - ] - }; + let refresh = |verb: &'static str| { + [ + "provider", + "refresh", + verb, + provider.as_str(), + "--credential-key", + CREDENTIAL_KEY, + ] + }; - kc.cli(refresh("rotate")).await?.success()?; - let status = kc.cli(refresh("status")).await?.success()?; - testkit::ensure!( - combined(&status).contains("refreshed"), - "a valid refresh did not reach the refreshed state:\n{}", - combined(&status) - ); + kc.cli(refresh("rotate")).await?.success()?; + let status = kc.cli(refresh("status")).await?.success()?; + testkit::ensure!( + combined(&status).contains("refreshed"), + "a valid refresh did not reach the refreshed state:\n{}", + combined(&status) + ); - kc.keycloak().revoke(&grant.refresh_token).await?; - let rotate = kc.cli(refresh("rotate")).await?.failure()?; - testkit::ensure!( - combined(&rotate).contains("invalid_grant"), - "a revoked refresh failed without invalid_grant:\n{}", - combined(&rotate) - ); + kc.keycloak().revoke(&grant.refresh_token).await?; + let rotate = kc.cli(refresh("rotate")).await?.failure()?; + testkit::ensure!( + combined(&rotate).contains("invalid_grant"), + "a revoked refresh failed without invalid_grant:\n{}", + combined(&rotate) + ); - let status = combined(&kc.cli(refresh("status")).await?.success()?); - for expected in [ - "reauthorization_required", - "reauthorize", - "oauth_invalid_grant", - ] { - testkit::ensure!( - status.contains(expected), - "the revoked refresh status omits {expected}:\n{}", - status - ); - } - // A parked refresh is scheduled at i64::MAX; it must not render as a date. + let status = combined(&kc.cli(refresh("status")).await?.success()?); + for expected in [ + "reauthorization_required", + "reauthorize", + "oauth_invalid_grant", + ] { testkit::ensure!( - !status.contains("292278994"), - "the parked refresh rendered its scheduling sentinel as a date:\n{}", + status.contains(expected), + "the revoked refresh status omits {expected}:\n{}", status ); - Ok(()) } - .await; + // A parked refresh is scheduled at i64::MAX; it must not render as a date. + testkit::ensure!( + !status.contains("292278994"), + "the parked refresh rendered its scheduling sentinel as a date:\n{}", + status + ); + Ok(()) +} - // testkit cleans up sandboxes, not providers or profiles, so remove them - // here whether or not the test passed. A panic or a killed test process - // skips this; the names are unique to the run, so leftovers don't collide. - // A failed cleanup doesn't fail the test, but it is reported. - for args in [ - ["provider", "delete", provider.as_str()], - ["profile", "delete", profile_id.as_str()], - ] { - if let Err(error) = kc.cli(args).await.and_then(CliOutput::success) { - eprintln!("cleanup failed: {error}"); - } - } - result +/// Delete the `kind` named `name` when the test ends, since testkit only +/// cleans up sandboxes by itself. +fn delete_on_cleanup(cx: &Context, kind: &'static str, name: &str) { + let cli = cx.cli_handle(); + let name = name.to_string(); + cx.on_cleanup(format!("delete {kind} {name}"), async move || { + cli.run([kind, "delete", &name]).await?.success()?; + Ok(()) + }); } fn combined(output: &CliOutput) -> String { diff --git a/tests/support/keycloak/src/lib.rs b/tests/support/keycloak/src/lib.rs index 76a4e9269f..54341b2c6c 100644 --- a/tests/support/keycloak/src/lib.rs +++ b/tests/support/keycloak/src/lib.rs @@ -34,7 +34,8 @@ //! ``` //! //! The fixture owns no Keycloak state, so it has nothing to clean up. Tests -//! that create resources in Keycloak or `OpenShell` must remove them. +//! that create resources in Keycloak or `OpenShell` must remove them, for +//! example with [`Context::on_cleanup`]. use std::fmt; use std::ops::{Deref, DerefMut}; diff --git a/tests/testkit/Cargo.lock b/tests/testkit/Cargo.lock index 6889775edc..01973c00e4 100644 --- a/tests/testkit/Cargo.lock +++ b/tests/testkit/Cargo.lock @@ -1994,6 +1994,7 @@ dependencies = [ "serde", "serde_json", "tokio", + "tonic", ] [[package]] diff --git a/tests/testkit/README.md b/tests/testkit/README.md index ae71d0f81b..1945067e74 100644 --- a/tests/testkit/README.md +++ b/tests/testkit/README.md @@ -37,9 +37,27 @@ async fn long_running_exec_delete(cx: &mut Context) -> testkit::Result { refused, assert the failure in the body, for example with `cx.cli([...]).await?.failure()?` and `testkit::ensure!`. -Cleanup runs after every test, whether it passes, fails, or panics. It only -deletes sandboxes created through `cx.sandbox(...)`. Delete anything else a -test creates, such as providers, in the test itself. +Cleanup runs after every test, whether it passes, fails, or panics. It +deletes the sandboxes created through `cx.sandbox(...)` or tracked with +`cx.track(name)`, and runs the hooks registered with `cx.on_cleanup`. Register +a hook for anything else a test creates, such as a provider. Register it before +the create when the delete succeeds for a missing resource, so it also covers a +create the gateway applied but the CLI reported as failed: + +```rust +let cli = cx.cli_handle(); +let name = provider.clone(); +cx.on_cleanup(format!("delete provider {provider}"), async move || { + cli.run(["provider", "delete", &name]).await?.success()?; + Ok(()) +}); +cx.cli(["provider", "create", "--name", &provider /* ... */]).await?.success()?; +``` + +Hooks and sandbox deletes run one at a time, last registered first, each +limited to `testkit::CLEANUP_TIMEOUT`. A failure, panic, or timeout is +reported, the remaining hooks still run, and a test that otherwise passed +fails. A killed test process skips cleanup, so names are unique to the run. ## What a test can do @@ -54,7 +72,9 @@ test creates, such as providers, in the test itself. up. `cx.adopt(name)` returns a `Sandbox` handle for one the CLI created. - `cx.expect_listed` and `cx.expect_absent` to check a sandbox in list and get. - `cx.cli([...])` to run the installed CLI against the same gateway, with - `success()`, `failure()`, and `json()` on the output. + `success()`, `failure()`, and `json()` on the output. `cx.cli_handle()` + returns that CLI for code that can't borrow `cx`, such as a cleanup hook. +- `cx.on_cleanup(name, hook)` to remove something when the test ends. - `cx.client()` for SDK calls testkit does not wrap. - `poll_until` for waiting on other state. diff --git a/tests/testkit/testkit/Cargo.toml b/tests/testkit/testkit/Cargo.toml index cfcf0391ee..b8570fbe2c 100644 --- a/tests/testkit/testkit/Cargo.toml +++ b/tests/testkit/testkit/Cargo.toml @@ -24,3 +24,6 @@ rand = "0.9" serde = "1" serde_json = "1" tokio = { version = "1.43", features = ["macros", "process", "rt", "time"] } + +[dev-dependencies] +tonic = "0.14" diff --git a/tests/testkit/testkit/src/cleanup.rs b/tests/testkit/testkit/src/cleanup.rs new file mode 100644 index 0000000000..f1a1e0228e --- /dev/null +++ b/tests/testkit/testkit/src/cleanup.rs @@ -0,0 +1,216 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +//! The cleanup hooks a test registers, run after its body. + +use std::any::Any; +use std::panic::AssertUnwindSafe; +use std::sync::{Arc, Mutex, PoisonError}; +use std::time::Duration; + +use futures::FutureExt; +use futures::future::BoxFuture; + +use crate::error::Result; + +/// How long one cleanup hook may take. +pub const CLEANUP_TIMEOUT: Duration = Duration::from_mins(2); + +type HookFn = Box BoxFuture<'static, Result> + Send>; + +struct Hook { + /// What the hook does, for reporting a failure. + name: String, + /// Registers the hook at most once; see [`Cleanup::push_keyed`]. + key: Option, + run: HookFn, +} + +impl Hook { + fn new(name: String, key: Option, hook: F) -> Self + where + F: FnOnce() -> Fut + Send + 'static, + Fut: Future + Send + 'static, + { + Self { + name, + key, + run: Box::new(move || hook().boxed()), + } + } +} + +/// An ordered list of cleanup hooks, shared by a test's context and the +/// handles that register or cancel hooks. +#[derive(Clone, Default)] +pub(crate) struct Cleanup { + hooks: Arc>>, +} + +impl Cleanup { + /// Add a hook. Hooks run in reverse order of registration. + pub(crate) fn push(&self, name: impl Into, hook: F) + where + F: FnOnce() -> Fut + Send + 'static, + Fut: Future + Send + 'static, + { + lock(&self.hooks).push(Hook::new(name.into(), None, hook)); + } + + /// Add a hook unless one with `key` is registered, in which case the + /// existing hook keeps its place in the order. + pub(crate) fn push_keyed(&self, key: &str, name: impl Into, hook: F) + where + F: FnOnce() -> Fut + Send + 'static, + Fut: Future + Send + 'static, + { + let mut hooks = lock(&self.hooks); + if !hooks.iter().any(|hook| hook.key.as_deref() == Some(key)) { + hooks.push(Hook::new(name.into(), Some(key.to_string()), hook)); + } + } + + /// Drop the hook registered with `key`, if any, without running it. + pub(crate) fn remove(&self, key: &str) { + lock(&self.hooks).retain(|hook| hook.key.as_deref() != Some(key)); + } + + /// Run every hook, last registered first, each limited to `timeout`. A + /// hook that fails, panics, or times out doesn't stop the others. Returns + /// one message per failed hook. + pub(crate) async fn run(&self, timeout: Duration) -> Vec { + let mut failures = Vec::new(); + // Pop one at a time rather than draining, so the lock isn't held + // while a hook runs. A `while let` would hold it through the body. + loop { + let Some(hook) = lock(&self.hooks).pop() else { + break; + }; + let outcome = match std::panic::catch_unwind(AssertUnwindSafe(hook.run)) { + Ok(future) => { + tokio::time::timeout(timeout, AssertUnwindSafe(future).catch_unwind()).await + } + Err(panic) => Ok(Err(panic)), + }; + let failure = match outcome { + Ok(Ok(Ok(()))) => continue, + Ok(Ok(Err(error))) => error.to_string(), + Ok(Err(panic)) => format!("panicked: {}", panic_message(panic.as_ref())), + Err(_) => format!("timed out after {timeout:?}"), + }; + failures.push(format!("{}: {failure}", hook.name)); + } + failures + } +} + +fn lock(hooks: &Mutex>) -> std::sync::MutexGuard<'_, Vec> { + hooks.lock().unwrap_or_else(PoisonError::into_inner) +} + +fn panic_message(panic: &(dyn Any + Send)) -> &str { + panic + .downcast_ref::<&str>() + .copied() + .or_else(|| panic.downcast_ref::().map(String::as_str)) + .unwrap_or("non-string panic payload") +} + +#[cfg(test)] +mod tests { + use std::sync::{Arc, Mutex}; + use std::time::Duration; + + use super::Cleanup; + use crate::error::Error; + + const TIMEOUT: Duration = Duration::from_secs(5); + + type Log = Arc>>; + + /// Register a hook, keyed if `key` is set, that records `name` in `log` + /// when it runs. + fn record(cleanup: &Cleanup, key: Option<&str>, log: &Log, name: &'static str) { + let log = Arc::clone(log); + let hook = async move || { + log.lock().unwrap().push(name); + Ok(()) + }; + match key { + Some(key) => cleanup.push_keyed(key, name, hook), + None => cleanup.push(name, hook), + } + } + + #[tokio::test] + async fn runs_hooks_last_registered_first() { + let log = Arc::new(Mutex::new(Vec::new())); + let cleanup = Cleanup::default(); + for name in ["first", "second", "third"] { + record(&cleanup, None, &log, name); + } + assert!(cleanup.run(TIMEOUT).await.is_empty()); + assert_eq!(*log.lock().unwrap(), ["third", "second", "first"]); + // Each hook runs once. + assert!(cleanup.run(TIMEOUT).await.is_empty()); + assert_eq!(log.lock().unwrap().len(), 3); + } + + #[tokio::test] + async fn keyed_hook_is_registered_once_and_can_be_removed() { + let log = Arc::new(Mutex::new(Vec::new())); + let cleanup = Cleanup::default(); + record(&cleanup, Some("a"), &log, "a"); + record(&cleanup, None, &log, "b"); + // Already registered: keeps its place before "b". + record(&cleanup, Some("a"), &log, "a again"); + record(&cleanup, Some("c"), &log, "c"); + cleanup.remove("c"); + assert!(cleanup.run(TIMEOUT).await.is_empty()); + assert_eq!(*log.lock().unwrap(), ["b", "a"]); + } + + #[tokio::test] + async fn a_running_hook_can_change_the_remaining_hooks() { + let log = Arc::new(Mutex::new(Vec::new())); + let cleanup = Cleanup::default(); + record(&cleanup, Some("removed"), &log, "removed"); + let handle = cleanup.clone(); + cleanup.push("remove", async move || { + handle.remove("removed"); + Ok(()) + }); + assert!(cleanup.run(TIMEOUT).await.is_empty()); + assert!(log.lock().unwrap().is_empty()); + } + + #[tokio::test] + async fn failing_panicking_and_slow_hooks_are_reported_and_the_rest_still_run() { + let log = Arc::new(Mutex::new(Vec::new())); + let cleanup = Cleanup::default(); + record(&cleanup, None, &log, "ok first"); + cleanup.push("slow", async || { + tokio::time::sleep(Duration::from_secs(60)).await; + Ok(()) + }); + cleanup.push("panics", async || -> crate::Result { panic!("hook panic") }); + cleanup.push( + "panics before its future", + || -> std::future::Ready { panic!("closure panic") }, + ); + cleanup.push("fails", async || Err(Error::msg("hook error"))); + record(&cleanup, None, &log, "ok last"); + + let failures = cleanup.run(Duration::from_millis(100)).await; + assert_eq!( + failures, + [ + "fails: hook error", + "panics before its future: panicked: closure panic", + "panics: panicked: hook panic", + "slow: timed out after 100ms", + ] + ); + assert_eq!(*log.lock().unwrap(), ["ok last", "ok first"]); + } +} diff --git a/tests/testkit/testkit/src/context.rs b/tests/testkit/testkit/src/context.rs index ccbb6edf79..3953923b88 100644 --- a/tests/testkit/testkit/src/context.rs +++ b/tests/testkit/testkit/src/context.rs @@ -3,8 +3,6 @@ //! Per-test context: the SDK client, the run ID, and the resources to clean up. -use std::collections::BTreeSet; -use std::sync::{Arc, Mutex, PoisonError}; use std::time::Duration; use openshell_sdk::raw::{AuthedGrpcClient, proto}; @@ -14,6 +12,7 @@ use openshell_sdk::{ }; use rand::Rng; +use crate::cleanup::{CLEANUP_TIMEOUT, Cleanup}; use crate::cli::{Cli, CliOutput}; use crate::error::{Error, Result, ResultExt}; use crate::poll::{Poll, poll_until}; @@ -24,19 +23,15 @@ pub const DELETE_TIMEOUT: Duration = Duration::from_mins(2); pub const EXEC_TIMEOUT: Duration = Duration::from_mins(2); /// Stopping includes stopping compute, which can take a while on some drivers. pub const STOP_TIMEOUT: Duration = Duration::from_mins(5); -const CLEANUP_TIMEOUT: Duration = Duration::from_mins(2); const STATUS_INTERVAL: Duration = Duration::from_secs(1); -/// Names of sandboxes created by a test and not yet confirmed deleted. -type Registry = Arc>>; - /// What a test uses to act on the system under test. pub struct Context { client: WorkspaceScopedClient, grpc: Grpc, cli: Cli, run_id: String, - sandboxes: Registry, + cleanup: Cleanup, } impl Context { @@ -49,7 +44,7 @@ impl Context { }, cli, run_id: generate_run_id(), - sandboxes: Registry::default(), + cleanup: Cleanup::default(), } } @@ -83,6 +78,12 @@ impl Context { self.cli.run_with_env(None, env, args).await } + /// The CLI that [`Context::cli`] runs, for code that can't borrow the + /// context, such as a cleanup hook. + pub fn cli_handle(&self) -> Cli { + self.cli.clone() + } + /// The ID unique to this test run, used in resource names. pub fn run_id(&self) -> &str { &self.run_id @@ -104,7 +105,7 @@ impl Context { client: self.client.clone(), grpc: self.grpc.clone(), policy: None, - sandboxes: Arc::clone(&self.sandboxes), + cleanup: self.cleanup.clone(), spec: SandboxSpec { name: Some(self.name(suffix)), ..SandboxSpec::default() @@ -115,8 +116,49 @@ impl Context { /// Track a sandbox the test is about to create through the CLI, so cleanup /// deletes it even if the test fails after the gateway accepted the create. /// Call this before the CLI command; use [`Context::adopt`] afterwards. + /// + /// Cleanup deletes the sandbox and waits until it is gone, at the point + /// in the cleanup order where it was first tracked. Tracking a sandbox + /// again, for example by adopting it, doesn't move it. pub fn track(&self, name: &str) { - lock(&self.sandboxes).insert(name.to_string()); + track_sandbox(&self.cleanup, &self.client, name); + } + + /// Run `hook` after the test body, whether the body passes, fails, or + /// panics. Use it to remove what testkit doesn't track, such as + /// providers, profiles, or images. `name` says what the hook does, for + /// reporting a failure. + /// + /// Hooks run one at a time, last registered first, after the body and + /// before the test reports. Sandboxes from [`Context::sandbox`] and + /// [`Context::track`] are cleaned up the same way, so a hook registered + /// before a sandbox is created runs after the sandbox is deleted. Each + /// hook has [`CLEANUP_TIMEOUT`]. A hook that fails, panics, or times out + /// is reported and the remaining hooks still run; it fails a test whose + /// body passed. + /// + /// `provider delete` succeeds for a missing provider, so this registers + /// the hook before the create. It then also covers a create the gateway + /// applied but the CLI reported as failed. + /// + /// ```ignore + /// let cli = cx.cli_handle(); + /// let provider = cx.name("provider"); + /// let name = provider.clone(); + /// cx.on_cleanup(format!("delete provider {provider}"), async move || { + /// cli.run(["provider", "delete", &name]).await?.success()?; + /// Ok(()) + /// }); + /// cx.cli(["provider", "create", "--name", &provider /* ... */]) + /// .await? + /// .success()?; + /// ``` + pub fn on_cleanup(&self, name: impl Into, hook: F) + where + F: FnOnce() -> Fut + Send + 'static, + Fut: Future + Send + 'static, + { + self.cleanup.push(name, hook); } /// A handle to a sandbox created outside the builder, e.g. by the CLI. @@ -131,7 +173,7 @@ impl Context { Ok(Sandbox { client: self.client.clone(), grpc: self.grpc.clone(), - sandboxes: Arc::clone(&self.sandboxes), + cleanup: self.cleanup.clone(), name: name.to_string(), id: found.id, }) @@ -181,47 +223,35 @@ impl Context { Ok(()) } - /// Delete every sandbox the test created and has not deleted. Returns the - /// cleanup failures; they are reported separately from the test result. - /// - /// Sandboxes are deleted concurrently, each with the full - /// [`CLEANUP_TIMEOUT`], so a slow delete cannot use up the time for the - /// others and cleanup takes about one timeout however many there are. + /// Run the cleanup hooks, including deleting the sandboxes the test + /// created and has not deleted. Returns the cleanup failures; they are + /// reported separately from the test result. pub(crate) async fn cleanup(&self) -> Vec { - let names: Vec = lock(&self.sandboxes).iter().cloned().collect(); - // The timeout wraps the whole delete, not just the wait for it, so a - // gateway that never answers the delete cannot hang cleanup. - let results = futures::future::join_all(names.iter().map(|name| async move { - let delete = async { - self.client - .delete_sandbox( - name, - DeleteOptions { - allow_missing: true, - }, - ) - .await?; - self.client.wait_deleted(name, CLEANUP_TIMEOUT, None).await - }; - match tokio::time::timeout(CLEANUP_TIMEOUT, delete).await { - Ok(result) => result.map_err(|error| error.to_string()), - Err(_) => Err(format!("timed out after {}s", CLEANUP_TIMEOUT.as_secs())), - } - })) - .await; - let mut failures = Vec::new(); - for (name, result) in names.iter().zip(results) { - match result { - Ok(()) => { - lock(&self.sandboxes).remove(name); - } - Err(error) => failures.push(format!("delete sandbox {name}: {error}")), - } - } - failures + self.cleanup.run(CLEANUP_TIMEOUT).await } } +/// Register the cleanup hook that deletes sandbox `name`, unless it is +/// already tracked. +fn track_sandbox(cleanup: &Cleanup, client: &WorkspaceScopedClient, name: &str) { + let client = client.clone(); + let owned = name.to_string(); + // The timeout wraps the whole hook, not just the wait, so a gateway that + // never answers the delete cannot hang cleanup. + cleanup.push_keyed(name, format!("delete sandbox {name}"), async move || { + client + .delete_sandbox( + &owned, + DeleteOptions { + allow_missing: true, + }, + ) + .await?; + client.wait_deleted(&owned, CLEANUP_TIMEOUT, None).await?; + Ok(()) + }); +} + /// The generated gRPC client, for public API the SDK does not wrap yet. #[derive(Clone)] struct Grpc { @@ -244,7 +274,7 @@ pub struct SandboxBuilder { client: WorkspaceScopedClient, grpc: Grpc, policy: Option, - sandboxes: Registry, + cleanup: Cleanup, spec: SandboxSpec, } @@ -285,7 +315,7 @@ impl SandboxBuilder { /// cleaned up. pub async fn create(self) -> Result { let name = self.spec.name.clone().unwrap_or_default(); - lock(&self.sandboxes).insert(name.clone()); + track_sandbox(&self.cleanup, &self.client, &name); let id = match self.policy { None => { self.client @@ -334,7 +364,7 @@ impl SandboxBuilder { Ok(Sandbox { client: self.client, grpc: self.grpc, - sandboxes: self.sandboxes, + cleanup: self.cleanup, name, id, }) @@ -353,7 +383,7 @@ const fn restart_policy(policy: SandboxRestartPolicy) -> proto::SandboxRestartPo pub struct Sandbox { client: WorkspaceScopedClient, grpc: Grpc, - sandboxes: Registry, + cleanup: Cleanup, name: String, id: String, } @@ -542,13 +572,24 @@ impl Sandbox { .wait_deleted(&self.name, DELETE_TIMEOUT, Some(&self.id)) .await .context(format!("wait for sandbox {} to be deleted", self.name))?; - lock(&self.sandboxes).remove(&self.name); + self.cleanup.remove(&self.name); Ok(()) } } -fn lock(registry: &Registry) -> std::sync::MutexGuard<'_, BTreeSet> { - registry.lock().unwrap_or_else(PoisonError::into_inner) +#[cfg(test)] +impl Context { + /// A context for a gateway that isn't running, so gateway calls fail. + pub(crate) fn unconnected() -> Self { + const ENDPOINT: &str = "http://127.0.0.1:1"; + let channel = tonic::transport::Channel::from_static(ENDPOINT).connect_lazy(); + let interceptor = + openshell_sdk::EdgeAuthInterceptor::new(None, None).expect("no-op interceptor"); + let client = OpenShellClient::from_parts(channel, interceptor); + let gateway = + crate::gateway::Gateway::from_endpoint(ENDPOINT.to_string(), "default".to_string()); + Self::new(&client, "default", Cli::from_env(&gateway)) + } } /// Short, lowercase, and DNS-label safe, so names work on every driver. diff --git a/tests/testkit/testkit/src/gateway.rs b/tests/testkit/testkit/src/gateway.rs index ef40cb1bf9..a3c27341a2 100644 --- a/tests/testkit/testkit/src/gateway.rs +++ b/tests/testkit/testkit/src/gateway.rs @@ -85,7 +85,7 @@ impl Gateway { ) } - fn from_endpoint(endpoint: String, workspace: String) -> Self { + pub(crate) fn from_endpoint(endpoint: String, workspace: String) -> Self { Self { name: None, endpoint, diff --git a/tests/testkit/testkit/src/lib.rs b/tests/testkit/testkit/src/lib.rs index 168fdbe894..a78164ae8e 100644 --- a/tests/testkit/testkit/src/lib.rs +++ b/tests/testkit/testkit/src/lib.rs @@ -22,6 +22,7 @@ //! Tests find the gateway the same way the `openshell` CLI does (see //! [`gateway`]). +mod cleanup; mod cli; mod context; mod error; @@ -29,6 +30,7 @@ pub mod gateway; mod poll; mod run; +pub use cleanup::CLEANUP_TIMEOUT; pub use cli::{BIN_ENV, CLI_TIMEOUT, Cli, CliOutput}; pub use context::{ Context, DELETE_TIMEOUT, EXEC_TIMEOUT, ExecOutput, READY_TIMEOUT, STOP_TIMEOUT, Sandbox, diff --git a/tests/testkit/testkit/src/run.rs b/tests/testkit/testkit/src/run.rs index 731ead0557..701fc6e6ec 100644 --- a/tests/testkit/testkit/src/run.rs +++ b/tests/testkit/testkit/src/run.rs @@ -44,7 +44,16 @@ where let mut cx = Context::new(&client, &gateway.workspace, Cli::from_env(&gateway)); eprintln!("[{name}] run id {}", cx.run_id()); - let outcome = AssertUnwindSafe(body(&mut cx)).catch_unwind().await; + run_in(name, &mut cx, body).await; +} + +/// Run `body` in `cx`, then its cleanup hooks, and fail the test if either +/// failed. +async fn run_in(name: &str, cx: &mut Context, body: F) +where + F: AsyncFnOnce(&mut Context) -> Result, +{ + let outcome = AssertUnwindSafe(body(cx)).catch_unwind().await; let cleanup_failures = cx.cleanup().await; for failure in &cleanup_failures { eprintln!("[{name}] cleanup failed: {failure}"); @@ -60,3 +69,127 @@ where ), } } + +#[cfg(test)] +mod tests { + use std::panic::AssertUnwindSafe; + use std::sync::Arc; + use std::sync::atomic::{AtomicBool, Ordering}; + + use futures::FutureExt; + + use super::run_in; + use crate::context::Context; + use crate::error::{Error, Result}; + + /// Run `body` the way `#[testkit::test]` does, returning the test's panic + /// message if it failed. + async fn outcome(body: F) -> Option + where + F: AsyncFnOnce(&mut Context) -> Result, + { + let mut cx = Context::unconnected(); + let result = AssertUnwindSafe(run_in("test", &mut cx, body)) + .catch_unwind() + .await; + let panic = result.err()?; + Some( + panic + .downcast_ref::() + .cloned() + .or_else(|| panic.downcast_ref::<&str>().map(ToString::to_string)) + .unwrap_or_default(), + ) + } + + /// A cleanup hook that sets the returned marker when it runs. + fn mark_on_cleanup(cx: &Context) -> Arc { + let marker = Arc::new(AtomicBool::new(false)); + let set = Arc::clone(&marker); + cx.on_cleanup("set marker", async move || { + set.store(true, Ordering::SeqCst); + Ok(()) + }); + marker + } + + #[tokio::test] + async fn passing_test_runs_cleanup_and_passes() { + let mut marker = None; + let failure = outcome(async |cx: &mut Context| { + marker = Some(mark_on_cleanup(cx)); + Ok(()) + }) + .await; + assert_eq!(failure, None); + assert!(marker.unwrap().load(Ordering::SeqCst)); + } + + #[tokio::test] + async fn panicking_body_still_runs_cleanup_and_the_test_fails_with_its_panic() { + let mut marker = None; + let failure = outcome(async |cx: &mut Context| -> Result { + marker = Some(mark_on_cleanup(cx)); + panic!("body panic"); + }) + .await; + assert_eq!(failure.as_deref(), Some("body panic")); + assert!(marker.unwrap().load(Ordering::SeqCst)); + } + + #[tokio::test] + async fn failing_body_still_runs_cleanup_and_the_test_fails_with_its_error() { + let mut marker = None; + let failure = outcome(async |cx: &mut Context| { + marker = Some(mark_on_cleanup(cx)); + Err(Error::msg("body error")) + }) + .await; + assert_eq!(failure.as_deref(), Some("[test] body error")); + assert!(marker.unwrap().load(Ordering::SeqCst)); + } + + #[tokio::test] + async fn failing_cleanup_fails_a_passing_test() { + let failure = outcome(async |cx: &mut Context| { + cx.on_cleanup("fails", async || Err(Error::msg("hook error"))); + Ok(()) + }) + .await; + assert_eq!( + failure.as_deref(), + Some("[test] passed, but cleanup failed: fails: hook error") + ); + } + + #[tokio::test] + async fn failing_cleanup_does_not_replace_the_body_error() { + let failure = outcome(async |cx: &mut Context| { + cx.on_cleanup("fails", async || Err(Error::msg("hook error"))); + Err(Error::msg("body error")) + }) + .await; + assert_eq!(failure.as_deref(), Some("[test] body error")); + } + + #[tokio::test] + async fn sandbox_tracked_twice_is_deleted_once() { + // The gateway isn't running, so the delete fails and is reported. + let failure = outcome(async |cx: &mut Context| { + cx.track("leaked"); + cx.track("leaked"); + Ok(()) + }) + .await + .unwrap(); + assert!( + failure.starts_with("[test] passed, but cleanup failed: delete sandbox leaked: "), + "{failure}" + ); + assert_eq!( + failure.matches("delete sandbox leaked").count(), + 1, + "{failure}" + ); + } +}