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 NotContainEquivalentOf #1318

Merged

Conversation

ishimko
Copy link
Contributor

@ishimko ishimko commented Apr 26, 2020

Fixes #1287.

IMPORTANT

  • The code complies with the Coding Guidelines for C#.
  • The changes are covered by a new or existing set of unit tests which follow the Arrange-Act-Assert syntax such as is used in this example.
  • If the contribution affects the documentation, please include your changes in this pull request so the documentation will appear on the website.

Copy link
Member

@jnyrup jnyrup left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ishimko ishimko force-pushed the issue/1287-not-contain-equivalent-of branch 2 times, most recently from f89b103 to d04572d Compare April 26, 2020 11:20
@ishimko
Copy link
Contributor Author

ishimko commented Apr 26, 2020

Nice work 👍
Could I ask you to add a line to https://github.com/fluentassertions/fluentassertions/blob/develop/docs/_pages/releases.md as well?

Thanks @jnyrup! Should I put it into 6.0.0 section?

@jnyrup
Copy link
Member

jnyrup commented Apr 26, 2020

Thanks @jnyrup! Should I put it into 6.0.0 section?
Yes, the develop branch is what will eventually become 6.0.0.

@ishimko ishimko force-pushed the issue/1287-not-contain-equivalent-of branch from d04572d to 215184d Compare April 26, 2020 11:29
@ishimko
Copy link
Contributor Author

ishimko commented Apr 26, 2020

@jnyrup sorry, I've accidentally pushed "trim" of release.md. Let me fix this, don't like unnecessary changes.

@ishimko ishimko force-pushed the issue/1287-not-contain-equivalent-of branch from 215184d to 0f793ee Compare April 26, 2020 11:38
@ishimko
Copy link
Contributor Author

ishimko commented Apr 26, 2020

@jnyrup thanks for the review! I've reverted trim, it should be good now.

@ishimko ishimko force-pushed the issue/1287-not-contain-equivalent-of branch from 0f793ee to f1f7aeb Compare April 26, 2020 12:02

if (foundIndices.Count > 0)
{
using (new AssertionScope())
Copy link
Member

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();
    }
...
}

Copy link
Contributor Author

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?

Copy link
Member

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.

Copy link
Contributor Author

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).

Copy link
Member

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.

Copy link
Member

@dennisdoomen dennisdoomen left a 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())
Copy link
Member

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.

@jnyrup
Copy link
Member

jnyrup commented Apr 26, 2020

@vanashimko I saw you added a test to verify using new AssertionScope().
Unless you have anything else, I'm fine merging this.

@ishimko
Copy link
Contributor Author

ishimko commented Apr 26, 2020

@jnyrup I'm good

@jnyrup jnyrup changed the title Add "not contain equivalent of" assertion (#1287) Add NotContainEquivalentOf Apr 26, 2020
@jnyrup jnyrup merged commit 0b37e37 into fluentassertions:develop Apr 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add NotContainEquivalentOf
3 participants