string.ContainAllOf and string.ContainAnyOf by beeleebow · Pull Request #633 · fluentassertions/fluentassertions

beeleebow · 2017-09-02T12:05:03Z

One thing I always find myself doing is asserting the contents of a string multiple times. Usually I am wanting to assert that something like a log message contains some important information in no particular order (so as to not have an overly rigid test). For example:

logMsg.Should().Contain(customerNumber);
logMsg.Should().Contain("success");
logMsg.Should().Contain(timestamp);

I thought it might be nice if I could write this instead:

logMsg.Should().ContainAllOf(new []{ customerNumber, "success", timestamp"});

I also thought a similar ContainAnyOf could be useful, so I added it while I was there.

I updated the CONTRIBUTING.MD document as well, adding the disclaimer about the v 5 rework and suggesting PRs be targeted at develop (it said master before).

jnyrup · 2017-09-02T13:23:50Z

Nice additions!

I would rename ContainAllOf to be an overload of Contain.
And maybe rename ContainsAnyOf to ContainAny?
By having a glimpse on the other string assertions, that would make the naming more consistent.

To make it complete you could add the EquivalentOf and Not versions.

jnyrup · 2017-09-02T13:36:04Z

If staying with the name ContainAll‘ we could have the convenient ContainAll(params string[] values)` overload.

On a side note:
Your example could be shortened by utilizing .And:
logMsg.Should().Contain().And.Contain();

dennisdoomen · 2017-09-02T15:46:40Z

Ah, now I understand what you're trying to accomplish here. Did you know you can also do logMsg.Should().Match($"*{customerNumber}*success*{timestamp}*")

SaroTasciyan · 2017-09-02T17:57:10Z

I think ContainAllOf() and ContainAnyOf() adds value since it's functionally different than Match() (order independent, works with shared sub-strings).
I'd go for params instead of IEnumerable<string> tho.

beeleebow · 2017-09-03T00:43:06Z

Okay, great. Thanks for the feedback guys.

With regards to the params overloads, that was what I had in mind initially, but then saw the common params object[] becauseArgs pattern that was in place.

But we could have an extra overload that takes only a params string[], which probably satisfies most use cases. You just can't use a "because" with that overload.

With that said, here is what I am thinking:

Containment

ContainAll(IEnumerable<string> values, string because, params object[] becauseArgs) and ContainAll(params string[] values)
ContainAny(IEnumerable<string> values, string because, params object[] becauseArgs) and ContainAny(params string[] values)
NotContainAny(IEnumerable<string> values, string because, params object[] becauseArgs) and NotContainAny(params string[] values)

We could also add NotContainAll, but I'm not sure how valuable that is and the presence of both "not" and "all" could cause confusion about the semantics. Happy to add it if you guys think it's worthwhile.

Equivalency
From what I understand the "equivalent" methods for strings ignore whitespace and casing when doing comparisons. As @jnyrup suggests, we could add the following for consistency:

ContainEquivalentOfAll(IEnumerable<string> values, string because, params object[] becauseArgs) and ContainEquivalentOfAll(params string[] values)
ContainEquivalentOfAny(IEnumerable<string> values, string because, params object[] becauseArgs) and ContainEquivalentOfAny(params string[] values)
NotContainEquivalentOfAny(IEnumerable<string> values, string because, params object[] becauseArgs) and NotContainEquivalentOfAny(params string[] values)

There are similar arguments to be made about NotContainEquivalentOfAll.

Matching
Taking the consistency argument to it's exteme conclusion we could also add:

MatchAll(IEnumerable<string> patterns, string because, params object[] becauseArgs) and MatchAll(params string[] patterns)
MatchAny(IEnumerable<string> patterns, string because, params object[] becauseArgs) and MatchAny(params string[] patterns)
NotMatchAny(IEnumerable<string> patterns, string because, params object[] becauseArgs) and NotMatchAny(params string[] patterns)

(There is also the MatchEquivalentOf family of assertions...)

So that all said, it seems there is a balancing act to be struck between consistency and size/noise of the API. You guys are probably much better judges than me of where that line is so I'm happy to take your direction here.

As @jnyrup suggests the approach of chaining assertions via And is already available and is perhaps more in the spirit of the fluent style. So if we want to back out of this completely, then that is fine too :)

dennisdoomen · 2017-09-03T08:06:01Z

You'll end up with lots of extension methods. I see that as well, but on the other hand, they do add value.

… Fix typos (change -> chance, successfull -> successful)

jnyrup · 2017-09-03T12:31:52Z

NotContainAll can fill out a gap between ContainAny and ContainAll

logMsg.Should().NotContainAll(new[] {"A", "B"}, "Logging message should contain A or B but not both.").

But that is only if it works as "should contain any but not all".

Strictly speaking logMsg is allowed to not contain any of the provided values and still succeed.
Or put in code, the following would is valid according to set theory.

"C D E".Should().NotContainAll("A", "B")

I think the first case do make sense in a testing framework, while I share the concern that second can be confusing whether or not logMsg should contain any of the provided values.

This is similar to the case for

var list = new List<int>();

list.Should().OnlyContain(e => e == 0)

which fails on empty collections.
Even though an empty collection strictly speaking only has elements equal to zero.

dennisdoomen

Great work. I do have some (minor) remarks. Hopefully you are willing to address them.

dennisdoomen · 2017-09-05T05:32:59Z

Src/FluentAssertions/Primitives/StringAssertions.cs

+            var missing = values.Where(v => !Contains(Subject, v, StringComparison.Ordinal)).ToArray();
+            Execute.Assertion
+                .ForCondition(values.All(v => Contains(Subject, v, StringComparison.Ordinal)))
+                .BecauseOf(because, becauseArgs)


Use the phrase Expected {context:string} {0} to contain..... This allows wrapping this call in an AssertionScope (and an upcoming feature that will try to get the [CallerMemberName])

dennisdoomen · 2017-09-05T05:34:56Z

Tests/Shared.Specs/StringAssertionSpecs.cs

+            //-----------------------------------------------------------------------------------------------------------
+            act
+                .ShouldThrow<ArgumentException>()
+                .WithMessage("Cannot assert string containment of values in null collection");


I prefer to use wildcards in WithMessage so that only the relevant parts are verified. Also see point 10 from http://www.continuousimprover.com/2015/11/12-tips-to-write-unit-tests-that-dont.html

dennisdoomen · 2017-09-05T05:35:12Z

Tests/Shared.Specs/StringAssertionSpecs.cs

+            //-----------------------------------------------------------------------------------------------------------
+            // Assert
+            //-----------------------------------------------------------------------------------------------------------
+            act


Please verify only a single condition per test case.

dennisdoomen · 2017-09-05T05:35:52Z

Tests/Shared.Specs/StringAssertionSpecs.cs

+        #region ContainAll
+
+        [Fact]
+        public void When_null_collection_containall_should_throw_argument_exception()


Please don't repeat the name of a method or member in the name of the test case. See point 11 of http://www.continuousimprover.com/2015/11/12-tips-to-write-unit-tests-that-dont.html

dennisdoomen · 2017-09-05T05:36:35Z

Tests/Shared.Specs/StringAssertionSpecs.cs

+            //-----------------------------------------------------------------------------------------------------------
+            // Assert
+            //-----------------------------------------------------------------------------------------------------------
+            const string expectedErrorMsg = "Expected string \"red green yellow\" to contain strings: " +


Why don't you use interpolation strings?

No problems. Happy to make the changes. Do you prefer the changes be added in new commits or rolled into the original commits (i.e. rewrite git history)? Also, what C# language level is acceptable? 7? (I played it cautious with string.Format etc. because I wasn't sure what your build process etc. was)

I prefer an amend. And yes, C# 7 is perfectly fine.

beeleebow · 2017-09-06T02:15:58Z

@dennisdoomen have performed the requested changes. Let me know if I have missed anything. Note that this PR contains only ContainAll, ContainAny, NotContainAll, and NotContainAny. I may revisit the "equivalent" and "match" versions at a later date, in their own PRs.

Update CONTRIBUTING.md to say PRs should target develop (not master).…

a62c4f3

… Fix typos (change -> chance, successfull -> successful)

dennisdoomen requested changes Sep 5, 2017

View reviewed changes

Brad Bow added 3 commits September 6, 2017 11:23

Add NotContainAll string assertion

f7b0b91

Add NotContainAny string assertion

a5c301d

Add ContainAny and ContainAll string assertions

dc7391e

dennisdoomen approved these changes Sep 6, 2017

View reviewed changes

dennisdoomen merged commit 1decf46 into fluentassertions:develop Sep 6, 2017

Overlong mentioned this pull request Feb 7, 2021

SelfReferencingCollectionAssertions.OnlyContain fails when collection is empty #1492

Closed

Conversation

beeleebow commented Sep 2, 2017

Uh oh!

jnyrup commented Sep 2, 2017

Uh oh!

jnyrup commented Sep 2, 2017

Uh oh!

dennisdoomen commented Sep 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SaroTasciyan commented Sep 2, 2017

Uh oh!

beeleebow commented Sep 3, 2017

Uh oh!

dennisdoomen commented Sep 3, 2017

Uh oh!

jnyrup commented Sep 3, 2017

Uh oh!

dennisdoomen left a comment

Choose a reason for hiding this comment

Uh oh!

dennisdoomen Sep 5, 2017

Choose a reason for hiding this comment

Uh oh!

dennisdoomen Sep 5, 2017

Choose a reason for hiding this comment

Uh oh!

dennisdoomen Sep 5, 2017

Choose a reason for hiding this comment

Uh oh!

dennisdoomen Sep 5, 2017

Choose a reason for hiding this comment

Uh oh!

dennisdoomen Sep 5, 2017

Choose a reason for hiding this comment

Uh oh!

beeleebow Sep 5, 2017

Choose a reason for hiding this comment

Uh oh!

dennisdoomen Sep 5, 2017

Choose a reason for hiding this comment

Uh oh!

beeleebow commented Sep 6, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

dennisdoomen commented Sep 2, 2017 •

edited

Loading