[NRTM] Issue 296: Generic Collections NotBeInAscendingOrder and NotBeInDescendingOrder #989
Conversation
… with NotBeIn and add obsolete attributes to old methods
…sertions and refactor BeOrderedBy to prevent wetness
…ollection assertions
jnyrup
left a comment
There was a problem hiding this comment.
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.
Src/FluentAssertions/Collections/GenericCollectionAssertions.cs
Outdated
Show resolved
Hide resolved
Src/FluentAssertions/Collections/GenericCollectionAssertions.cs
Outdated
Show resolved
Hide resolved
…ds to use non obsolete implementations per feedback
|
@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 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. |
|
cc @jnyrup |
| { | ||
| ICollection<T> unordered = Subject.ConvertOrCastToCollection(); | ||
|
|
||
| if (!unordered.Any()) |
There was a problem hiding this comment.
❓ How about sequences with a single item?
I guess they are, like empty lists, also both ordered and unordered at the same time.
There was a problem hiding this comment.
@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 😕
There was a problem hiding this comment.
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>();
}There was a problem hiding this comment.
@david-a-jetter Have you had a chance to look if the tests above still passes?
There was a problem hiding this comment.
@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();
}There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 ⬆️ ?
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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.
|
@jnyrup can this me merged? |
|
@dennisdoomen Not yet, see discussion above about the behavior of |
|
Can we come up with some guidelines on what behavior we expect, so @david-a-jetter or somebody else can finish this PR? |
|
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. |
* 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
This seemed like a simple enough issue to pick up as my first contribution.
-- Add Obsolete attribute to improperly named methods, to be removed in a major release
Resolves #296