Skip to content

Implement NormalizeLineEndings - #4976

Merged
manfred-brands merged 11 commits into
nunit:mainfrom
Bartleby2718:main
Apr 19, 2025
Merged

manfred-brands merged 11 commits into
nunit:mainfrom
Bartleby2718:main

Conversation

@Bartleby2718

@Bartleby2718 Bartleby2718 commented Apr 12, 2025

Copy link
Copy Markdown
Contributor

This fixes #4975.

Comment thread src/NUnitFramework/framework/Constraints/CollectionItemsEqualConstraint.cs Outdated
/// </summary>
internal sealed class LineEndingNormalizingStringComparer : IEqualityComparer<string>
{
/// <summary>

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only two of these are used, but I just created singletons for all 6 StringComparison values.

Comment thread src/NUnitFramework/framework/Constraints/EqualConstraint.cs Outdated
return x.Equals(y, ignoreCase ? StringComparison.CurrentCultureIgnoreCase : StringComparison.Ordinal);
IEqualityComparer<string> comparer = (ignoreCase, NormalizeLineEndings) switch
{
(true, true) => LineEndingNormalizingStringComparer.CurrentCultureIgnoreCase,

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CurrentCultureIgnoreCase and Ordinal to be consistent with the existing code.

_contents = new List<object?>(source);
}

public override string ToString() => $"SimpleObjectCollection [{string.Join(", ", _contents)}]";

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated because I found Test Explorer misleading without this.

Comment thread src/NUnitFramework/framework/Constraints/UniqueItemsConstraint.cs Outdated
Comment thread src/NUnitFramework/framework/Constraints/EqualConstraint.cs Outdated
Comment thread src/NUnitFramework/framework/Constraints/EqualStringWithoutUsingConstraint.cs Outdated
Comment thread src/NUnitFramework/framework/Constraints/ContainsConstraint.cs Outdated
Comment thread src/NUnitFramework/framework/Constraints/NUnitEqualityComparer.cs Outdated

@manfred-brands manfred-brands left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @Bartleby2718 for your contribution.
You already found your casing problem in parameters and fields.
It would have been easier if you had just fixed the instead of putting a comment on your own PR that you need to fix it.

The actual comparer looks good.
The main change I would like is to rename the word "Normalize" and "Normalizing"
One what is normal for one might not be normal for another.
But more to match the other modifiers that it "Ignore" an aspect.
It doesn't change any strings but ignores different style line endings.

Comment thread src/NUnitFramework/framework/Constraints/CollectionItemsEqualConstraint.cs Outdated
Comment thread src/NUnitFramework/framework/Constraints/Comparers/StringsComparer.cs Outdated
Comment thread src/NUnitFramework/framework/Constraints/EqualConstraint.cs Outdated
Comment thread src/NUnitFramework/framework/Constraints/EqualConstraint.cs Outdated
Comment thread src/NUnitFramework/framework/Constraints/EqualStringWithoutUsingConstraint.cs Outdated
Comment thread src/NUnitFramework/tests/Constraints/AnyOfConstraintTests.cs

@manfred-brands manfred-brands left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @Bartleby2718. mostly fine.
A few things remain.

Comment thread src/NUnitFramework/framework/Constraints/StringConstraint.cs Outdated
Comment thread src/NUnitFramework/framework/Constraints/UniqueItemsConstraint.cs Outdated
Comment thread src/NUnitFramework/framework/Constraints/MessageWriter.cs

@manfred-brands manfred-brands left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good now.

This returned the same number for every string, which is technically allowed.
It just cuased the unit test to slow down dramatically.
@manfred-brands

Copy link
Copy Markdown
Member

@Bartleby2718 There is a serious performance degradation:

Warning : NUnit.Framework.Tests.Constraints.UniqueItemsConstraintTests.PerformanceTests_FastPath(System.Collections.Generic.List`1[System.String],True)
  Warn.Unless(() =>
            {
                if (ignoreCase)
                    Assert.That(values, Is.Unique.IgnoreCase);
                else
                    Assert.That(values, Is.Unique);
            }, HelperConstraints.HasMaxTime(100))
Elapsed time of 3696ms exceeds maximum of 100ms
   at NUnit.Framework.Tests.Constraints.UniqueItemsConstraintTests.PerformanceTests_FastPath(IEnumerable values, Boolean ignoreCase) in /_/src/NUnitFramework/tests/Constraints/UniqueItemsConstraintTests.cs:line 186

I have traced it down to a subtle line I missed in my review: comparer.GetHashCode() which should have been comparer.GetHashCode(s)
Putting all items in the same 'bucket' in a HashSet slowed down the test dramatically.

@manfred-brands
manfred-brands merged commit d4a5c38 into nunit:main Apr 19, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support ignoring line ending format when comparing strings

2 participants