Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tests with numeric strings fail under non-en-US culture #1520

Closed
amis92 opened this issue Apr 25, 2018 · 5 comments
Closed

Tests with numeric strings fail under non-en-US culture #1520

amis92 opened this issue Apr 25, 2018 · 5 comments

Comments

@amis92
Copy link
Contributor

amis92 commented Apr 25, 2018

There is an issue with three tests in Avalonia.Markup.UnitTests.Data:

  • Avalonia.Markup.UnitTests.Data.BindingExpressionTests.Should_Convert_Get_String_To_Double
  • Avalonia.Markup.UnitTests.Data.BindingExpressionTests.Should_Convert_Set_Double_To_String
  • Avalonia.Markup.UnitTests.Data.BindingExpressionTests.Should_Handle_DataValidation
  • Avalonia.Visuals.UnitTests.Media.MatrixTests.Parse_Parses
  • Avalonia.Visuals.UnitTests.Media.RectTests.Parse_Parses

When run under a culture that has another decimal separator than en-US full stop . (e.g. comma , in fr-FR), these tests fail as the conversion from/to string of these numeric values fails.

These tests surface a general problem: XAML type conversion is done with CurrentCulture which isn't explicitly set. I'm not exactly sure how it should behave, but I suspect that XAML parsing should be done explicitly in InvariantCulture (i.e. decimal separator is dot/full stop .).

@Gillibald
Copy link
Contributor

My current PR already has the fix to this problem. We just have to use ToString(CultureInfo.CurrentCulture) for all values we compare to. I can create a separate PR for this to have it fixed faster.

Xaml conversion always justifies current culture and that's fine.

@amis92
Copy link
Contributor Author

amis92 commented Apr 26, 2018

First off, for your changes it'd be much nicer to write $"{12.3}" (an approach used in xUnit tests) instead of ToString with culture.

Also, I think it'd be better off in it's own PR. These fixes are only valid for BindingExpressionTests, though.

With Rect and Matrix, it's not that simple, because they perform string.Split(new[]{',',' '}) which simply invalidates any culture-specific parsing later(edit: no longer, they use StringTokenizer). You'd have to either only use spaces as number separators, or use one depending on CurrentCulture, mirroring behavior of String Tokenizer.

They also differ in performing ToString: Rect uses InvariantCulture, Matrix CurrentCulture.

@Gillibald
Copy link
Contributor

$"{12.3}" is just a shortcut to string.Format("{0}",12.3) which uses CurrentCulture by default. Don't know if that's allowed by code style conventions of this project.

Maybe you are right that numeric values should be parsed with invariant culture. That way everything should just work.

@Gillibald
Copy link
Contributor

This can be closed

@grokys grokys closed this as completed May 3, 2018
@amis92
Copy link
Contributor Author

amis92 commented May 4, 2018

For the sake of reference, the Matrix/Rect cases were fixed by #1539

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants