BeInDescendingOrder does not validate correctly the order of a list of strings

[norcino]

Description

This is the first time I try to use 'BeInDescendingOrder' and I am getting an unexpected behaviour.
It seems to be failing when the list is actually correctly ordered.

Reproduction Steps

var descending = new List<string>
{
    "Z",
    "v",
    "a",
    "1"
};

try
{
    // This should not fail the validation but it does
    descending.Should().BeInDescendingOrder<string>(p => p);
}
catch
{
    // Failed with error:
    // Microsoft.VisualStudio.TestTools.UnitTesting.AssertFailedException: 'Expected descending {"Z", "v", "a", "1"} to be ordered "by " and result in {"v", "a", "Z", "1"}.'
}

// I try using Linq and make sure the strings are ordered descending
var definitelyDescending = descending.OrderByDescending(t => t).ToList();

try
{
    // Also the list of string ordered descending by linq is failing
    definitelyDescending.Should().BeInDescendingOrder<string>(p => p);
}
catch
{
    // Failed with error:
    // Microsoft.VisualStudio.TestTools.UnitTesting.AssertFailedException: 'Expected descending {"Z", "v", "a", "1"} to be ordered "by " and result in {"v", "a", "Z", "1"}.'
}

// Giving a go to ascending also causes a failure
var ascending = new List<string>
{
    "1",
    "a",
    "v",
    "Z"
};

try
{
    ascending.Should().BeInAscendingOrder(p => p);
}
catch
{
    // Failed with error:
    // Microsoft.VisualStudio.TestTools.UnitTesting.AssertFailedException: 'Expected ascending {"1", "a", "v", "Z"} to be ordered "by " and result in {"1", "Z", "a", "v"}.'
}

JSFiddle to reproduce the issue and demonstrate that using 'CompareTo' does work and the list is in the correct order.
https://dotnetfiddle.net/cODDFk

using System;
using System.Linq;
using Builder; // nuget pkg object.builder
using FluentAssertions;
			
public class Program
{
	public static void Main()
	{
		 var listOfStrings = Builder<string>.New().BuildMany(100);

		var orderedListOfStrings = listOfStrings.OrderByDescending(s => s).ToList();

		for (var i = 0; i < orderedListOfStrings.Count -1; i++)
		{
			if(orderedListOfStrings[i].CompareTo(orderedListOfStrings[i+1]) < 0) {
				Console.WriteLine(" - The strings are not ordered Descending using CompareTo");
			}
		}
		Console.WriteLine(" + The strings seems to be ordered correctly when using CompareTo");

		try
		{
			orderedListOfStrings.Should().BeInDescendingOrder(); }
		catch
		{
			Console.WriteLine(" - The strings are not ordered Descending according to 'BeInDescendingOrder'");
		}
		
		try
		{
			orderedListOfStrings.Should().BeInAscendingOrder();
		}
		catch
		{
			Console.WriteLine(" - The strings are not ordered BeInAscendingOrder according to 'BeInAscendingOrder'");
		}
	}
}

Expected behavior

I would expect the 'BeInDescendingOrder' to succeed when verifying a list of strings ordered using linq.

Actual behavior

Currently with the test data I get a validation failure when trying to process a list of strings correctly ordered

Regression?

No response

Known Workarounds

No response

Configuration

Using .net 6

Other information

No response

[dennisdoomen]

Since 6.9.0, by default, we order strings using an ordinal string comparison because we received bug reports where people were using different cultures without even realizing. There's an overload that takes an IComparer<T> in case you want to overrule this.

[norcino]

@dennisdoomen I don't want to reopen not argue with the decision made, but I would like to share my point of view.

Ignoring the technicalities, by default for dotnet and SQL server the array ordered ["Z", "v", "a", "1"], is the correct descending order.
Now I believe a library like this should, out of the box, validated correctly whatever default behaviour dotnet uses, customization should be allowed, for altering this default behaviour.
This was the first and only time when I used a fluent assertions method with a different behaviour to what I was expecting.

I hope you see my point.

[dennisdoomen]

Yeah, it's a trade-off. In #2075, we noticed that our tests started to fail depending on the operating system culture of whoever was running our tests. We could have solved that by fixing the culture for all tests to for example en-us. I'm not sure what's the best approach. Any thoughts on this @jnyrup ?

[norcino]

This is definitely a wide and complex issue, as I said I am not arguing, we need to document it clearly also through the method comments (for the intellisense).
An alternative, if the problem is the culture, is to perhaps try to get the current thread culture.
I mean when I ask linq to order a string array, they must be using some logic we can mimic.
The ugliest and list optimal is probably to clone the enumerable, order it with linq, and compare the 2 lists to make sure each element is in the right place, then of course leave the method open to configuration.

[IT-VBFK]

We could have solved that by fixing the culture for all tests to for example en-us

Remember, in #2084 and #2092 we fixed this :)

[dennisdoomen]

Remember, in #2084 and #2092 we fixed this :)

But that was just about the framework language right? It didn't change the thread's culture, did it?

[jnyrup]

Remember, in #2084 and #2092 we fixed this :)

But that was just about the framework language right? It didn't change the thread's culture, did it?

Those two PRs are enforcing what culture FA is running its build and tests under.
They are not enforcing a specific culture on consumers of FA.

We should not be messing with the culture of consumers.
If I set my culture set to da-DK, I don't want any library overriding it to en-US.

[dennisdoomen]

Those two PRs are enforcing what culture FA is running its build and tests under.
They are not enforcing a specific culture on consumers of FA.

Given that statement, why would we use ordinal comparisons then? We don't need it anymore to prevent contributors from having to deal with failing tests in FA.

[jnyrup]

The thought was to ensure that consumer test outcomes do not change between local and CI runs where culture often changes.