Merge pull request #358 from google:all_with_conjunction

copybara-github · copybara-github · commit c7d88d10a870 · 2024-02-02T09:20:12.000-08:00
PiperOrigin-RevId: 603705344
diff --git a/googletest/src/description.rs b/googletest/src/description.rs
@@ -89,6 +89,7 @@ use crate::internal::description_renderer::{List, INDENTATION_SIZE};
 pub struct Description {
     elements: List,
     initial_indentation: usize,
+    is_conjunction: bool,
 }
 
 impl Description {
@@ -199,6 +200,19 @@ impl Description {
     pub fn is_empty(&self) -> bool {
         self.elements.is_empty()
     }
+
+    pub(crate) fn conjunction_description(self) -> Self {
+        Self { is_conjunction: true, ..self }
+    }
+
+    pub(crate) fn is_conjunction_description(&self) -> bool {
+        self.is_conjunction
+    }
+
+    pub(crate) fn push_in_last_nested(mut self, inner: Description) -> Self {
+        self.elements.push_at_end(inner.elements);
+        self
+    }
 }
 
 impl Display for Description {
diff --git a/googletest/src/internal/description_renderer.rs b/googletest/src/internal/description_renderer.rs
@@ -83,6 +83,17 @@ impl List {
         self.0.is_empty()
     }
 
+    /// Append a new [`List`] in the last element which must be a
+    /// [`Block::Nested`]. Panic if `self` is empty or the last element is
+    /// not [`Block::Nested`].
+    pub(crate) fn push_at_end(&mut self, list: List) {
+        if let Some(Block::Nested(self_list)) = self.0.last_mut() {
+            self_list.push_nested(list);
+        } else {
+            panic!("pushing elements at the end of {self:#?} which last element is not Nested")
+        }
+    }
+
     fn render_with_prefix(
         &self,
         f: &mut dyn Write,
diff --git a/googletest/src/matchers/all_matcher.rs b/googletest/src/matchers/all_matcher.rs
@@ -53,114 +53,34 @@
 #[macro_export]
 #[doc(hidden)]
 macro_rules! __all {
-    ($($matcher:expr),* $(,)?) => {{
-        use $crate::matchers::__internal_unstable_do_not_depend_on_these::AllMatcher;
-        AllMatcher::new([$(Box::new($matcher)),*])
+    ($(,)?) => {{
+        $crate::matchers::anything()
+    }} ;
+    ($matcher:expr $(,)?) => {{
+        $matcher
+    }};
+    ($head:expr, $head2:expr $(,)?) => {{
+        $crate::matchers::__internal_unstable_do_not_depend_on_these::ConjunctionMatcher::new($head, $head2)
+    }};
+    ($head:expr, $head2:expr, $($tail:expr),+ $(,)?) => {{
+        $crate::__all![
+            $crate::matchers::__internal_unstable_do_not_depend_on_these::ConjunctionMatcher::new($head, $head2),
+            $($tail),+
+        ]
     }}
 }
 
-/// Functionality needed by the [`all`] macro.
-///
-/// For internal use only. API stablility is not guaranteed!
-#[doc(hidden)]
-pub mod internal {
-    use crate::description::Description;
-    use crate::matcher::{Matcher, MatcherResult};
-    use crate::matchers::anything;
-    use std::fmt::Debug;
-
-    /// A matcher which matches an input value matched by all matchers in the
-    /// array `components`.
-    ///
-    /// For internal use only. API stablility is not guaranteed!
-    #[doc(hidden)]
-    pub struct AllMatcher<'a, T: Debug + ?Sized, const N: usize> {
-        components: [Box<dyn Matcher<ActualT = T> + 'a>; N],
-    }
-
-    impl<'a, T: Debug + ?Sized, const N: usize> AllMatcher<'a, T, N> {
-        /// Constructs an [`AllMatcher`] with the given component matchers.
-        ///
-        /// Intended for use only by the [`all`] macro.
-        pub fn new(components: [Box<dyn Matcher<ActualT = T> + 'a>; N]) -> Self {
-            Self { components }
-        }
-    }
-
-    impl<'a, T: Debug + ?Sized, const N: usize> Matcher for AllMatcher<'a, T, N> {
-        type ActualT = T;
-
-        fn matches(&self, actual: &Self::ActualT) -> MatcherResult {
-            for component in &self.components {
-                match component.matches(actual) {
-                    MatcherResult::NoMatch => {
-                        return MatcherResult::NoMatch;
-                    }
-                    MatcherResult::Match => {}
-                }
-            }
-            MatcherResult::Match
-        }
-
-        fn explain_match(&self, actual: &Self::ActualT) -> Description {
-            match N {
-                0 => anything::<T>().explain_match(actual),
-                1 => self.components[0].explain_match(actual),
-                _ => {
-                    let failures = self
-                        .components
-                        .iter()
-                        .filter(|component| component.matches(actual).is_no_match())
-                        .collect::<Vec<_>>();
-
-                    if failures.len() == 1 {
-                        failures[0].explain_match(actual)
-                    } else {
-                        Description::new()
-                            .collect(
-                                failures
-                                    .into_iter()
-                                    .map(|component| component.explain_match(actual)),
-                            )
-                            .bullet_list()
-                    }
-                }
-            }
-        }
-
-        fn describe(&self, matcher_result: MatcherResult) -> Description {
-            match N {
-                0 => anything::<T>().describe(matcher_result),
-                1 => self.components[0].describe(matcher_result),
-                _ => {
-                    let header = if matcher_result.into() {
-                        "has all the following properties:"
-                    } else {
-                        "has at least one of the following properties:"
-                    };
-                    Description::new().text(header).nested(
-                        Description::new()
-                            .bullet_list()
-                            .collect(self.components.iter().map(|m| m.describe(matcher_result))),
-                    )
-                }
-            }
-        }
-    }
-}
-
 #[cfg(test)]
 mod tests {
-    use super::internal;
     use crate::matcher::{Matcher, MatcherResult};
     use crate::prelude::*;
     use indoc::indoc;
 
     #[test]
     fn description_shows_more_than_one_matcher() -> Result<()> {
-        let first_matcher = starts_with("A");
+        let first_matcher: StrMatcher<String, _> = starts_with("A");
         let second_matcher = ends_with("string");
-        let matcher: internal::AllMatcher<String, 2> = all!(first_matcher, second_matcher);
+        let matcher = all!(first_matcher, second_matcher);
 
         verify_that!(
             matcher.describe(MatcherResult::Match),
@@ -175,8 +95,8 @@ mod tests {
 
     #[test]
     fn description_shows_one_matcher_directly() -> Result<()> {
-        let first_matcher = starts_with("A");
-        let matcher: internal::AllMatcher<String, 1> = all!(first_matcher);
+        let first_matcher: StrMatcher<String, _> = starts_with("A");
+        let matcher = all!(first_matcher);
 
         verify_that!(
             matcher.describe(MatcherResult::Match),
@@ -187,9 +107,9 @@ mod tests {
     #[test]
     fn mismatch_description_shows_which_matcher_failed_if_more_than_one_constituent() -> Result<()>
     {
-        let first_matcher = starts_with("Another");
+        let first_matcher: StrMatcher<str, _> = starts_with("Another");
         let second_matcher = ends_with("string");
-        let matcher: internal::AllMatcher<str, 2> = all!(first_matcher, second_matcher);
+        let matcher = all!(first_matcher, second_matcher);
 
         verify_that!(
             matcher.explain_match("A string"),
@@ -200,7 +120,7 @@ mod tests {
     #[test]
     fn mismatch_description_is_simple_when_only_one_consistuent() -> Result<()> {
         let first_matcher = starts_with("Another");
-        let matcher: internal::AllMatcher<str, 1> = all!(first_matcher);
+        let matcher = all!(first_matcher);
 
         verify_that!(
             matcher.explain_match("A string"),
diff --git a/googletest/src/matchers/conjunction_matcher.rs b/googletest/src/matchers/conjunction_matcher.rs
@@ -21,7 +21,23 @@ use crate::{
 };
 use std::fmt::Debug;
 
-/// Matcher created by [`Matcher::and`].
+/// Matcher created by [`Matcher::and`] and [`all!`].
+///
+/// Both [`Matcher::and`] and [`all!`] nest on m1. In other words,
+/// both `x.and(y).and(z)` and `all![x, y, z]` produce:
+/// ```ignore
+/// ConjunctionMatcher {
+///     m1: ConjunctionMatcher {
+///         m1: x,
+///         m2: y
+///     },
+///     m2: z
+/// }
+/// ```
+///
+/// This behavior must be respected
+/// to ensure that [`Matcher::explain_match`] and [`Matcher::describe`] produce
+/// useful descriptions.
 ///
 /// **For internal use only. API stablility is not guaranteed!**
 #[doc(hidden)]
@@ -31,7 +47,7 @@ pub struct ConjunctionMatcher<M1, M2> {
 }
 
 impl<M1, M2> ConjunctionMatcher<M1, M2> {
-    pub(crate) fn new(m1: M1, m2: M2) -> Self {
+    pub fn new(m1: M1, m2: M2) -> Self {
         Self { m1, m2 }
     }
 }
@@ -51,22 +67,42 @@ where
 
     fn explain_match(&self, actual: &M1::ActualT) -> Description {
         match (self.m1.matches(actual), self.m2.matches(actual)) {
-            (MatcherResult::Match, MatcherResult::Match) => Description::new()
-                .nested(self.m1.explain_match(actual))
-                .text("and")
-                .nested(self.m2.explain_match(actual)),
             (MatcherResult::NoMatch, MatcherResult::Match) => self.m1.explain_match(actual),
             (MatcherResult::Match, MatcherResult::NoMatch) => self.m2.explain_match(actual),
-            (MatcherResult::NoMatch, MatcherResult::NoMatch) => Description::new()
-                .nested(self.m1.explain_match(actual))
-                .text("and")
-                .nested(self.m2.explain_match(actual)),
+            (_, _) => {
+                let m1_description = self.m1.explain_match(actual);
+                if m1_description.is_conjunction_description() {
+                    m1_description.nested(self.m2.explain_match(actual))
+                } else {
+                    Description::new()
+                        .bullet_list()
+                        .collect([self.m1.explain_match(actual), self.m2.explain_match(actual)])
+                        .conjunction_description()
+                }
+            }
         }
     }
 
     fn describe(&self, matcher_result: MatcherResult) -> Description {
-        format!("{}, and {}", self.m1.describe(matcher_result), self.m2.describe(matcher_result))
-            .into()
+        let m1_description = self.m1.describe(matcher_result);
+        if m1_description.is_conjunction_description() {
+            m1_description.push_in_last_nested(self.m2.describe(matcher_result))
+        } else {
+            let header = if matcher_result.into() {
+                "has all the following properties:"
+            } else {
+                "has at least one of the following properties:"
+            };
+            Description::new()
+                .text(header)
+                .nested(
+                    Description::new().bullet_list().collect([
+                        self.m1.describe(matcher_result),
+                        self.m2.describe(matcher_result),
+                    ]),
+                )
+                .conjunction_description()
+        }
     }
 }
 
@@ -88,7 +124,9 @@ mod tests {
             err(displays_as(contains_substring(indoc!(
                 "
                 Value of: 1
-                Expected: is anything, and never matches
+                Expected: has all the following properties:
+                  * is anything
+                  * never matches
                 Actual: 1,
                   which is anything
                 "
@@ -104,7 +142,9 @@ mod tests {
             err(displays_as(contains_substring(indoc!(
                 "
                     Value of: 1
-                    Expected: never matches, and is anything
+                    Expected: has all the following properties:
+                      * never matches
+                      * is anything
                     Actual: 1,
                       which is anything
                 "
@@ -120,11 +160,37 @@ mod tests {
             err(displays_as(contains_substring(indoc!(
                 "
                 Value of: 1
-                Expected: never matches, and never matches
+                Expected: has all the following properties:
+                  * never matches
+                  * never matches
+                Actual: 1,
+                  * which is anything
+                  * which is anything
+                "
+            ))))
+        )
+    }
+
+    #[test]
+    fn and_long_chain_of_matchers() -> Result<()> {
+        let result = verify_that!(
+            1,
+            anything().and(not(anything())).and(anything()).and(not(anything())).and(anything())
+        );
+        verify_that!(
+            result,
+            err(displays_as(contains_substring(indoc!(
+                "
+                Value of: 1
+                Expected: has all the following properties:
+                  * is anything
+                  * never matches
+                  * is anything
+                  * never matches
+                  * is anything
                 Actual: 1,
-                    which is anything
-                  and
-                    which is anything
+                  * which is anything
+                  * which is anything
                 "
             ))))
         )
diff --git a/googletest/src/matchers/mod.rs b/googletest/src/matchers/mod.rs
@@ -105,7 +105,6 @@ pub use crate::{
 // should only be used through their respective macros.
 #[doc(hidden)]
 pub mod __internal_unstable_do_not_depend_on_these {
-    pub use super::all_matcher::internal::AllMatcher;
     pub use super::any_matcher::internal::AnyMatcher;
     pub use super::conjunction_matcher::ConjunctionMatcher;
     pub use super::disjunction_matcher::DisjunctionMatcher;
