[NRTM] Issue 296: Generic Collections NotBeInAscendingOrder and NotBeInDescendingOrder by david-a-jetter · Pull Request #989 · fluentassertions/fluentassertions

david-a-jetter · 2019-01-06T21:47:42Z

This seemed like a simple enough issue to pick up as my first contribution.

Add properly named non-generic assertion methods
-- Add Obsolete attribute to improperly named methods, to be removed in a major release
Add methods to Generic Collection Assertions for NotBeInAscendingOrder and NotBeInDescendingOrder
Add unit tests for above
Add documentation for above

Resolves #296

… with NotBeIn and add obsolete attributes to old methods

…sertions and refactor BeOrderedBy to prevent wetness

…ollection assertions

…tion

jnyrup

Thanks for picking up this task. 👍
Aside from a few comments, it all looks good to me.
Please see http://dawehner.github.io/github,/code/review/2017/09/08/emoji-code-review.html for an explanation of the emojis.

docs/_pages/documentation.md

Src/FluentAssertions/Collections/GenericCollectionAssertions.cs

CONTRIBUTING.md

Src/FluentAssertions/Collections/CollectionAssertions.cs

…ds to use non obsolete implementations per feedback

david-a-jetter · 2019-01-08T01:08:48Z

@jnyrup Addressed comments.

I agree with moving to the non-obsolete methods since those will be covered by unit tests and the "redirect" makes sense.

I personally prefer the space after the ! since I find it stands out - people I work with often comment it is ugly, and I say that it's supposed to be - otherwise it's too easy to miss, but I can acquiesce to other styles too.

Lastly - I use this library all over the place, in nearly every project I work on. It's kinda cool to be contributing to it now.

dennisdoomen · 2019-01-08T20:06:14Z

cc @jnyrup

jnyrup · 2019-01-11T19:29:54Z

Src/FluentAssertions/Collections/GenericCollectionAssertions.cs

