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

Provide opt-out to `AssertionOptions(o => o.WithStrictOrdering())` #974

Merged
merged 8 commits into from Dec 5, 2018

Conversation

@BrunoJuchli
Copy link
Contributor

BrunoJuchli commented Nov 18, 2018

This handles #966

Added the following configuration options:

  • WithoutStrictOrdering()
  • WithoutStrictOrderingForBytes()

I'm a bit unsure about WithoutStrictOrderingForBytes(). Reasoning:

Should WithoutStrictOrdering() remove ByteArrayOrderingRule? It seems likely that strict byte array ordering is almost always demanded. This poses a design-problem though. Since it's possible it was removed from the default it's not clear whether it should be present after performing:

 .WithStrictOrdering()
 .WithoutStrictOrdering()

Personally, it seems risky to have WithoutStrictOrdering remove the ByteArrayOrderingRule. One might expect bytes still to be compared in order. And you'll only find out your expectation is wrong when a bug occurs which you believed covered by your tests...

Having WithoutStrictOrdering always clearing the OrderingRuleCollection and then adding a ByteArrayOrderingRule might not be expected - especially if the ByteArrayOrderingRule was not present on the default. The issue will make itself visible by throwing an exception, though.
To remove the ByteArrayOrderingRule I could add another method to the interface (WithoutStrictOrderingForBytes). Removing all ordering rules would then become:

.WithoutStrictOrdering()
.WithoutStrictOrderingForBytes()

from a usage perspective I'd prefer this.

@dennisdoomen

This comment has been minimized.

Copy link
Member

dennisdoomen commented Nov 19, 2018

I think we can live with the situation that byte arrays are always compared in order, irrespective to the options you provide. Do you agree @jnyrup ?

@jnyrup

This comment has been minimized.

Copy link
Collaborator

jnyrup commented Nov 19, 2018

I agree that ByteArrayOrderingRule should still be present after calling WithoutStrictOrdering.

I'm not sure about WithoutStrictOrderingForBytes.
How would you express that functionality today?

WithoutStrictOrdering should be documentated in https://github.com/fluentassertions/fluentassertions/blob/master/docs/_pages/documentation.md.

@dennisdoomen

This comment has been minimized.

Copy link
Member

dennisdoomen commented Nov 20, 2018

I'm not sure about WithoutStrictOrderingForBytes.

I don't think we need this, do we really?

@BrunoJuchli

This comment has been minimized.

Copy link
Contributor Author

BrunoJuchli commented Nov 21, 2018

@jnyrup

I'm not sure about WithoutStrictOrderingForBytes.
How would you express that functionality today?

IMHO you can't.
Originally I had another impression because IEquivalencyAssertionOptions.OrderingRules is public gettable and I thought there was a way to get to IEquivalencyAssertionOptions. It would have been possible to perform OrderingRules.Add(RuleWhichIgnoresByteOrdering). Since the last rule applying to a member wins the IgnoreByteOrdering rule would have won over the the default behavior.


Hence I agree that WithoutStrictOrderingForBytes should not be introduced. Strict ordering for bytes should always be kept (as is the current behavior).
Furthermore OrderingRuleCollection.Clear must be internal, otherwise it would be possible to remove strict ordering for bytes.

@jnyrup

WithoutStrictOrdering should be documentated in https://github.com/fluentassertions/fluentassertions/blob/master/docs/_pages/documentation.md.

Will do, thanks for the hint.

@jnyrup

This comment has been minimized.

Copy link
Collaborator

jnyrup commented Nov 21, 2018

@BrunoJuchli Sounds good and thanks for thinking about making Clear() internal. No need to expose it.

Bruno Lorenz Juchli added 3 commits Nov 22, 2018
…ForBytes() and made OrderingRuleCollection.Clear() internal.
…tStrictorderingFor(...)`.
@BrunoJuchli

This comment has been minimized.

Copy link
Contributor Author

BrunoJuchli commented Nov 22, 2018

Breaking Change

As per review of @jnyrup I've introduced a breaking change.

Previously:

    .WithoutStrictOrderingFor(x => x.Foo)
    .WithStrictOrdering();

would result in strict ordering for everything but .Foo.

Now the same configuration results in strict ordering for everything.

Request for Decision

@fluentassertions
Could authority please decide among one of the following (or any other...) options:

  1. WithoutStrictOrdering and WithStrictOrdering do override previous WithStrictOrderingFor / WithoutStrictOrderingFor configurations
  2. WithoutStrictOrdering and WithStrictOrdering do not override previous WithStrictOrderingFor / WithoutStrictOrderingFor configurations
  3. WithStrictOrdering does not override previous WithoutStrictOrderingFor, but WithoutStrictOrdering does override previous WithStrictOrderingFor (=> no breakinge change. WithoutStrictOrdering didn't previously exist. Downside: WithStrictOrdering and WithoutStrictOrdering don't behave the same).

Personally I'd like the 1st option as it's how I expected FA to behave, but of course you might have a different opinion. Also, 1st option is a breaking change, the others aren't. I wouldn't go with the 3rd option though.

@dennisdoomen

This comment has been minimized.

Copy link
Member

dennisdoomen commented Nov 22, 2018

I think option 1 is the best. The order of the methods is the important part.

@jnyrup

This comment has been minimized.

Copy link
Collaborator

jnyrup commented Nov 22, 2018

I agree that option 1 is the best.
A potential runtime breaking change is fine with me, as the current behavior is unexpected to me.

I would expect the following two equivalences:

.WithoutStrictOrderingFor(x => x.Foo)
.WithStrictOrdering();

// to be equivalent to

.WithStrictOrdering();
.WithStrictOrderingFor(x => x.Foo)
.WithoutStrictOrdering();

// to be equivalent to

.WithoutStrictOrdering();

We apparently only have a single test for each of WithStrictOrderingFor and WithStrictOrderingFor.
It would make me less worried with tests for both the Expression<Func<IMemberInfo, bool>> and the Expression<Func<TExpectation, object>> overloads of WithStrictOrderingFor/WithoutStrictOrderingFor.
This also acts as documentation for the intended behavior.

@BrunoJuchli

This comment has been minimized.

Copy link
Contributor Author

BrunoJuchli commented Nov 22, 2018

Thanks

@jnyrup

We apparently only have a single test for each of WithStrictOrderingFor and WithStrictOrderingFor.
It would make me less worried with tests for both the Expression<Func<IMemberInfo, bool>> and the Expression<Func<TExpectation, object>> overloads of WithStrictOrderingFor/WithoutStrictOrderingFor.
This also acts as documentation for the intended behavior.

I'm not really sure I understand what you mean.
As i gather there's no WithoutStrictOrderingFor(Expression<Func<TExpectation, object>>). Only WithStrictOrderingFor(...) has two overloads.
For the existing overloads there's a test checking that performing WithStrictOrdering / WithoutStrictOrdering resets them. (=> only in the "resetting" direction, i.E. WithStrictOrderingFor(..) is reset by WithoutStrictOrdering()).

@jnyrup

This comment has been minimized.

Copy link
Collaborator

jnyrup commented Nov 22, 2018

To my surprise you're right.
I just assumed both overloads to exist for both WithStrictOrderingFor and WithoutStrictOrderingFor.
Seems like it should have been a part of #307.

My comment about a single test was regarding the existing test coverage for variants of StrictOrderingFor. As the fourth overload is non-existing, the test coverage you provide looks great.

@jnyrup
jnyrup approved these changes Nov 22, 2018
@jnyrup jnyrup merged commit 31d55a9 into fluentassertions:master Dec 5, 2018
1 check passed
1 check passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.