Skip to content

Commit

Permalink
Remove the need for allocation from is_utf8_string.
Browse files Browse the repository at this point in the history
Previously, `IsEncodedStringMatcher` converted its actual value input to
a `Vec` and constructed a `String` out of that in order to match it.
This is because using a string slice caused problems with the borrow
checker as described in #323.

The fundamental problem is that the type against which the inner matcher
is matching is a reference whose lifetime must be named in the trait
bound:

```
impl<ActualT: AsRef<[u8]> + Debug, InnerMatcherT> Matcher
    for IsEncodedStringMatcher<ActualT, InnerMatcherT>
where
    InnerMatcherT: Matcher<ActualT = &'what str>,
                                       ^^^^ What goes here???
```

One option is just to remove the reference:

```
impl<ActualT: AsRef<[u8]> + Debug, InnerMatcherT> Matcher
    for IsEncodedStringMatcher<ActualT, InnerMatcherT>
where
    InnerMatcherT: Matcher<ActualT = str>,
```

This doesn't work when the inner matcher is `eq`, since one would then
be comparing an `str` and a string slice:

```
verify_that!("A string".as_bytes(), is_utf8_string(eq("A string"))) // Compile error: cannot compare str and &str
```

However, this problem does not occur with `StrMatcher`:

```
verify_that!("A string".as_bytes(), is_utf8_string(constains_substring("A string"))) // Passes
```

So this also introduces an easy way to obtain a `StrMatcher` to check
string equality:

```
verify_that!("A string".as_bytes(), is_utf8_string(eq_str("A string"))) // Passes
```

This is slightly inconvenient, since one must use a different matcher
to compare string equality in this case. But it is well-documented and
not too inconvenient to use. So it should be okay.

The introduction of `eq_str` also allows fixing a longstanding
limitation of the matcher `property!` -- that it could not match on
properties returning string slices. This PR therefore also updates the
documentation accordingly and adds an appropriate test.
  • Loading branch information
