Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add version of StringAssertions.Contain that allows specifying the number of times a substring should occur. #818

Closed
basbossink opened this issue Mar 30, 2018 · 14 comments · Fixed by #1145

Comments

@basbossink
Copy link

basbossink commented Mar 30, 2018

Currently StringAssertions has no easy way of asserting that a substring should occur a particular number of times within a string. Imagine the syntax could be something like

actual.Shoud().Contain("Fred", Twice, "Fred is important");

or

actual.Should().Contain("Fred").Twice("Fred is important");

or

actual.Should().Contain("Fred", 2, "Fred is important");
@jnyrup
Copy link
Member

jnyrup commented Mar 30, 2018

How about the Match assertion?

This asserts that actual has at least two distinct Freds in it.

actual.Should().Match("*Fred*Fred*", "Fred is important");

I emphasize distinct because I feel it could be difficult to get such an API right.
E.g. does aaa contain aa one or two times?

Would actual.Should().Contain(expectedSubstring, k) mean that actual should contain expectedSubstring exactly k times or at least k times?

Just for the record a very unreadable way to assert that actual contains exactly two distinct Freds.

actual.Should().MatchRegex("^(?:(?!Fred).)*Fred(?:(?!Fred).)*Fred(?:(?!Fred).)*$", "Fred is important");

@basbossink
Copy link
Author

Thanks for the explanation, in this case the Match assertion is a good alternative. The second suggestion is imho a little bit more readable. In my example I would expect Twice to mean exactly two times. I could also image something like:

actual.Should().Contain("Fred").AtLeast.Twice("Fred is important");
actual.Should().Contain("Fred").AtMost.Twice("Fred is important");
actual.Should().Contain("Fred").Exactly.Twice("Fred is important"); //which would be equivalent to:
actual.Should().Contain("Fred").Twice("Fred is important");

Given that my request can by implemented using the Match, I would treat this request as a light suggestion and handle it as you see fit. Thanks for your efforts.

@dennisdoomen
Copy link
Member

It could work, provided that Fred does indeed exists in that actual. The successive checks would just apply additional checks. But then we should also add those extensions to all related assertions.

@mkolumb
Copy link

mkolumb commented Nov 9, 2018

Hi,

I would like to do this.
But I think we should do it using one of the following api

actual.Should().ContainExactly("Fred", 2, "Fred is important");
actual.Should().ContainExactly("Fred").Twice("Fred is important");
actual.Should().ContainExactly("Fred").Times(2, "Fred is important");

API proposed by @basbossink is more readable, but I think it will be a breaking change.

@mkolumb
Copy link

mkolumb commented Nov 9, 2018

Of course I can implement other similar methods like e.g. ContainAtLeast if proposed API will be accepted.

@dennisdoomen
Copy link
Member

I personally prefer the proposal of @basbossink

@mkolumb
Copy link

mkolumb commented Nov 9, 2018

Ok, I agree.

Look at the following API
actual.Should().Contain("Fred").AtMost.Twice("Fred is important");

If we have a string that does not contain "Fred" then assert should not throw.
But if we keep the current API
actual.Should().Contain("Fred")

Then it will throw an exception.
So we need an equivalent for
actual.Should().Contain("Fred")

How it should looks like?
Maybe actual.Should().Contain("Fred").Any?

@jnyrup
Copy link
Member

jnyrup commented Nov 9, 2018

We could do something similar to FakeItEasy's assertion syntax:

actual.Should().Contain("Fred", 4, Times.Exactly);
actual.Should().Contain("Fred", 4, Times.OrLess);
actual.Should().Contain("Fred", 4, Times.OrMore);

I'm still interested in the discussion about whether "aaa" contains "aa" once or twice.

@mkolumb
Copy link

mkolumb commented Nov 9, 2018

Good point.
I think that "aaa" contains "aa" only once.
Imagine you have 3 apples.
If you want to give someone 2 apples, you can do it only once.

@mkolumb
Copy link

mkolumb commented Nov 9, 2018

I have one more proposal

actual.Should().Contain("Fred", AtLeast.Twice, "Fred is important");
actual.Should().Contain("Fred", AtLeast.Times(2), "Fred is important");
actual.Should().Contain("Fred", MoreThan.Twice, "Fred is important");
actual.Should().Contain("Fred", MoreThan.Times(2), "Fred is important");
actual.Should().Contain("Fred", Exactly.Twice, "Fred is important");
actual.Should().Contain("Fred", Exactly.Times(2), "Fred is important");

There will be a several predefined options (once, twice, etc.) and the Times method to specify a number.

It also solves the problem with breaking changes.

@dennisdoomen
Copy link
Member

I still prefer the more fluent proposal of @basbossink

@mkolumb
Copy link

mkolumb commented Nov 9, 2018

@dennisdoomen I understand.
If I should implement this version, the problem of current Contain method still exists.
How I should solve this?
And the second issue is mentioned by @jnyrup

whether "aaa" contains "aa" once or twice

@mkolumb
Copy link

mkolumb commented Nov 9, 2018

In case of Contain method problem I see 2 solutions:

  1. Assumption that Contain should throw always if expected string wouldn't be found (leave it as is)
  2. Replace the current Contain by something like that actual.Should().Contain("Fred").Any

Personally, I think that FluentAssertions should be careful when introducing breaking changes.
So my suggestion is to implement 1 version.

I have almost completed the feature, but I would like to adjust the API to final version before sending PR.

@mkolumb
Copy link

mkolumb commented Nov 12, 2018

Hi guys, I created the pull request #962

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants