From 992b88e85eb7e8ee951b3f8bd4c471284801637b Mon Sep 17 00:00:00 2001 From: Taylor Cramer Date: Tue, 23 Sep 2025 13:35:20 -0700 Subject: [PATCH] Add unstable support for adding failures in test-generated threads PiperOrigin-RevId: 810562543 --- .github/workflows/ci.yml | 21 +++- googletest/Cargo.toml | 7 ++ googletest/src/internal/test_outcome.rs | 113 +++++++++++++----- googletest/src/lib.rs | 1 + integration_tests/Cargo.toml | 11 +- .../add_failure_in_new_thread_fails_test.rs | 24 ++++ integration_tests/src/integration_tests.rs | 9 ++ 7 files changed, 151 insertions(+), 35 deletions(-) create mode 100644 integration_tests/src/add_failure_in_new_thread_fails_test.rs diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 4ec7167e..bb3dbfd6 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -37,7 +37,7 @@ jobs: - name: cargo clippy uses: actions-rs/clippy-check@b5b5f21f4797c02da247df37026fcd0a5024aa4d # v1.0.7 with: - args: --all-targets --all-features + args: --all-targets --features all_stable_features token: ${{ secrets.GITHUB_TOKEN }} test-latest-deps: @@ -55,14 +55,29 @@ jobs: - name: cargo update run: cargo update - name: cargo test --locked - run: cargo test --locked --all-features + run: cargo test --locked --features all_stable_features test: runs-on: ubuntu-latest name: test / ubuntu / ${{ matrix.toolchain }} strategy: matrix: - toolchain: [stable, nightly, beta] + toolchain: [stable, beta] + steps: + - uses: actions/checkout@08c6903cd8c0fde910a37f88322edcfb5dd907a8 # v5.0.0 + - name: Install ${{ matrix.toolchain }} + uses: dtolnay/rust-toolchain@e97e2d8cc328f1b50210efc529dca0028893a2d9 + with: + toolchain: ${{ matrix.toolchain }} + - name: cargo test --locked + run: cargo test --locked --features all_stable_features + + test-nightly: + runs-on: ubuntu-latest + name: test / ubuntu / ${{ matrix.toolchain }} + strategy: + matrix: + toolchain: nightly steps: - uses: actions/checkout@08c6903cd8c0fde910a37f88322edcfb5dd907a8 # v5.0.0 - name: Install ${{ matrix.toolchain }} diff --git a/googletest/Cargo.toml b/googletest/Cargo.toml index 7882340d..a57b49aa 100644 --- a/googletest/Cargo.toml +++ b/googletest/Cargo.toml @@ -41,3 +41,10 @@ rustversion = "1.0.22" [dev-dependencies] indoc = "2" quickcheck = "1.0.3" + +[features] +# Enables use of the nightly-only `thread_spawn_hook` for capturing test +# failures in spawned threads. +unstable_thread_spawn_hook = [] +# A group like `--all-features`, but excluding nightly-only features. +all_stable_features = ["anyhow", "proptest"] diff --git a/googletest/src/internal/test_outcome.rs b/googletest/src/internal/test_outcome.rs index 341eb72f..4204f7f5 100644 --- a/googletest/src/internal/test_outcome.rs +++ b/googletest/src/internal/test_outcome.rs @@ -14,6 +14,8 @@ use std::cell::{RefCell, RefMut}; use std::fmt::{Debug, Display, Error, Formatter}; +use std::sync::atomic::{AtomicBool, Ordering as AtomicOrdering}; +use std::sync::Arc; use std::thread_local; /// The outcome hitherto of running a test. @@ -23,16 +25,45 @@ use std::thread_local; /// /// **For internal use only. API stablility is not guaranteed!** #[doc(hidden)] -pub enum TestOutcome { - /// The test ran or is currently running and no assertions have failed. - Success, - /// The test ran or is currently running and at least one assertion has - /// failed. - Failure, +pub struct TestOutcome { + is_success: AtomicBool, +} + +impl Default for TestOutcome { + fn default() -> Self { + Self::new() + } +} + +impl TestOutcome { + pub fn new() -> Self { + Self { is_success: AtomicBool::new(true) } + } + pub fn fail(&self) { + self.is_success.store(false, AtomicOrdering::Relaxed) + } + #[must_use] + pub fn is_success(&self) -> bool { + self.is_success.load(AtomicOrdering::Relaxed) + } + #[must_use] + pub fn is_failed(&self) -> bool { + self.is_success.load(AtomicOrdering::Relaxed) + } } thread_local! { - static CURRENT_TEST_OUTCOME: RefCell> = const { RefCell::new(None) }; + // Whether or not the current test has failed. + // + // If inside a `#[gtest]` function, this value will initially be set to a new `TestOutcome`. + // Upon assertion failure (e.g. `expect_that!` failing), the `TestOutcome` will be updated to + // indicate failure. + // + // The `Arc` is used to share the `TestOutcome` across threads that have been spawned by the + // `#[gtest]` function, which can then set it to fail upon an assertion failure in a thread. + static CURRENT_TEST_OUTCOME: RefCell>> = const { RefCell::new(None) }; + #[cfg(feature = "unstable_thread_spawn_hook")] + static HAS_SET_SPAWN_HOOK: std::cell::Cell = const { std::cell::Cell::new(false) }; } impl TestOutcome { @@ -44,8 +75,26 @@ impl TestOutcome { #[doc(hidden)] pub fn init_current_test_outcome() { Self::with_current_test_outcome(|mut current_test_outcome| { - *current_test_outcome = Some(TestOutcome::Success); - }) + *current_test_outcome = Some(Arc::new(TestOutcome::new())); + }); + + #[cfg(feature = "unstable_thread_spawn_hook")] + if !HAS_SET_SPAWN_HOOK.get() { + // Ensure that the spawn hook is only set once so that we don't accumulate spawn + // hooks for threads that run multiple tests. + HAS_SET_SPAWN_HOOK.set(true); + std::thread::add_spawn_hook(|_thread| { + let outcome: Option> = + Self::with_current_test_outcome(|current_test_outcome| { + current_test_outcome.clone() + }); + move || { + Self::with_current_test_outcome(|mut current_test_outcome| { + *current_test_outcome = outcome; + }); + } + }) + } } /// Evaluates the current test's [`TestOutcome`], producing a suitable @@ -62,23 +111,21 @@ impl TestOutcome { /// **For internal use only. API stablility is not guaranteed!** #[doc(hidden)] pub fn close_current_test_outcome( - inner_result: Result<(), E>, + test_return_value: Result<(), E>, ) -> Result<(), TestFailure> { - TestOutcome::with_current_test_outcome(|mut outcome| { - let outer_result = match &*outcome { - Some(TestOutcome::Success) => match inner_result { - Ok(()) => Ok(()), - Err(_) => Err(TestFailure), - }, - Some(TestOutcome::Failure) => Err(TestFailure), - None => { - panic!("No test context found. This indicates a bug in GoogleTest.") - } + TestOutcome::with_current_test_outcome(|mut outcome_arc| { + let Some(outcome) = outcome_arc.as_ref() else { + panic!("No test context found. This indicates a bug in GoogleTest.") + }; + let outer_result = if outcome.is_success() && test_return_value.is_ok() { + Ok(()) + } else { + Err(TestFailure) }; - if let Err(fatal_assertion_failure) = inner_result { + if let Err(fatal_assertion_failure) = test_return_value { println!("{fatal_assertion_failure}"); } - *outcome = None; + *outcome_arc = None; outer_result }) } @@ -88,12 +135,14 @@ impl TestOutcome { #[track_caller] pub(crate) fn get_current_test_outcome() -> Result<(), TestAssertionFailure> { TestOutcome::with_current_test_outcome(|mut outcome| { - let outcome = outcome + let is_success = outcome .as_mut() - .expect("No test context found. This indicates a bug in GoogleTest."); - match outcome { - TestOutcome::Success => Ok(()), - TestOutcome::Failure => Err(TestAssertionFailure::create("Test failed".into())), + .expect("No test context found. This indicates a bug in GoogleTest.") + .is_success(); + if is_success { + Ok(()) + } else { + Err(TestAssertionFailure::create("Test failed".into())) } }) } @@ -101,10 +150,10 @@ impl TestOutcome { /// Records that the currently running test has failed. fn fail_current_test() { TestOutcome::with_current_test_outcome(|mut outcome| { - let outcome = outcome + outcome .as_mut() - .expect("No test context found. This indicates a bug in GoogleTest."); - *outcome = TestOutcome::Failure; + .expect("No test context found. This indicates a bug in GoogleTest.") + .fail(); }) } @@ -112,7 +161,9 @@ impl TestOutcome { /// /// This is primarily intended for use by assertion macros like /// `expect_that!`. - fn with_current_test_outcome(action: impl FnOnce(RefMut>) -> T) -> T { + fn with_current_test_outcome( + action: impl FnOnce(RefMut>>) -> T, + ) -> T { CURRENT_TEST_OUTCOME.with(|current_test_outcome| action(current_test_outcome.borrow_mut())) } diff --git a/googletest/src/lib.rs b/googletest/src/lib.rs index 15baec41..50babba2 100644 --- a/googletest/src/lib.rs +++ b/googletest/src/lib.rs @@ -12,6 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. +#![cfg_attr(feature = "unstable_thread_spawn_hook", feature(thread_spawn_hook))] #![doc = include_str!("../crate_docs.md")] extern crate googletest_macro; diff --git a/integration_tests/Cargo.toml b/integration_tests/Cargo.toml index 40ee57c2..047ec4e9 100644 --- a/integration_tests/Cargo.toml +++ b/integration_tests/Cargo.toml @@ -36,6 +36,9 @@ rustversion = "1.0.22" tempfile = "3.22.0" tokio = { version = "1.47", features = ["time", "macros", "rt"] } +[features] +unstable_thread_spawn_hook=["googletest/unstable_thread_spawn_hook"] + [[bin]] name = "integration_tests" path = "src/integration_tests.rs" @@ -131,6 +134,12 @@ name = "add_failure_macro_causes_failure_but_continues_execution" path = "src/add_failure_macro_causes_failure_but_continues_execution.rs" test = false +[[bin]] +name = "add_failure_in_new_thread_fails_test" +path = "src/add_failure_in_new_thread_fails_test.rs" +test=false +required-features=["unstable_thread_spawn_hook"] + [[bin]] name = "add_failure_macro_allows_empty_message" path = "src/add_failure_macro_allows_empty_message.rs" @@ -449,4 +458,4 @@ test = false [[bin]] name = "expect_pred_macro_on_assertion_failure_with_format_args" path = "src/expect_pred_macro_on_assertion_failure_with_format_args.rs" -test = false \ No newline at end of file +test = false diff --git a/integration_tests/src/add_failure_in_new_thread_fails_test.rs b/integration_tests/src/add_failure_in_new_thread_fails_test.rs new file mode 100644 index 00000000..d5e3de13 --- /dev/null +++ b/integration_tests/src/add_failure_in_new_thread_fails_test.rs @@ -0,0 +1,24 @@ +// Copyright 2025 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +use googletest::{add_failure_at, gtest}; + +#[gtest] +fn should_fail() { + std::thread::spawn(|| { + add_failure_at!("file.rs", 1, 1); + }) + .join() + .unwrap(); +} diff --git a/integration_tests/src/integration_tests.rs b/integration_tests/src/integration_tests.rs index 82132fa3..cc02df25 100644 --- a/integration_tests/src/integration_tests.rs +++ b/integration_tests/src/integration_tests.rs @@ -645,6 +645,15 @@ mod tests { verify_that!(output, contains_regex("Success message with argument: An argument")) } + #[cfg(feature = "unstable_thread_spawn_hook")] + #[gtest] + fn add_failure_in_new_thread_fails_test() -> Result<()> { + let output = + run_external_process_in_tests_directory("add_failure_in_new_thread_fails_test")?; + expect_that!(output, contains_regex("test should_fail ... FAILED")); + Ok(()) + } + #[gtest] fn add_failure_macro_causes_failure_but_continues_execution() -> Result<()> { let output = run_external_process_in_tests_directory(