From 7042ff0d4b057690d4207fcaaa2aa25666ebad8d Mon Sep 17 00:00:00 2001 From: Bradford Hovinen Date: Fri, 7 Jul 2023 17:38:51 +0200 Subject: [PATCH 1/2] Ensure that the assertion failure message of a fatal test failure is output even if there are preceding non-fatal errors in the test. There was a bug in which, if a test had a mix of fatal and non-fatal assertions, and a non-fatal assertion failed, then the test assertion failure message of a subsequent fatal assertion failure would not be output. This did not affect the overall test result but would be confusing and misleading by not including all failure messages. This change fixes the bug and introduces an integration test to protect against it. --- googletest/src/internal/test_outcome.rs | 16 +++++------ integration_tests/Cargo.toml | 5 ++++ .../src/fatal_and_non_fatal_failure.rs | 28 +++++++++++++++++++ integration_tests/src/integration_tests.rs | 25 +++++++++++++++-- run_integration_tests.sh | 1 + 5 files changed, 65 insertions(+), 10 deletions(-) create mode 100644 integration_tests/src/fatal_and_non_fatal_failure.rs diff --git a/googletest/src/internal/test_outcome.rs b/googletest/src/internal/test_outcome.rs index f21f2f8f..da526763 100644 --- a/googletest/src/internal/test_outcome.rs +++ b/googletest/src/internal/test_outcome.rs @@ -62,23 +62,23 @@ impl TestOutcome { /// /// **For internal use only. API stablility is not guaranteed!** #[doc(hidden)] - pub fn close_current_test_outcome(result: Result<(), E>) -> Result<(), ()> { + pub fn close_current_test_outcome(inner_result: Result<(), E>) -> Result<(), ()> { TestOutcome::with_current_test_outcome(|mut outcome| { - let result = match &*outcome { - Some(TestOutcome::Success) => match result { + let outer_result = match &*outcome { + Some(TestOutcome::Success) => match inner_result { Ok(()) => Ok(()), - Err(f) => { - print!("{}", f); - Err(()) - } + Err(_) => Err(()), }, Some(TestOutcome::Failure) => Err(()), None => { panic!("No test context found. This indicates a bug in GoogleTest.") } }; + if let Err(fatal_assertion_failure) = inner_result { + println!("{fatal_assertion_failure}"); + } *outcome = None; - result + outer_result }) } diff --git a/integration_tests/Cargo.toml b/integration_tests/Cargo.toml index 5d67582c..12e141b6 100644 --- a/integration_tests/Cargo.toml +++ b/integration_tests/Cargo.toml @@ -94,6 +94,11 @@ name = "failure_due_to_returned_error" path = "src/failure_due_to_returned_error.rs" test = false +[[bin]] +name = "fatal_and_non_fatal_failure" +path = "src/fatal_and_non_fatal_failure.rs" +test = false + [[bin]] name = "first_failure_aborts" path = "src/first_failure_aborts.rs" diff --git a/integration_tests/src/fatal_and_non_fatal_failure.rs b/integration_tests/src/fatal_and_non_fatal_failure.rs new file mode 100644 index 00000000..ed95d9cd --- /dev/null +++ b/integration_tests/src/fatal_and_non_fatal_failure.rs @@ -0,0 +1,28 @@ +// Copyright 2022 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. + +fn main() {} + +#[cfg(test)] +mod tests { + use googletest::prelude::*; + + #[googletest::test] + fn fatal_and_non_fatal_failure() -> Result<()> { + let value = 2; + verify_that!(value, eq(3)).and_log_failure(); + verify_that!(value, eq(4))?; + Ok(()) + } +} diff --git a/integration_tests/src/integration_tests.rs b/integration_tests/src/integration_tests.rs index 513c4b47..2295b275 100644 --- a/integration_tests/src/integration_tests.rs +++ b/integration_tests/src/integration_tests.rs @@ -126,8 +126,8 @@ mod tests { } #[test] - fn should_output_second_failure_message_on_second_assertion_failure_with_expect_that() - -> Result<()> { + fn should_output_second_failure_message_on_second_assertion_failure_with_expect_that( + ) -> Result<()> { let output = run_external_process_in_tests_directory("two_expect_that_failures")?; verify_that!( @@ -220,6 +220,27 @@ mod tests { ) } + #[test] + fn should_log_fatal_and_non_fatal_errors_to_stdout() -> Result<()> { + let output = run_external_process_in_tests_directory("fatal_and_non_fatal_failure")?; + + verify_that!( + output, + all!( + contains_substring(indoc! {" + Expected: is equal to 3 + Actual: 2, + which isn't equal to 3 + "}), + contains_substring(indoc! {" + Expected: is equal to 4 + Actual: 2, + which isn't equal to 4 + "}) + ) + ) + } + #[test] fn should_abort_after_first_failure() -> Result<()> { let output = run_external_process_in_tests_directory("first_failure_aborts")?; diff --git a/run_integration_tests.sh b/run_integration_tests.sh index ceaa09af..0fddc556 100644 --- a/run_integration_tests.sh +++ b/run_integration_tests.sh @@ -35,6 +35,7 @@ INTEGRATION_TEST_BINARIES=( "failure_due_to_fail_macro_with_empty_message" "failure_due_to_fail_macro_with_format_arguments" "failure_due_to_returned_error" + "fatal_and_non_fatal_failure" "first_failure_aborts" "google_test_with_rstest" "non_fatal_failure_in_subroutine" From d4cdd2cc3930685b6417a1abe596edada5fdc03b Mon Sep 17 00:00:00 2001 From: Bradford Hovinen Date: Fri, 7 Jul 2023 18:00:21 +0200 Subject: [PATCH 2/2] Introduce `TestFailure` to be `Result::Err` for the generated outer function of `#[googletest]` tests. Previously the function generated by the `#[googletest]` macro would be `()`. This resulted in a Clippy warning that `()` did not implement `std::errror::Error`. It would also result in a slightly confusing test failure output: ``` ...test assertion failure output... Error: () ``` The new empty struct `TestFailure` implements `std::error::Error` and displays to a more helpful message referring to the test assertion failure output above. --- googletest/src/internal/test_outcome.rs | 31 ++++++++++++++++++++++--- googletest_macro/src/lib.rs | 2 +- 2 files changed, 29 insertions(+), 4 deletions(-) diff --git a/googletest/src/internal/test_outcome.rs b/googletest/src/internal/test_outcome.rs index da526763..1a661fc3 100644 --- a/googletest/src/internal/test_outcome.rs +++ b/googletest/src/internal/test_outcome.rs @@ -62,14 +62,16 @@ impl TestOutcome { /// /// **For internal use only. API stablility is not guaranteed!** #[doc(hidden)] - pub fn close_current_test_outcome(inner_result: Result<(), E>) -> Result<(), ()> { + pub fn close_current_test_outcome( + inner_result: 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(()), + Err(_) => Err(TestFailure), }, - Some(TestOutcome::Failure) => Err(()), + Some(TestOutcome::Failure) => Err(TestFailure), None => { panic!("No test context found. This indicates a bug in GoogleTest.") } @@ -114,6 +116,29 @@ No test context found. } } +/// A market struct indicating that a test has failed. +/// +/// This exists to implement the [Error][std::error::Error] trait. It displays +/// to a message indicating that the actual test assertion failure messages are +/// in the text above. +pub struct TestFailure; + +impl std::error::Error for TestFailure {} + +impl std::fmt::Debug for TestFailure { + fn fmt(&self, f: &mut Formatter<'_>) -> Result<(), Error> { + writeln!(f, "See failure output above")?; + Ok(()) + } +} + +impl std::fmt::Display for TestFailure { + fn fmt(&self, f: &mut Formatter<'_>) -> Result<(), Error> { + writeln!(f, "See failure output above")?; + Ok(()) + } +} + /// A report that a single test assertion failed. /// /// **For internal use only. API stablility is not guaranteed!** diff --git a/googletest_macro/src/lib.rs b/googletest_macro/src/lib.rs index a29d041b..4fa0514e 100644 --- a/googletest_macro/src/lib.rs +++ b/googletest_macro/src/lib.rs @@ -95,7 +95,7 @@ pub fn test( }; let function = quote! { #(#attrs)* - #sig -> std::result::Result<(), ()> { + #sig -> std::result::Result<(), googletest::internal::test_outcome::TestFailure> { #maybe_closure use googletest::internal::test_outcome::TestOutcome; TestOutcome::init_current_test_outcome();