Add tests for de-CH

[tmilnthorp]

No description provided.

[codecov-commenter]

Codecov Report

Merging #799 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #799   +/-   ##
=======================================
  Coverage   80.51%   80.51%           
=======================================
  Files         280      280           
  Lines       41851    41851           
=======================================
  Hits        33696    33696           
  Misses       8155     8155           

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5719cb1...64923d2. Read the comment docs.

[angularsen]

If you want, we could refactor this to combinatorial parameters. It would make it a lot less verbose to cover all the same test cases.

https://github.com/AArnott/Xunit.Combinatorial

[tmilnthorp]

Oh man. I Googled for it using the word "matrix" as was bummed thinking you couldn't do it. Thanks!

[angularsen]

Matrix was my first go to search as well :D

[pgrawehr]

... and I implemented it first in a do-it-yourself way (see patch below)

[pgrawehr]

Now this did fail for me...

This was partially my misstake. And more seriously Microsoft's...

Windows decided recently to change some of the culture defaults. In this case, the default thousands separator for de-CH is ’ (a typographic apostrophe), while I (for whatever reason, probably because it always was like this) have set it to ' (the normal apostrophe). Now using new CultureInfo("de-CH") returns something different if this is the current locale and the defaults are overwritten in control panel. When I change this to new CultureInfo("de-CH", false) everywhere, your PR passes for me.

Note 1: Always use new CultureInfo(name, false) in unit tests, or the behavior might differ on a different computer due to user overrides (the "false" says: ignore any user settings).

Note 2: That doesn't solve the general problem that if the separator is set to ', the problem arises.

[pgrawehr]

0001-Separate-culture-from-string-list.txt

Here's a suggested patch for this PR. Supports mixing arbitrary cultures with arbitrary test data without cloning all the strings. Tests are written to fail (due to the manipulated "de-AT" culture settings)

[tmilnthorp]

I updated the tests but upon further reflection I don't think it's the way to go. A string of 1,000' makes sense for en-US but not for de-CH (I assume it would be 1'111'). Then again would they even use ' for ft?

[tmilnthorp]

Doh. Force push fail.

[angularsen]

I guess you are right, the new tests didn't really add anything. If we are testing more cultures, it should test new inputs I think.

[angularsen]

Also, the other PR is removing support for thousand separators so that input variation goes out the window here.

[tmilnthorp]

Yea we definitely don't want to remove that ability. I created #803 as a starting point.

[pgrawehr]

I updated the tests but upon further reflection I don't think it's the way to go. A string of 1,000' makes sense for en-US but not for de-CH (I assume it would be 1'111'). Then again would they even use ' for ft?

I belive you are right. While there are (very few) use cases for american units in europe, they rarely ever use a combination of feet and inches. Pilots use feet for altitude and screen sizes are advertised in inches, but if you give the height of a persion with 5' 3" you'll be looked at very peculiarly. While ' for ft is rare, using " for inches is common in places where this is still in use.

Therefore, I think it is ok that this isn't fully working, however builds should not fail, regardless of current culture of the developer and the failure should be consistent and well defined. Your new PR is getting to that.