Remove StringUtil class and update tests references #4819

[yergios]

Fixes #4819

Proceeded to removing the whole internal StringUtil class and replaced its methods references at the tests for built-in System.StringComparison.

[manfred-brands]

In my opinion path comparison should be using Ordinal comparison.
But I would like a second opinion before I request changes. @OsirisTerje ?

[stevenaw]

I would typically be cautious about changing the comparison, however after a bit of investigation it would seem that the current recommendations are for these comparisons to be ordinal (https://learn.microsoft.com/en-us/dotnet/standard/base-types/best-practices-strings).

I did find this mono issue from a few years back where it's alluded that this culture-specific comparison may be a relic of older Windows versions. In the thread it's then suggested that, at the time, mono should not make that change "blindly" on their end and that instead caution should be taken (mono/mono#10230 (comment)).

All that is to say I'm also curious @OsirisTerje 's thoughts on the matter.

EDIT: Also, if it's not an easy decision on our end then it might also be worth not holding up the PR over as I'm sure @mr-sergito is eager to land their first PR :)

[stevenaw]

Thanks @mr-sergito ! The changes look great. I have left one minor suggestion outside of the Ordinal vs culture discussion to help avoid duplication while still allowing us to remove the StringUtil class.

[stevenaw]

One suggestion: As SamePathConstraint and SamePathOrUnderConstraint both inherit from PathConstraint it may help avoid duplication if we exposed a protected property or method here in PathConstraint to encapsulate the case-sensitivity check and return a StringComparison for use.

[yergios]

Hey @stevenaw ! Thanks for the review, I'll make this improvement right away.

[OsirisTerje]

Being a Norwegian we do have locale issues. We should not use CurrentCulture, as that may change dependent upon locale installation. Since this is for Paths, Ordinal would be the preferred. Imho.

But, since this is a PathConstraint and part of the framework, may it have some effects on someones tests if we change it? We don't have any issues for the PathConstraint itself (open nor closed) as far as I can see.

[manfred-brands]

But, since this is a PathConstraint and part of the framework, may it have some effects on someones tests if we change it? We don't have any issues for the PathConstraint itself (open nor closed) as far as I can see.

We currently have a non-consistent system due to changes in culture comparison in .net 6

Using the German culture (de-DE), .NET Framework treats 'ss' as being equal to 'ß'. But .NET 6+ does not, neither does the file system. The below test passes in the first, fails in the latter.

[Test]
[SetCulture("de-DE")]
public void PathNameComparison()
{
    Assert.That("Grass", Is.SamePath("Graß"));
}

But when creating files with those two names, regardless of Framework, I get two files:

[Test]
[SetCulture("de-DE")]
public void PathContentComparison()
{
    const string Contents = "Written";
    const string OtherContents = "But not really";
    File.WriteAllText("Graß", OtherContents);
    File.WriteAllText("Grass", Contents);
    string content = File.ReadAllText("Graß");
    Assert.That(content, Is.EqualTo(Contents));
}

To me this means two things:

1. Even though we used CurrentCulture we have no tests for this (special non-ascii characters) as otherwise we would have notice .NET core difference.
2. File System is not Culture aware (but then my OS is en-AU)

So I think we should use Ordinal(IgnoreCase) file file system comparison.

[stevenaw]

I'm alright with converting these to ordinal too. I haven't been able to find any evidence of a present or historical OS filesystem which relies on culture. Based on that, we can likely infer that any path-related tests which rely on culture are rare at best, and any that do would likely not accurately test the underlying system.

Lastly, I have to wonder if the culture comparisons in this helper were originally added for other reasons. Some .net functions like StartsWith(string) and EndsWith(string) are themselves culture-aware and so it's possible the culture check were originally added in the NUnit helper to accurately test the corresponding assertion. I haven't done the code archeology to confirm, but it could explain how culture comparisons also came to be used on path-specific use cases.

In summary, and based on the reactions to @manfred-brands comment above, I think the team agrees we should convert these to ordinal 🙂

[yergios]

Thanks everyone for the insights! I honestly didn't know that much about string comparisons, it has been interesting.

I've made the suggested changes regarding string comparisons on path-related tests.

[stevenaw]

This looks great to me, thanks for your contribution @mr-sergito !

@manfred-brands I'll wait a few days to merge to allow you the chance to review if you wanted given you helped shape direction of this too

Forgot to run CI

[stevenaw]

Looks like a test failure on the Mac agent. I'm having a hard time seeing the failure on my phone but I'll try again from a computer later on. Seems related to a "custom teardown action"?

[stevenaw]

CI is happy again

[manfred-brands]

I also have no further change requests.
Thanks again @mr-sergito for your cooperation.