GetNumberGroupSizes test fails on OSX Yosemite.

[eerhardt]

The culture data for ur-IN is different on OSX El Capitan than on Yosemite. Thus we have a failing test that is expecting the wrong data.

This fix gets the current OSX Kernel version by P/Invoking sysctlbyname("kern.osrelease") and compares it to 15.0 (El Capitan).

Fix #4667

@tarekgh @stephentoub @mellinoe

[tarekgh]

LGTM.

I think in the future we need to change such tests to read the underlying OS data and compare against it. I know this is not trivial but I think doing that will not require us to change the test when the data change.

[stephentoub]

The lengths are size_t, so would oldpLen and newpLen be better as ref IntPtr and IntPtr? And should this be SetLastError = true as well? Separately we should think through how we want to handle this kind of P/Invoking in tests.

[stephentoub]

Also, I wonder if we're going to run into this in other places, and if so whether we should centralize this check somewhere to make it available to other tests in the future.

It'd also be nice if we could avoid needing such a P/Invoke in the tests, and instead have such version information exposed from RuntimeInformation, such that tests like this could then just use it. cc: @Priya91

[eerhardt]

The lengths are size_t, so would oldpLen and newpLen be better as ref IntPtr and IntPtr?

Since this code is OSX specific, I thought the ABI was already constant? Which is why we don't need shims for OSX specific native calls in the product.

And should this be SetLastError = true as well?

Added.

Also, I wonder if we're going to run into this in other places, and if so whether we should centralize this check somewhere to make it available to other tests in the future.

I like using the rule "2nd time pays for generality". So if/when we need it somewhere else, we can refactor it there at that time.

It'd also be nice if we could avoid needing such a P/Invoke in the tests, and instead have such version information exposed from RuntimeInformation

#4334 adds a RuntimeInformation.OSDescription string, which will look something like Darwin Kernel Version 15.0.0: Sat Sep 19 15:53:46, which could be parsed by this test (once the API is added). However, I didn't think it would be good to rely on the structure of this string.

[stephentoub]

However, I didn't think it would be good to rely on the structure of this string.

@weshaggard, without exposing version information, how are developers supposed to do this kind of thing? I keep hearing "they should instead use feature detection"... how does that work in a case like this?

[tarekgh]

I want to emphasize for the globalization in general, the developers shouldn't assume any specific data and should behave according to what we/OS provide. so in real scenarios they shouldn't need to check the OS version. I can understand we add this in our testing to validate what we are reading but I have suggested in the future we shouldn't write the test like that and instead the test should read the OS data and validate it against what the framework is providing.

[stephentoub]

by the way I wrote a test on Windows which do what I am saying and covering much more areas and was reliable when running on different Windows versions.

Reliable, sure. Accurate in detecting failure, who knows 😄

Anyway, I've made my points, and I know you understand them. It seems we just fundamentally disagree.

I believe we need to expose OS version information. We're not doing so out of fear that developers may misuse it. So be it. If we don't provide it, developers are going to find worse, less reliable, less efficient ways to get at it, making the end result worse than if we'd just provided it in the first place.

[eatdrinksleepcode]

If I may interject...

I agree with @stephentoub: doing the same thing in the tests that you do in the code and then asserting that you get the same result is not a particularly valuable test. However, @tarekgh's point about the underlying data changing is also well taken. Choosing between "the lesser of two evils" seems to be the only choice in the current setup.

May I suggest that the reason for this difficulty is that these unit tests have a dependency on an external system over which the tests have no control: namely, the underlying OS. This results in a situation where the test does not control its input data, which is counter to the usual goals for unit tests (well-known inputs produce well-known outputs). The typical way to achieve this goal is to stub the external dependency to use well-known data; in this case, stub the underlying system calls to return representative data sets.

The downside of using such stubs is that the tests now only provide confidence that the code works given the well-known input data in the tests; there needs to be a corresponding confidence that the OS will actually supply data like what is in the tests in order to complete the chain. This can be accomplished using a form of contract test: a test that is specifically designed to validate that the external dependency returns the data (the "contract") that you expect in your unit tests. The key is that because these contract tests will only fail when the external dependency changes, and the external dependency in this case is the OS which will not change very often in CI, they don't need to run in CI. They only really need to be run any time the external dependency changes (which in the case of CoreFX would be when a new OS or OS version is added to the supported list).

This makes the tests much more valuable because it means that we can essentially test all of the expected scenarios from all OSs with normal CI and local developer builds. Having CI for all possible OSs is not necessary, and a local developer doesn't have to wait for a full CI run to know if he has broken a scenario from an OS other than what is available to him.

Contract tests also address another point that @stephentoub brought up:

how do we write the regression test for the reported failure?

We don't need to write a regression test if we fixed the issue.

If I understand correctly, inherent in @tarekgh's response is that you will know if you fixed the issue by running the tests on the OS on which the issue was found. By using contract tests, you can instead add a new contract to the unit tests that represents that OS, then fix the issue, which will cause that test to pass, all on your local machine, regardless of its OS. The only thing you need the OS in question for is to establish the contract.

The downside to this approach is that running unit tests alone on an OS is not sufficient to determine whether that OS is fully supported by the framework. But that is simple to overcome: simply run the contract tests as well. In this way, the activities of everyday development are separated from the activity of supporting a new OS (or OS version), which IMHO is a good thing.

Stubbing the underlying OS is probably not a common approach; I don't know if there are any tests currently in CoreFX that take this approach. But for a framework with cross-platform ambitions like CoreFX, I think it is an approach worth considering.

[weshaggard]

Thanks for the feedback @eatdrinksleepcode it is a very insightful approach. I'm not sure how difficult it will be for that level of abstraction in the culture data but it would certainly be something we should consider, as I agree that both of the existing approaches aren't ideal. That said given the options of checking the OS version verse just asking the OS for the data I prefer the latter, if anything. I have similar concerns with tests that check the resource strings as they are very fragile in general and essentially hard coding the strings and expect them to never change or be localized.

@stephentoub please decouple this issue from providing an API for getting the OS version. We have had that conversation numerous times but if you want to continue that discussion please file a separate issue explicitly for that.

[stephentoub]

please decouple this issue

https://github.com/dotnet/corefx/issues/4741

[eerhardt]

I know we've beaten this issue over and over on this thread, but I just wanted to respond to a few points.

"just asking the OS for the data"

It isn't that simple. On Windows it might be that simple to just call a native method to get the culture data, but on Unix we are using ICU, and then doing our own logic on top of it where necessary. One such example is https://github.com/dotnet/coreclr/blob/master/src/corefx/System.Globalization.Native/localeNumberData.cpp#L258 where we turn an ICU string number pattern into one of our own enum values. This parsing logic isn't simple, and to @stephentoub's point, if the test had to do the same thing, we wouldn't really be testing anything. Note there are other cases like this, just poke around under coreclr/src/corefx/System.Globalization.Native to see them.

Building on this point, @eatdrinksleepcode, you bring up a really good idea and is the real way to solve this issue IMO. However, it would be a lot of work to get some sort of stub in place. A good chunk of the code we are testing here is native code built around ICU. We would need to inject the stub lower than that native code in order to keep it under test. Some day we might be able to build something like this, but I just don't think we have the time/resources right now.

[stephentoub]

A few comments, but otherwise LGTM. Thanks.

[justinvp]

Should these be readonly?

[stephentoub]

Since there are only nits (from me and Justin) and I'd like to try to get the OS X CI run unblocked, I'm going to merge this, and the nits can be addressed subsequently.