Fix unit tests to run on non-english devices

[rudyhuyn]

Fixes #272

Description of the changes:

• add a parameter to the CurrencyDataLoader constructor to override the locale
• add a method LocalizationService::OverrideWithLanguage to force LocalizationService to override the current locale (some static methods were converted to const instance methods in order to take into account the new locale)
• Use them to force unit tests to run with en-US

How changes were validated:

[HowardWolosky]

Locking the PR since the associated Issue isn't approved yet. That way conversation on the problem being addressed is discussed in the Issue, so that future PR discussion can be limited to the implementation of the fix.

[mcooley]

Oops, looks like the issue was approved but the PR was left locked. Sorry about that.

[sanderl]

Hi Rudy, I looked at your change and it looks good to me. I think the test hook works well to allow the tests to run on other languages. We just need this branch to be updated to take in the latest changes from master. Thanks!

[sanderl]

Please update the branch with the latest from master.

[rudyhuyn]

Please update the branch with the latest from master.

I will tonight!

[sanderl]

The CI build is failing on release builds. I was able to repro this locally as well. I did see that it is not failing on debug. It looks like it is related to the LocalizationService constructor. This thread might be relevant: https://social.msdn.microsoft.com/Forums/vstudio/en-US/7e5773ae-018d-4fb9-8a15-6edef9ce41be/weird-error-on-release-build.

It is interesting that we are hitting an internal compiler error, we are following up with the compiler team.

[rudyhuyn]

I can update the diff and add an explicit parameter-less constructor if you want

[sanderl]

I can update the diff and add an explicit parameter-less constructor if you want

I tried adding the default constructor explicitly on my local build and there were more build issues related to the LocalizationService constructor (I don't have access to my build right now to see exactly what the errors are). I haven't had a chance to work through them.

Daniel Belcher recommended trying to turn off optimization for that section of code. I haven't tried it, so I don't know if it fixes the problem.

I already sent the email to the compiler team, so I am hoping to hear something soon.

[rudyhuyn]

I would be very interested to know the reason of this issue!

recommended trying to turn off optimization for that section of code. I haven't tried it, so I don't know if it fixes the problem.

I would prefer to keep the optimization but instead refactor a little the code. Because the constructor is private, we can remove the default argument for overridedLanguage and explicitly pass nullptr in the code.

Tested in Release mode, including unit tests

[sanderl]

I would be very interested to know the reason of this issue!

The response I got from my email is that changing the constructor to have the optional parameter is not an ABI compatible change. The issue is that existing code referencing the 0 parameter constructor will not work. He said that default arguments work by copying the default argument to each call site, so any existing call sites built on old metadata or old headers will not work.

[rudyhuyn]

I would be very interested to know the reason of this issue!

The response I got from my email is that changing the constructor to have the optional parameter is not an ABI compatible change. The issue is that existing code referencing the 0 parameter constructor will not work. He said that default arguments work by copying the default argument to each call site, so any existing call sites built on old metadata or old headers will not work.

Sorry, my question was about "I tried adding the default constructor explicitly on my local build and there were more build issues related to the LocalizationService constructor". I tried myself and faced the same errors than you. It's pretty clear that ABI will have an issue with default parameters, but it doesn't really explain why we weren't able to use 2 constructors

[sanderl]

I would be very interested to know the reason of this issue!

The response I got from my email is that changing the constructor to have the optional parameter is not an ABI compatible change. The issue is that existing code referencing the 0 parameter constructor will not work. He said that default arguments work by copying the default argument to each call site, so any existing call sites built on old metadata or old headers will not work.

Sorry, my question was about "I tried adding the default constructor explicitly on my local build and there were more build issues related to the LocalizationService constructor". I tried myself and faced the same errors than you. It's pretty clear that ABI will have an issue with default parameters, but it doesn't really explain why we weren't able to use 2 constructors

I was just told there was probably something with the metadata.