CurrentCulture in unit tests?

[weltkante]

While updating the PaddingConverter tests in PR #2679 I noticed that it contains tests which only succeed on english (or similar) environments which use comma as separator. Some languages use semicolon due to comma being used as decimal separator. This leads to false-positive failing tests on developer machines.

Failing tests are PaddingConverter_ConvertFrom_String_ReturnsExpected on e.g. this input as well as PaddingConverter_ConvertTo_String_ReturnsExpected. (I'm only selectively running tests locally so I'd assume there could be more instances of this problem in other tests.)

Do you think tests should be running under fixed CurrentCulture (maybe some sort of fixture)? Maybe check how corefx is solving this problem when they are testing their converters/parsers.

[weltkante]

Just for the record, there are currently 105 failing tests, at least 94 of them are due to tests assuming english culture.

This also means you probably have no test coverage against culture dependent regressions in CI.

If you want an overview over which tests are failing I have a mostly working branch here, trying to get myself unstuck because currently running tests is a pain (I can't see if I'm adding a new test failure when running tests locally) - but I wouldn't want to PR that branch, there are probably better solutions than manually identifying which tests need english culture.

Personally I'd probably prefer if all tests are run under the same culture and then have two passes over all tests in two representative cultures, say one for english and one for a culture which is sufficiently different (de-DE and es-ES are candidates that differ in at least 3 separator tokens commonly used in WinForms). That should catch most generic culture regressions if you care about that kind of thing, of course regressions in a specific culture are only caught by exhaustive testing of every language which has its own localized strings, probably not worth for CI that runs on every PR.

Once you figure out what you actually want I can do a PR for the code part of the required changes, but someone else has to look at the infrastructure changes if any are necessary, arcade CI is a black box to me.

Also its probably still advisable to query the dotnet runtime guys how they handle culture dependent tests, they might have a better idea of what makes sense and how to do things.

(PS: I'm in no hurry, just reporting back from what I'm seeing while working my PRs.)

[RussKie]

I see you're making progress on this issue. Few thoughts on the subject.

I wonder if we could do it declaratively via traits, so we can have attributes similar to NUnit.
I found this article which looks interesting. If I understood it correctly we could have a custom attribute, say SetCulture (references: #1, #2) that sets the desired culture on the current thread.

Alternatively, may be we can add it to ThreadExceptionFixture (we'll need to rename it to have a more generic name)?

[weltkante]

I see you're making progress on this issue. Few thoughts on the subject.

Yes its solved locally for me by annotating every test individually.

maybe we can add it to ThreadExceptionFixture

I tried doing it in a fixture but it seemed to not work. I assumed fixtures have no guarantee to run on the same thread as tests and didn't examine this further. Its possible I was mistaken, I didn't spend a lot of time on it.

we'll need to rename it to have a more generic name

You can have multiple fixtures

[RussKie]

I think this issue has resolved. I'm no longer getting any locale specific failures.
Holler if you still do, and we can reopen it.