+            {
+                ICollection<T> unordered = Subject.ConvertOrCastToCollection();
+
+                if (!unordered.Any())


❓ How about sequences with a single item?
I guess they are, like empty lists, also both ordered and unordered at the same time.

@jnyrup My initial thought was that such an assertion should pass / not throw.

However, then I think about what problem this poses to the end user - and that would be that their test fails when really their test is invalid, and is either not using the appropriate assertion or does not have the appropriate arrange/act. And in that case, I would prefer the safety net of the failing assertion so I have an opportunity to correct my test.

The above may be confirmation bias because this is also the current behavior 😕

Here are some quick tests (if incorporated, please update them to FA style) on how FA currently behaves.
Would this PR break any of those?

[TestMethod] public void EmptyCollection_BeInAscendingOrder() { Action act = () => new int[] { }.Should().BeInAscendingOrder(); act.Should().NotThrow(); } [TestMethod] public void EmptyCollection_BeInDescendingOrder() { Action act = () => new int[] { }.Should().BeInDescendingOrder(); act.Should().NotThrow(); } [TestMethod] public void SingleElement_BeInAscendingOrder() { Action act = () => new int[] { 42 }.Should().BeInAscendingOrder(); act.Should().NotThrow(); } [TestMethod] public void SingleElement_BeInDescendingOrder() { Action act = () => new int[] { 42 }.Should().BeInDescendingOrder(); act.Should().NotThrow(); } [TestMethod] public void EmptyCollection_NotBeInAscendingOrder() { Action act = () => new int[] { }.Should().NotBeAscendingInOrder(); act.Should().Throw<Exception>(); } [TestMethod] public void EmptyCollection_NotBeInDescendingOrder() { Action act = () => new int[] { }.Should().NotBeDescendingInOrder(); act.Should().Throw<Exception>(); } [TestMethod] public void SingleElement_NotBeInAscendingOrder() { Action act = () => new int[] { 42 }.Should().NotBeAscendingInOrder(); act.Should().Throw<Exception>(); } [TestMethod] public void SingleElement_NotBeInDescendingOrder() { Action act = () => new int[] { 42 }.Should().NotBeDescendingInOrder(); act.Should().Throw<Exception>(); }

@david-a-jetter Have you had a chance to look if the tests above still passes?

@jnyrup These tests pass - and I can work to rearrange them so they match in style of the other tests.

However, it seems like a similar test using an anonymous type and specifying the property results in a failure. Looks like more work to do still... different behavior of primitive collections vs. complex types, which is not okay. For example:

[Fact] public void When_asserting_single_item_collections_are_not_in_an_descendingly_ordered_collection_are_not_ordered_descending_it_should_succeed() { //----------------------------------------------------------------------------------------------------------- // Arrange //----------------------------------------------------------------------------------------------------------- var collection = new[] { new { Text = "b", Numeric = 1 } }; //----------------------------------------------------------------------------------------------------------- // Act //----------------------------------------------------------------------------------------------------------- Action act = () => collection.Should().NotBeInDescendingOrder(o => o.Numeric, Comparer<int>.Default); //----------------------------------------------------------------------------------------------------------- // Assert //----------------------------------------------------------------------------------------------------------- act.Should().NotThrow(); }

Hmm. If somebody uses these methods, aren't they already expecting a collection? Why else would you use those specific assertions? So if instead of a collection, the test results in an empty collection or a collection with just one item, I would expect it to fail.

Hmm. If somebody uses these methods, aren't they already expecting a collection? Why else would you use those specific assertions? So if instead of a collection, the test results in an empty collection or a collection with just one item, I would expect it to fail.

@jnyrup did you see ⬆️ ?

@david-a-jetter I'm not sure what the problem is (or maybe I forgot).

I just tried asserting collection sortedness in NUnit:
empty and single element collections both:

passes Assert.That(list, Is.Ordered);, and

fails Assert.That(list, Is.Not.Ordered);

I came to think of two inherent properties of empty/single element collections.
They are:

always ordered, and

both in ascending and descending order at the same time.

I came to think of two inherent properties of empty/single element collections.
They are:

always ordered, and

both in ascending and descending order at the same time.

I think this is what we should implement.

dennisdoomen · 2019-01-23T09:31:22Z

@jnyrup can this me merged?

jnyrup · 2019-01-23T09:51:48Z

@dennisdoomen Not yet, see discussion above about the behavior of NotBeOrderedBy.

Uncertainty about NotBeOrderedBy

dennisdoomen · 2019-05-25T18:00:50Z

Can we come up with some guidelines on what behavior we expect, so @david-a-jetter or somebody else can finish this PR?

danielmpetrov · 2019-09-16T17:36:11Z

Is this PR still of interest? @dennisdoomen @jnyrup

What's the stopper here?

Maybe if @david-a-jetter is MIA I can try to finish it instead.

@jnyrup

* rename NotBeAscendingIn and NotBeDescendingIn order methods to prefix with NotBeIn and add obsolete attributes to old methods * add not ascending and not descending methods to generic collection assertions and refactor BeOrderedBy to prevent wetness * finished unit new unit tests for NotBeInDescendingOrder for generic collection assertions * add unit tests for IsNotInAscendingOrder and fix bug for empty collection * updated docs for non generic assertion renames * update some code style and update collection serrtions obsolete methods to use non obsolete implementations per feedback * Add tests suggested by @jnyrup * Remove duplicated tests * Add and organize tests into regions * Update docs * Update tests * Update documentation * Add remarks to the generic variants * Remove extra space in exception message * Decorate obsolete methods with EditorBrowsableState.Never

david-a-jetter added 5 commits December 30, 2018 10:08

rename NotBeAscendingIn and NotBeDescendingIn order methods to prefix…

04e6fd5

… with NotBeIn and add obsolete attributes to old methods

add not ascending and not descending methods to generic collection as…

de54b2d

…sertions and refactor BeOrderedBy to prevent wetness

finished unit new unit tests for NotBeInDescendingOrder for generic c…

ebe838c

…ollection assertions

add unit tests for IsNotInAscendingOrder and fix bug for empty collec…

9b70dc2

…tion

updated docs for non generic assertion renames

206b2e3

jnyrup requested changes Jan 7, 2019

View reviewed changes

update some code style and update collection serrtions obsolete metho…

ee2cb1d

…ds to use non obsolete implementations per feedback

jnyrup previously approved these changes Jan 8, 2019

View reviewed changes

jnyrup reviewed Jan 11, 2019

View reviewed changes

david-a-jetter changed the title ~~Issue 296: Generic Collections NotBeInAscendingOrder and NotBeInDescendingOrder~~ [NRTM] Issue 296: Generic Collections NotBeInAscendingOrder and NotBeInDescendingOrder Feb 3, 2019

danielmpetrov mentioned this pull request Sep 18, 2019

Continuation of PR #989 #1140

Merged

jnyrup closed this in #1140 Sep 21, 2019

Conversation

david-a-jetter commented Jan 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jnyrup left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

david-a-jetter commented Jan 8, 2019

Uh oh!

dennisdoomen commented Jan 8, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

david-a-jetter Feb 3, 2019 • edited by jnyrup Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dennisdoomen commented Jan 23, 2019

Uh oh!

jnyrup commented Jan 23, 2019

Uh oh!

dennisdoomen commented May 25, 2019

Uh oh!

danielmpetrov commented Sep 16, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

david-a-jetter commented Jan 6, 2019 •

edited

Loading

david-a-jetter Feb 3, 2019 •

edited by jnyrup

Loading