search

tdwright / contabs

Simple yet flexible tables for console apps.
MIT License
54 stars 20 forks source link

Unit tests failing on locales with different decimal separator #55

Closed zumo-sk closed 6 years ago

zumo-sk commented 6 years ago

20 unit tests are failing when using comma as a decimal separator in system locale, for example test CurrencyFieldCanBeFormatted fails with:

val should be "£1.91" but was "£1,91"

The tests should be rewritten to be compatible with different locales (for example, using CurrentCulture.NumberFormat.NumberDecimalSeparator, or switching to different locale before testing), or columns should have support for CultureInfo in addition to string formatting.

I'd be happy to help with this issue if you decide which way you would like to have this fixed.

tdwright commented 6 years ago

Hi @zumo-sk,

Good spot and thanks for reporting it.

I'd probably suggest doing both:

  1. Fix the test in the short term
  2. Add functionality to control globalisation settings

There's a lot to think about for the second, so how about we spin that out into a new issue?

Cheers, Tom

zumo-sk commented 6 years ago

Hi Tom,

Thanks for a prompt reply. I am totally new in the open source community and I joined it to get some hands on in C#, but I'd like to try to fix the tests, if it's OK for you. Maybe even add some extra ones related to locales. I agree that the feature to have possibility to specify globalization settings could be a new ticket. Will you create it? Or should I?

Marek

On Mon, Oct 22, 2018 at 3:18 AM Tom Wright notifications@github.com wrote:

Hi @zumo-sk https://github.com/zumo-sk,

Good spot and thanks for reporting it.

I'd probably suggest doing both:

  1. Fix the test in the short term
  2. Add functionality to control globalisation settings

There's a lot to think about for the second, so how about we spin that out into a new issue?

Cheers, Tom

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/tdwright/contabs/issues/55#issuecomment-431721540, or mute the thread https://github.com/notifications/unsubscribe-auth/AEAbguGDFRN_Sq_GVNMwu6Ebr1V93r0aks5unRzcgaJpZM4XyW5g .

tdwright commented 6 years ago

Hi Marek,

No worries at all. Although you picked quite the day for your first contribution! :wink:

Thanks for the the PR (#56) - I'll review it properly when I get a chunk of free time. Hopefully one evening this week. From what I've seen though, it appears to be fine and a helpful addition to the test suite.

Apologies for my automatic anglocentrism. I should have considered localisation properly. On that note, are there possibly other issues that could cause tests to fail in some locales?

Oh, and I'll open a discussion ticket for a table localisation feature right away.

Cheers, Tom

zumo-sk commented 6 years ago

I had nothing to do with the outage! I promise! 😅 I saw date formatting there, too - but that one seemed to work fine, on a first glance. But I guess it won't hurt to have tests for them. Maybe wait with review and I'll try to make tests for them later this week. I have no idea what is the correct procedure in this case (pulling back the pull request?), so I am just writing it to the ticket. 😛

zumo-sk commented 6 years ago

As promised, added few tests for date formats. I decided not to use locales, just different date formats, to test that the correct string conversion method is being called and that you can choose the formatting as you wish.

tdwright commented 6 years ago

Hi Marek,

What you did was absolutely fine. Pushing new changes to an existing PR is the correct procedure. To be honest, the main thing is that your changes are communicated clearly and effectively, so a big 👍.

The new tests look helpful, but I'll have a proper look when I get the chance.

Thanks for your contributions so far!

Tom

zumo-sk commented 6 years ago

Hi Tom, I wanted to take a look on other features and I checked the sources on another computer and I found out one of the tests was failing there, too - because of it seems that different versions of .NET framework or .NET core may have different decimal separators for different languages (that's my best guess), And one of the locales (SK) I used for date has "." as a decimal separator in one version and ". " (so dot and a space) in another. The documentation to locale settings is incredibly poor, so my question is, how should I address it? My suggestions:

  1. Using CZ instead of SK that also uses ".", but presumably without space on both computers I'm using.
  2. Change the test not to use locales, but hard-coded values.
tdwright commented 6 years ago

Hi Marek,

I noticed the same thing yesterday and posted a question on Stackoverflow.

It seems as though locales aren't as static or reliable as we'd both implicitly assumed. My suggestion would be to:

  1. Leave the test as it currently is, but limit the locales tested to those that are well established and widely known. en-GB and en-US would qualify, but I think we could find others too. (No disrespect to my Slovak friends, you understand...)

  2. In general, we just want to check that locales are being applied, not exactly what happens when they're applied. So for a longer list of locales we could compare the output with the output of a straight DateTime.ToString(locale). This was we avoid having to hard-code any values, or make any assumptions, whilst retaining the assurance that locales are being respected.

What do you think?

Tom

tdwright commented 6 years ago

With the merging of #59, I think we can confidently close this one now. 🎉

Thanks for your help with this one @zumo-sk. I hope your first open source contribution wasn't too traumatic! 😖