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 failing if current Culture setting does not use decimal point separator #792

Closed
jhm-ciberman opened this issue Mar 30, 2023 · 11 comments · Fixed by #837
Closed

Tests failing if current Culture setting does not use decimal point separator #792

jhm-ciberman opened this issue Mar 30, 2023 · 11 comments · Fixed by #837
Labels

Comments

@jhm-ciberman
Copy link

Describe the bug
I just cloned the repo for the first time to contribute, and tried to run the test suite and it fails. I don't know if I am missing some step or what could be happening.

To Reproduce

  • git clone git@github.com:aaubry/YamlDotNet.git && cd YamlDotNet
  • git submodule update --init
  • dotnet restore
  • dotnet test

dotnet test results

https://gist.github.com/jhm-ciberman/2dbfe53d6aa41e10e61e0dbb9fea4d5a

I pasted it in a gist because it was too long to include it in the issue. (65536+ chars)

Environment:

$ dotnet --version
7.0.104
$ git --version
git version 2.37.3.windows.1
@EdwardCooke
Copy link
Collaborator

Have you made any changes to the code? These tests run every time code is merged on both windows and Linux machines without any problems.

@jhm-ciberman
Copy link
Author

jhm-ciberman commented Mar 30, 2023

Nope. I cloned the repository at master as it is.

Reading the logs in detail it looks the failing test is the same, just repeated multiple times:

[xUnit.net 00:00:02.79]     ConformsWithYamlSpec(name: "YD5X", description: "Spec Example 2.5. Sequence of Sequences", inputFile: "C:\\repos\\YamlDotNet\\YamlDotNet.Test\\yaml-test-"..., outputFile: "C:\\repos\\YamlDotNet\\YamlDotNet.Test\\yaml-test-"..., error: False, quoting: True) [FAIL]
  Failed ConformsWithYamlSpec(name: "YD5X", description: "Spec Example 2.5. Sequence of Sequences", inputFile: "C:\\repos\\YamlDotNet\\YamlDotNet.Test\\yaml-test-"..., outputFile: "C:\\repos\\YamlDotNet\\YamlDotNet.Test\\yaml-test-"..., error: False, quoting: True) [20 ms]
  Error Message:
   Assert.Equal() Failure
                                     ↓ (pos 57)
Expected: ···cGwire\r\n  - 65\r\n  - 0.278\r\n- - Sammy Sosa\r\n  - 63\r\n  - 0.288\r···
Actual:   ···cGwire\r\n  - 65\r\n  - 278\r\n- - Sammy Sosa\r\n  - 63\r\n  - 288\r\n
                                     ↑ (pos 57)
  Stack Trace:
     at YamlDotNet.Test.Spec.SerializerSpecTests.ConformsWithYamlSpec(String name, String description, String inputFile, String outputFile, Boolean error, Boolean quoting) in C:\repos\YamlDotNet\YamlDotNet.Test\Spec\SerializerSpecTests.cs:line 119
   at InvokeStub_SerializerSpecTests.ConformsWithYamlSpec(Object, Object, IntPtr*)
   at System.Reflection.MethodInvoker.Invoke(Object obj, IntPtr* args, BindingFlags invokeAttr)

It looks a float parsing error I think. Could it be something related to the Culture of the system? I'm running Windows with a regional settings set to Argentina (My country). We use , (comma) as decimal separator instead of . decimal point.

image

@jhm-ciberman
Copy link
Author

jhm-ciberman commented Mar 30, 2023

I just reproduced. I changed my regional settings to English (United States) and run dotnet test and now it passes fine:

image

image

@jhm-ciberman jhm-ciberman changed the title Multiple tests failing in my machine Tests failing if current Culture setting not using decimal point separator Mar 30, 2023
@jhm-ciberman jhm-ciberman changed the title Tests failing if current Culture setting not using decimal point separator Tests failing if current Culture setting does not use decimal point separator Mar 30, 2023
@EdwardCooke
Copy link
Collaborator

EdwardCooke commented Mar 30, 2023

That was my next question after noticing the comma instead of a period in the exponential number of one of the failed tests. I think we can specify the culture at the process level when the test run starts so they can run in any country, maybe. It’s been a long time since I’ve had to deal with globalization and localization in any of my projects.

I know the en_US culture works. Not sure how that plays on Linux/windows in other cultures without it installed in the OS though.

I can make an attempt at testing it and try and find a workaround but it could be some time before I get a solution in place. If you would like to work on it let me know. Interesting problem.

@EdwardCooke
Copy link
Collaborator

Can you try adding this to the test csproj and see if the tests pass in your culture?

https://github.com/dotnet/runtime/blob/main/docs/design/features/globalization-invariant-mode.md#enabling-the-invariant-mode

@jhm-ciberman
Copy link
Author

jhm-ciberman commented Mar 30, 2023

It fails

CultureNotFoundException : Only the invariant culture is supported in globalization-invariant mode. See https://aka.ms/GlobalizationInvariantMode for more information.

[xUnit.net 00:00:01.55]     Given_Values_WithLocale_WriteYaml_ShouldReturn_Result(year: 2016, month: 12, day: 31, hour: 3, minute: 0, second: 0, kind: Utc, locale: "es-ES") [FAIL]
  Failed Given_Values_WithLocale_WriteYaml_ShouldReturn_Result(year: 2016, month: 12, day: 31, hour: 3, minute: 0, second: 0, kind: Utc, locale: "ko-KR") [2 ms]
  Error Message:
   System.Globalization.CultureNotFoundException : Only the invariant culture is supported in globalization-invariant mode. See https://aka.ms/GlobalizationInvariantMode for more information. (Parameter 'name')
ko-KR is an invalid culture identifier.
  Stack Trace:
     at System.Globalization.CultureInfo..ctor(String name, Boolean useUserOverride)
   at YamlDotNet.Test.Serialization.DateTimeConverterTests.Given_Values_WithLocale_WriteYaml_ShouldReturn_Result(Int32 year, Int32 month, Int32 day, Int32 hour, Int32 minute, Int32 second, DateTimeKind kind, String locale) in C:\repos\YamlDotNet\YamlDotNet.Test\Serialization\DateTimeConverterTests.cs:line 522
   at System.RuntimeMethodHandle.InvokeMethod(Object target, Void** arguments, Signature sig, Boolean isConstructor)
   at System.Reflection.MethodInvoker.Invoke(Object obj, IntPtr* args, BindingFlags invokeAttr)

But, I want to share some insights. The problem it seems are the tests that use object.ToString(). I tested this in a REPL console using my current culture (es-AR):

> var ic = System.Globalization.CultureInfo.InvariantCulture;

> (2.70).ToString()
"2,7"             // Uses comma
> (2.70).ToString(ic)
"2.7"             // Uses point
> var value = 2.7;

> value.ToString()
"2,7"            // Uses comma
> string.Format(ic, "{0}", value)
"2.7"            // Uses point

The test that fails are SerializerSpecTests.ConformsWithYamlSpec for float and double, and DeserializerTest.DeserializeScalarEdgeCases for multiple specs

The ConformsWithYamlSpec uses ToString inside the test so it could be the test itself that is culture dependent.

In the other hand, for DeserializeScalarEdgeCases I don't see it uses ToString in the test itself, but rather inside the serializer. Maybe it's the serializer that is culture sensitive, in that case, it could be a bug.

@EdwardCooke
Copy link
Collaborator

Ok. My next suggestion would be to set the globalization to en_US at the process level when the test runner starts

@jhm-ciberman
Copy link
Author

jhm-ciberman commented Mar 30, 2023

I got fixed DeserializeScalarEdgeCases. The fix is easy as use an invariant culture in the test:

  [Theory]
  [MemberData(nameof(DeserializeScalarEdgeCases_TestCases))]
  public void DeserializeScalarEdgeCases(IConvertible value, Type type)
  {
      var deserializer = new DeserializerBuilder().Build();
+     var stringValue = string.Format(CultureInfo.InvariantCulture, "{0}", value);
-     var result = deserializer.Deserialize(value.ToString(), type);
+     var result = deserializer.Deserialize(stringValue, type);
  
      result.Should().Be(value);
  }

I will keep searching the root cause for ConformsWithYamlSpec

@jhm-ciberman
Copy link
Author

I discovered the bug. In was a bug with the ScalarNodeDeserializer. So I think it's an important bug. I will make a PR with both fixes.

jhm-ciberman added a commit to jhm-ciberman/YamlDotNet that referenced this issue Mar 30, 2023
Adds `CultureInfo.InvariantCulture` in parsing method in
`ScalarNodeDeserializer.AttemptUnknownTypeDeserialization`.
This caused tests to fail in non english culture (Tested with "es-AR")
jhm-ciberman added a commit to jhm-ciberman/YamlDotNet that referenced this issue Mar 30, 2023
Adds `YamlFormatter.NumberFormat` in `Parse` methods in
`ScalarNodeDeserializer.AttemptUnknownTypeDeserialization`.
This caused tests to fail in non english culture (Tested with "es-AR")
@jhm-ciberman
Copy link
Author

@EdwardCooke I opened #793 and #794 to address both problems.

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

Successfully merging a pull request may close this issue.

3 participants
@jhm-ciberman @EdwardCooke and others