Fix culture and line endings issues in tests

[volanavlad]

Fix some tests failures:

• Due to bad line endings in xml resources. Replaced to xml entities 


• If current culture is not invariant some tests fail due to exception messages are localized while assert strings are not. So it would be better to have localized strings in tests.

[dnfclas]

All CLA requirements met.

[skofman1]

Thanks for the contribution @volanavlad ! Can you please open an issue with the problem that you are fixing. We will work with you on merging this fix.

[volanavlad]

Here is the issue: #7779

[joelverhagen]

Regarding the newline changes, could this perhaps be due to Git configuration? I think there is a setting that tells give to checkout text files with \r\n but check-in \n -- this is the setting I have on my machine. Perhaps you have a setting that checks out \n?

I am surprised to see \n in there for your case and I am wondering if that is even related to your locale.

[volanavlad]

You are right - issues are due to git settings and default OS UI culture.
But I think it is wrong to require windows line endings and En culture for all contributers while it can be easily fixed.
It annoys to have test errors while you need simply to check that your changes do not make new issues.

[joelverhagen]

Regarding the line ending setting - totally agree this is a bad experience. Let's fix it! If we switch to XML entities in the RESX file will that get clobbered by the Visual Studio RESX editor when we make a change to the string?

[volanavlad]

I have just tested it.
Visual Studio 2019 designer replaces XML entities to \r\n back :-(
But you can see it in commit diff and revert manually.

[joelverhagen]

Could you make the change on the test side?

[volanavlad]

Yes. I can move assert string to resx. So git setting will apply both to checked and actual strings. Test will work fine in both cases.

[volanavlad]

Fixed in commit '9ee4a940851e505d27ecb52d956b9ea4741f3159'

[volanavlad]

+new issue fixed: #7783

[volanavlad]

Sorry
It is very strange!
May be some internal string representations differs?

NuGetGallery.Security.MicrosoftTeamSubscriptionFacts.Policies_ReturnsMicrosoftTeamSubscriptionPolicies
Source: MicrosoftTeamSubscriptionFacts.cs line 15
Duration: 3 sec

Message:
Assert.Equal() Failure
↓ (pos 957)
Expected: ···or more information.\r\nPolicy violations: {0}"}
Actual: ···or more information.\r\nPolicy violations: {0}"}
↑ (pos 957)
Stack Trace:
Assert.Equal(String expected, String actual, Boolean ignoreCase, Boolean ignoreLineEndingDifferences, Boolean ignoreWhiteSpaceDifferences) line 244
Assert.Equal(String expected, String actual) line 174
MicrosoftTeamSubscriptionFacts.Policies_ReturnsMicrosoftTeamSubscriptionPolicies() line 48

[volanavlad]

It is json string in policy.Value
So it is escaped.

OK

Wait for next commit please.

[volanavlad]

fixed

[loic-sharma]

Instead of supporting localization here, would it be simpler to check the param name separately? In other words:

1. Assert that the message begins with 'bad' is not a valid version string.
2. Assert that the ex.ParamName is "value"

I'm concerned that supporting all locales will be tough 😅

[loic-sharma]

As an alternative, see my comment below: #7778 (comment)

[loic-sharma]

If current culture is not invariant some tests fail due to exception messages are localized while assert strings are not. So it would be better to have localized strings in tests.

Why is it better to have localized strings in tests? Wouldn't it be easier to set the current culture to invariant instead of localizing the tests? It seems like we could add something like this to our unit tests:

CultureInfo.CurrentCulture = CultureInfo.InvariantCulture;

[volanavlad]

Why is it better to have localized strings in tests? Wouldn't it be easier to set the current culture to invariant instead of localizing the tests? It seems like we could add something like this to our u

CultureInfo.CurrentCulture = CultureInfo.InvariantCulture;

I started with this idea. But it requires much more refactoring of test code. For example - it is quite strange to change current ui culture in the middle of the test (i.e. inside assert function like tests/NuGetGallery.Facts/TestUtils/ContractAssert.cs ThrowsArgException() )
So you need to implement some base test classes with cuture set in constructor and inherit failed test from it.
But there are existing inheritance - so you need to check it... etc...
Moreover there are some use cases where it is not enought to set culture in current thread only - you to ensure that async context is well too....
I think it is much easy to have localized strings where it is needed.

[loic-sharma]

I started with this idea. But it requires much more refactoring of test code. For example - it is quite strange to change current ui culture in the middle of the test (i.e. inside assert function like tests/NuGetGallery.Facts/TestUtils/ContractAssert.cs ThrowsArgException() )

Ah that's a good point. What do you think of creating a new attribute for unit tests that require localization? Something like:

public class InvariantCultureFactAttribute : FactAttribute
{
    public InvariantCultureFactAttribute()
    {
        CultureInfo.CurrentCulture = CultureInfo.InvariantCulture;
    }
}

Then the affected tests can look like:

[InvariantCultureFact]
public void MyTest()
{
    // The current culture is invariant now
}

This way we don't need to refactor too much code 😊

Moreover there are some use cases where it is not enought to set culture in current thread only - you to ensure that async context is well too....

Oof, that's tricky... For these tests that require multiple threads, I think it'd be best if we remove the assertions that require localization.

[zhhyu]

Hi, @volanavlad. We are working on this problem, and seeking the best solution. Feel free to follow up this PR or the issue. Thanks!

[agr]

I don't think introducing localization is the way to go: yes, it will fix it for Russian in the short term, but I imagine other languages would have the same issues as well, and we don't want to end up supporting a dozen of localisations just for the sake of tests.

I installed the Russian language pack in Windows and managed to reproduce the test failures due to exception text localization. All but two failures are due to, in my opinion, a bit too strict Assert.Equals(expectedMessage, ex.Message), which can be replaced with Assert.StartsWith(expectedStart, ex.Message) which would not include the new line characters and the "Parameter name" tail of the message (which gets localized).

Out of the remaining two, ThrowsForEmptyAndNonEmptyDuplicatesWhenDuplicateMetadataElementsDetectedAndParsingIsNotStrict should be fixed by setting current thread's UI culture carefully, since if we are expecting certain exception text, we have to eliminate the possible variability. Alternatively, we can file an issue against corefx to introduce a special type of exception for duplicate keys in a dictionary, but I have a feeling that is unlikely to get addressed.

Lastly, WillShowViewWithErrorsIfPackageIdIsBreaksParsing is potentially the tricky one since it seems the actual service code (the one being tested) needs improvement to not bubble service exceptions to the user, but use custom error message instead.

[zhhyu]

Hi, @volanavlad! We have a recent fix for this issue: #8021.
Feel free to have a try on the latest master or dev branch, and see whether the issue still exists. If it still exists, feel free to leave the comment here. Thanks!