Skip to content

Conversation

@ishymko
Copy link
Contributor

@ishymko ishymko 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.

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

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

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

ishymko commented Apr 26, 2020

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

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

ishymko commented Apr 26, 2020

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

@ishymko ishymko 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.

@ishymko
Copy link
Contributor Author

ishymko 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