hovinen committed Mar 28, 2024
1 parent 1d6ec26 commit 799ff7c
Show file tree
Hide file tree
Showing 6 changed files with 107 additions and 27 deletions.
4 changes: 4 additions & 0 deletions googletest/crate_docs.md
Original file line number Diff line number Diff line change
Expand Up @@ -127,13 +127,15 @@ The following matchers are provided in GoogleTest Rust:
| [`ends_with`] | A string ending with the given suffix. |
| [`eq`] | A value equal to the argument, in the sense of the [`PartialEq`] trait. |
| [`eq_deref_of`] | A value equal to the dereferenced value of the argument. |
| [`eq_str`] | A string equal to the argument. |
| [`err`] | A [`Result`][std::result::Result] containing an `Err` variant the argument matches. |
| [`field!`] | A struct or enum with a given field whose value the argument matches. |
| [`ge`] | A [`PartialOrd`] value greater than or equal to the given value. |
| [`gt`] | A [`PartialOrd`] value strictly greater than the given value. |
| [`has_entry`] | A [`HashMap`] containing a given key whose value the argument matches. |
| [`is_contained_in!`] | A container each of whose elements is matched by some given matcher. |
| [`is_nan`] | A floating point number which is NaN. |
| [`is_utf8_string`] | A byte sequence representing a UTF-8 encoded string which the argument matches. |
| [`le`] | A [`PartialOrd`] value less than or equal to the given value. |
| [`len`] | A container whose number of elements the argument matches. |
| [`lt`] | A [`PartialOrd`] value strictly less than the given value. |
Expand Down Expand Up @@ -169,6 +171,7 @@ The following matchers are provided in GoogleTest Rust:
[`empty`]: matchers::empty
[`ends_with`]: matchers::ends_with
[`eq`]: matchers::eq
[`eq_str`]: matchers::eq_str
[`eq_deref_of`]: matchers::eq_deref_of
[`err`]: matchers::err
[`field!`]: matchers::field
Expand All @@ -177,6 +180,7 @@ The following matchers are provided in GoogleTest Rust:
[`has_entry`]: matchers::has_entry
[`is_contained_in!`]: matchers::is_contained_in
[`is_nan`]: matchers::is_nan
[`is_utf8_string`]: matchers::is_utf8_string
[`le`]: matchers::le
[`len`]: matchers::len
[`lt`]: matchers::lt
Expand Down
50 changes: 28 additions & 22 deletions googletest/src/matchers/is_encoded_string_matcher.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,18 +25,21 @@ use std::{fmt::Debug, marker::PhantomData};
///
/// The input may be a slice `&[u8]` or a `Vec` of bytes.
///
/// When asserting the equality of the actual value with a given string, use
/// [`eq_str`] rather than [`eq`].
///
/// ```
/// # use googletest::prelude::*;
/// # fn should_pass() -> Result<()> {
/// let bytes: &[u8] = "A string".as_bytes();
/// verify_that!(bytes, is_utf8_string(eq("A string")))?; // Passes
/// verify_that!(bytes, is_utf8_string(eq_str("A string")))?; // Passes
/// let bytes: Vec<u8> = "A string".as_bytes().to_vec();
/// verify_that!(bytes, is_utf8_string(eq("A string")))?; // Passes
/// verify_that!(bytes, is_utf8_string(eq_str("A string")))?; // Passes
/// # Ok(())
/// # }
/// # fn should_fail_1() -> Result<()> {
/// # let bytes: &[u8] = "A string".as_bytes();
/// verify_that!(bytes, is_utf8_string(eq("Another string")))?; // Fails (inner matcher does not match)
/// verify_that!(bytes, is_utf8_string(eq_str("Another string")))?; // Fails (inner matcher does not match)
/// # Ok(())
/// # }
/// # fn should_fail_2() -> Result<()> {
Expand All @@ -48,11 +51,14 @@ use std::{fmt::Debug, marker::PhantomData};
/// # should_fail_1().unwrap_err();
/// # should_fail_2().unwrap_err();
/// ```
pub fn is_utf8_string<'a, ActualT: AsRef<[u8]> + Debug + 'a, InnerMatcherT>(
///
/// [`eq`]: crate::matchers::eq
/// [`eq_str`]: crate::matchers::eq_str
pub fn is_utf8_string<ActualT: AsRef<[u8]> + Debug, InnerMatcherT>(
inner: InnerMatcherT,
) -> impl Matcher<ActualT = ActualT>
where
InnerMatcherT: Matcher<ActualT = String>,
InnerMatcherT: Matcher<ActualT = str>,
{
IsEncodedStringMatcher { inner, phantom: Default::default() }
}
Expand All @@ -62,15 +68,15 @@ struct IsEncodedStringMatcher<ActualT, InnerMatcherT> {
phantom: PhantomData<ActualT>,
}

impl<'a, ActualT: AsRef<[u8]> + Debug + 'a, InnerMatcherT> Matcher
impl<ActualT: AsRef<[u8]> + Debug, InnerMatcherT> Matcher
for IsEncodedStringMatcher<ActualT, InnerMatcherT>
where
InnerMatcherT: Matcher<ActualT = String>,
InnerMatcherT: Matcher<ActualT = str>,
{
type ActualT = ActualT;

fn matches(&self, actual: &Self::ActualT) -> MatcherResult {
String::from_utf8(actual.as_ref().to_vec())
std::str::from_utf8(actual.as_ref())
.map(|s| self.inner.matches(&s))

Check warning on line 80 in googletest/src/matchers/is_encoded_string_matcher.rs

View workflow job for this annotation

GitHub Actions / clippy

this expression creates a reference which is immediately dereferenced by the compiler

warning: this expression creates a reference which is immediately dereferenced by the compiler --> googletest/src/matchers/is_encoded_string_matcher.rs:80:41 | 80 | .map(|s| self.inner.matches(&s)) | ^^ help: change this to: `s` | = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_borrow = note: `#[warn(clippy::needless_borrow)]` on by default
.unwrap_or(MatcherResult::NoMatch)
}
Expand All @@ -91,7 +97,7 @@ where
}

fn explain_match(&self, actual: &Self::ActualT) -> Description {
match String::from_utf8(actual.as_ref().to_vec()) {
match std::str::from_utf8(actual.as_ref()) {
Ok(s) => {
format!("which is a UTF-8 encoded string {}", self.inner.explain_match(&s)).into()

Check warning on line 102 in googletest/src/matchers/is_encoded_string_matcher.rs

View workflow job for this annotation

GitHub Actions / clippy

this expression creates a reference which is immediately dereferenced by the compiler

warning: this expression creates a reference which is immediately dereferenced by the compiler --> googletest/src/matchers/is_encoded_string_matcher.rs:102:88 | 102 | format!("which is a UTF-8 encoded string {}", self.inner.explain_match(&s)).into() | ^^ help: change this to: `s` | = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_borrow
}
Expand All @@ -107,70 +113,70 @@ mod tests {

#[test]
fn matches_string_as_byte_slice() -> Result<()> {
verify_that!("A string".as_bytes(), is_utf8_string(eq("A string")))
verify_that!("A string".as_bytes(), is_utf8_string(eq_str("A string")))
}

#[test]
fn matches_string_as_byte_vec() -> Result<()> {
verify_that!("A string".as_bytes().to_vec(), is_utf8_string(eq("A string")))
verify_that!("A string".as_bytes().to_vec(), is_utf8_string(eq_str("A string")))
}

#[test]
fn matches_string_with_utf_8_encoded_sequences() -> Result<()> {
verify_that!("äöüÄÖÜ".as_bytes().to_vec(), is_utf8_string(eq("äöüÄÖÜ")))
verify_that!("äöüÄÖÜ".as_bytes().to_vec(), is_utf8_string(eq_str("äöüÄÖÜ")))
}

#[test]
fn does_not_match_non_equal_string() -> Result<()> {
verify_that!("äöüÄÖÜ".as_bytes().to_vec(), not(is_utf8_string(eq("A string"))))
verify_that!("äöüÄÖÜ".as_bytes().to_vec(), not(is_utf8_string(eq_str("A string"))))
}

#[test]
fn does_not_match_non_utf_8_encoded_byte_sequence() -> Result<()> {
verify_that!(&[192, 64, 255, 32], not(is_utf8_string(eq("A string"))))
verify_that!(&[192, 64, 255, 32], not(is_utf8_string(eq_str("A string"))))
}

#[test]
fn has_correct_description_in_matched_case() -> Result<()> {
let matcher = is_utf8_string::<&[u8], _>(eq("A string"));
let matcher = is_utf8_string::<&[u8], _>(eq_str("A string"));

verify_that!(
matcher.describe(MatcherResult::Match),
displays_as(eq("is a UTF-8 encoded string which is equal to \"A string\""))
displays_as(eq_str("is a UTF-8 encoded string which is equal to \"A string\""))
)
}

#[test]
fn has_correct_description_in_not_matched_case() -> Result<()> {
let matcher = is_utf8_string::<&[u8], _>(eq("A string"));
let matcher = is_utf8_string::<&[u8], _>(eq_str("A string"));

verify_that!(
matcher.describe(MatcherResult::NoMatch),
displays_as(eq("is not a UTF-8 encoded string which is equal to \"A string\""))
displays_as(eq_str("is not a UTF-8 encoded string which is equal to \"A string\""))
)
}

#[test]
fn has_correct_explanation_in_matched_case() -> Result<()> {
let explanation = is_utf8_string(eq("A string")).explain_match(&"A string".as_bytes());
let explanation = is_utf8_string(eq_str("A string")).explain_match(&"A string".as_bytes());

verify_that!(
explanation,
displays_as(eq("which is a UTF-8 encoded string which is equal to \"A string\""))
displays_as(eq_str("which is a UTF-8 encoded string which is equal to \"A string\""))
)
}

#[test]
fn has_correct_explanation_when_byte_array_is_not_utf8_encoded() -> Result<()> {
let explanation = is_utf8_string(eq("A string")).explain_match(&&[192, 128, 0, 64]);
let explanation = is_utf8_string(eq_str("A string")).explain_match(&&[192, 128, 0, 64]);

verify_that!(explanation, displays_as(starts_with("which is not a UTF-8 encoded string: ")))
}

#[test]
fn has_correct_explanation_when_inner_matcher_does_not_match() -> Result<()> {
let explanation =
is_utf8_string(eq("A string")).explain_match(&"Another string".as_bytes());
is_utf8_string(eq_str("A string")).explain_match(&"Another string".as_bytes());

verify_that!(
explanation,
Expand Down
2 changes: 1 addition & 1 deletion googletest/src/matchers/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ pub use points_to_matcher::points_to;
pub use predicate_matcher::{predicate, PredicateMatcher};
pub use some_matcher::some;
pub use str_matcher::{
contains_substring, ends_with, starts_with, StrMatcher, StrMatcherConfigurator,
contains_substring, ends_with, eq_str, starts_with, StrMatcher, StrMatcherConfigurator,
};
pub use subset_of_matcher::subset_of;
pub use superset_of_matcher::superset_of;
Expand Down
11 changes: 7 additions & 4 deletions googletest/src/matchers/property_matcher.rs
Original file line number Diff line number Diff line change
Expand Up @@ -78,10 +78,10 @@
/// # .unwrap();
/// ```
///
/// Unfortunately, this matcher does *not* work with methods returning string
/// slices:
/// When the property returns a string slice and you wish to assert its
/// equality, use [`eq_str`] instead of [`eq`]:
///
/// ```compile_fail
/// ```
/// # use googletest::prelude::*;
/// #[derive(Debug)]
/// pub struct MyStruct {
Expand All @@ -92,7 +92,7 @@
/// }
///
/// let value = MyStruct { a_string: "A string".into() };
/// verify_that!(value, property!(*MyStruct.get_a_string(), eq("A string"))) // Does not compile
/// verify_that!(value, property!(*MyStruct.get_a_string(), eq_str("A string")))
/// # .unwrap();
/// ```
///
Expand All @@ -101,6 +101,9 @@
/// rather than accessing a field.
///
/// The list of arguments may optionally have a trailing comma.
///
/// [`eq`]: crate::matchers::eq
/// [`eq_str`]: crate::matchers::eq_str
#[macro_export]
#[doc(hidden)]
macro_rules! __property {
Expand Down
52 changes: 52 additions & 0 deletions googletest/src/matchers/str_matcher.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,58 @@ use std::fmt::Debug;
use std::marker::PhantomData;
use std::ops::Deref;

/// Matches a string equal to the given string.
///
/// This has the same effect as [`eq`]. Unlike [`eq`], [`eq_str`] can be used
/// in cases where the actual type being matched is `str`. In paticular, one
/// can use [`eq_str`] together with:
///
/// * [`property!`] where the property returns a string slice, and
/// * [`is_utf8_string`].
///
/// ```
/// # use googletest::prelude::*;
/// # fn should_pass_1() -> Result<()> {
/// verify_that!("Some value", eq_str("Some value"))?; // Passes
/// verify_that!("Some value".to_string(), eq_str("Some value"))?; // Passes
/// # Ok(())
/// # }
/// # fn should_fail() -> Result<()> {
/// verify_that!("Another value", eq_str("Some value"))?; // Fails
/// # Ok(())
/// # }
/// # fn should_pass_2() -> Result<()> {
/// let value_as_bytes = "Some value".as_bytes();
/// verify_that!(value_as_bytes, is_utf8_string(eq_str("Some value")))?; // Passes
/// # Ok(())
/// # }
/// # should_pass_1().unwrap();
/// # should_fail().unwrap_err();
/// # should_pass_2().unwrap();
///
/// #[derive(Debug)]
/// pub struct MyStruct {
/// a_string: String,
/// }
/// impl MyStruct {
/// pub fn get_a_string(&self) -> &str { &self.a_string }
/// }
///
/// let value = MyStruct { a_string: "A string".into() };
/// verify_that!(value, property!(*MyStruct.get_a_string(), eq_str("A string"))) // Passes
/// # .unwrap();
/// ```
///
/// [`eq`]: crate::matchers::eq
/// [`is_utf8_string`]: crate::matchers::is_utf8_string
pub fn eq_str<A: ?Sized, T>(expected: T) -> StrMatcher<A, T> {
StrMatcher {
configuration: Configuration { mode: MatchMode::Equals, ..Default::default() },
expected,
phantom: Default::default(),
}
}

/// Matches a string containing a given substring.
///
/// Both the actual value and the expected substring may be either a `String` or
Expand Down
15 changes: 15 additions & 0 deletions googletest/tests/property_matcher_test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,21 @@ fn matches_struct_with_matching_string_reference_property() -> Result<()> {
verify_that!(value, property!(*StructWithString.get_property_ref(), eq("Something")))
}

#[test]
fn matches_struct_with_matching_string_slice_property() -> Result<()> {
#[derive(Debug)]
struct StructWithString {
property: String,
}
impl StructWithString {
fn get_property_ref(&self) -> &str {
&self.property
}
}
let value = StructWithString { property: "Something".into() };
verify_that!(value, property!(*StructWithString.get_property_ref(), eq_str("Something")))
}

#[test]
fn matches_struct_with_matching_slice_property() -> Result<()> {
#[derive(Debug)]
Expand Down

0 comments on commit 799ff7c

Please sign in to comment.