-
Notifications
You must be signed in to change notification settings - Fork 731
Add NotContainEquivalentOf #1318
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 NotContainEquivalentOf #1318
Conversation
jnyrup
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work 👍
Could I ask you to add a line to https://github.com/fluentassertions/fluentassertions/blob/develop/docs/_pages/releases.md as well?
Tests/FluentAssertions.Specs/Collections/CollectionAssertionSpecs.cs
Outdated
Show resolved
Hide resolved
Tests/FluentAssertions.Specs/Collections/CollectionAssertionSpecs.cs
Outdated
Show resolved
Hide resolved
f89b103 to
d04572d
Compare
Thanks @jnyrup! Should I put it into 6.0.0 section? |
|
d04572d to
215184d
Compare
|
@jnyrup sorry, I've accidentally pushed "trim" of release.md. Let me fix this, don't like unnecessary changes. |
215184d to
0f793ee
Compare
|
@jnyrup thanks for the review! I've reverted trim, it should be good now. |
0f793ee to
f1f7aeb
Compare
|
|
||
| if (foundIndices.Count > 0) | ||
| { | ||
| using (new AssertionScope()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this could be written as:
if (foundIndices.Count > 0)
{
Assertion assertion = Execute.Assertion
.BecauseOf(because, becauseArgs)
.WithExpectation("Expected {context:collection} {0} not to contain equivalent of {1}{reason}, ", Subject, unexpected)
.AddReportable("configuration", options.ToString());
if (foundIndices.Count == 1)
{
assertion
.FailWith("but found one at index {0}.", foundIndices[0])
.Then
.ClearExpectation();
}
...
}There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried similar approach earlier and it did not work unfortunately.
Firstly I was surprised that AddReportable is a void method, however I've extracted return value from .WithExpectation and just called AddReportable on it (that's not an issue of course).
The problem I faced is that AddReportable does not work when called outside new AssertionScope(). I'm not familiar with the codebase much, but quick look brought me to conclusion that "With" stuff is only attached on Dispose which seems to be not called on regular non-lazy assertion scope.
Am I missing something? What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't used AddReportable much, so @dennisdoomen might be better to answer this than me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok thanks.
BTW I added a test with using NotContainEquivalentOf inside AssertionScope: 05b5f66. I did it in a separate commit, because I don't know how this aligns with your testing approach. Looks like it works fine (if I understand "expected" correctly).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AddReportable was indeed only used by EquivalencyValidator to initialize an AssertionScope within which the entire equivalency engine runs.
…her assertions inside assertion scope
dennisdoomen
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nothing to add to @jnyrup's comments.
|
|
||
| if (foundIndices.Count > 0) | ||
| { | ||
| using (new AssertionScope()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AddReportable was indeed only used by EquivalencyValidator to initialize an AssertionScope within which the entire equivalency engine runs.
|
@vanashimko I saw you added a test to verify using |
|
@jnyrup I'm good |
Fixes #1287.
IMPORTANT