Fix tests failing when run on systems with non-English language/locale settings

[dotnokato]

Bug

Fixes: NuGet/Home#12820

Regression? Last working version:

Description

Fixed tests that were failing when run on systems with non-English language/locale settings

• tests that relied on English language
  • updated asserts to use product resource string instead of hardcoded en-US strings where viable
  • for tests asserting exception message text, set the Thread.CurrentThread.CurrentUICulture = CultureInfo.InvariantCulture;
• tests failing due to expected values being English-localized (e.g. decimal separator = .)
  • converted the expected values from string to decimal and back to (localized) string
  • set Thread.CurrentThread.CurrentCulture = CultureInfo.InvariantCulture; where the above was not viable
• updated test PerformUpToDateCheck_WhenNonBuildIntegratedProjectIsAParentOfADirtySpec_ReturnsAListWithoutNonBuildIntegratedProjects to use TestDirectory instead of creating test files in C:\ drive
• renamed test ToReadableTimeFormat_Localized_AssumesEnglishLocale_ContainsTimeNumber_Succeeds to ToReadableTimeFormat_Localized_AssumesCurrentCulture_ContainsTimeNumber_Succeeds so that the name corresponds to the behaviour of method under test (after verifying the way it is being used)

PR Checklist

• PR has a meaningful title

• PR has a linked issue.

• Described changes

• Tests

  • Automated tests added
  • OR
  • Test exception
  • OR
  • N/A

• Documentation

  • Documentation PR or issue filled
  • OR
  • N/A

[jeffkl]

Would it make sense to set this globally for all of our tests so that in the future we don't cause this to happen again?

[dotnokato]

I think it would. The question is how to do it assembly/codebase wise?
Or do we want at least to set this on a test class level instead of individual tests level?

[zivkan]

To be honest, making the threads run in invariant culture doesn't make me proud of our code quality.

Something I particularly don't like is that there's no cleanup afterwards to set the culture back to the original value. If there are other tests that explicitly want to test some other culture (I think Fernando added one or two within the last 2 years, but before that I'd say NuGet has none), then this could introduce flakiness depending on whether or not the other test re-uses the same thread. We also run tests in parllel, and this method is async, which increases risk that another test could re-use the same thread.

My memory is that xunit has its own set of threads to run tests on, which means that anywhere NuGet does .ConfigureAwait(false) (which I think we only have two or three examples of, in NuGet.Protocol, but we might end up doing more in the future), or anywhere Task.Run() is used, code might end up running on the thread pool, which is still using the default UI culture.

There's always a case of "don't let perfect be the enemy of good", so we don't have to change the test code to work correctly when using the default system culture. Maybe this "set current thread UI culture to invariant" is good enough. But how difficult is it to "properly fix" tests instead? I've been meaning to try myself, but I don't want to change my system locale on a machine I really use, and creating a new VM, installing all the dev tools, and cloning the repo has been too much effort for me so far.

[ghost]

This PR has been automatically marked as stale because it has no activity for 7 days. It will be closed if no further activity occurs within another 90 days of this comment. If it is closed, you may reopen it anytime when you're ready again, as long as you don't delete the branch.

[dotnokato]

I've pushed updated version that uses UseCulture attribute similar to https://github.com/xunit/samples.xunit/blob/main/UseCulture/UseCultureAttribute.cs. This way, the thread culture is only changed for the time of the test. Then it is being reset to the original value.

[ghost]

This PR has been automatically marked as stale because it has no activity for 7 days. It will be closed if no further activity occurs within another 90 days of this comment. If it is closed, you may reopen it anytime when you're ready again, as long as you don't delete the branch.