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

Handle unicode and e-notation differences between S.T.J and J.N tests #35042

Merged
merged 10 commits into from
Nov 17, 2020

Conversation

marcusturewicz
Copy link
Contributor

Fixes #32350

@ghost
Copy link

ghost commented Apr 16, 2020

Tagging subscribers to this area: @jozkee
Notify danmosemsft if you want to be subscribed.

Copy link
Member

@jozkee jozkee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would suggest to go for replacing the comparison against Json.Net with hardcoded strings as suggested in the issues description of #32350 unless there is a Json.Net configuration that allows you to get an output similar to System.Text.Json.

@jozkee
Copy link
Member

jozkee commented May 5, 2020

CI errors show that precision between Json.net and System.Text.Json also changes in some cases, any idea why is that?

    System.Text.Json.Tests.Utf8JsonWriterTests.WriteNumbers(formatted: True, skipValidation: True, keyString: "message") [FAIL]
      Assert.Equal() Failure
                                         � (pos 99)
      Expected: תתת\r\n  "message": 123.45,\r\n  "message": -1234.5,\r\n  "message": 5תתת
      Actual:   תתת\r\n  "message": 123.449997,\r\n  "message": -1234.5,\r\n  "messageתתת
                                         � (pos 99)
      Stack Trace:
        /_/src/libraries/System.Text.Json/tests/JsonTestHelper.cs(746,0): at System.Text.Json.JsonTestHelper.AssertContentsAgainstJsonNet(String expectedValue, String value)
        /_/src/libraries/System.Text.Json/tests/JsonTestHelper.cs(722,0): at System.Text.Json.JsonTestHelper.AssertContents(String expectedValue, ArrayBufferWriter`1 buffer)
        /_/src/libraries/System.Text.Json/tests/Utf8JsonWriterTests.cs(5557,0): at System.Text.Json.Tests.Utf8JsonWriterTests.WriteNumbers(Boolean formatted, Boolean skipValidation, String keyString)

@layomia layomia added this to the 5.0 milestone May 15, 2020
@marcusturewicz
Copy link
Contributor Author

Can't seem to see why the CI is failing, just says Helix things. Anyone able to help?

Copy link
Member

@jozkee jozkee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left some suggestions to consider.
Once we manage to enable this test, it will start to fail in .NET framework because of precision discrepancies on floating point values (see #435 (comment)). e.g: JsonSerializer.Serialize(123.45f) will produce 123.45 on .NET 5 but it will produce 123.449997 on .NET Framework.

At that point, I'm not sure if we should ifdef the floating point values to only apply for .NET 5.0 or disable the whole test from running on netfx.

@jozkee jozkee modified the milestones: 5.0.0, 6.0.0 Sep 14, 2020
@stephentoub
Copy link
Member

@marcusturewicz, are you still working on this? Thanks.

@marcusturewicz
Copy link
Contributor Author

@marcusturewicz, are you still working on this? Thanks.

I'll have one more crack based on feedback above.

@jozkee
Copy link
Member

jozkee commented Nov 16, 2020

@marcusturewicz can you please take a look at jozkee@5ef3af3? I think said change should suffice to finish up this PR.

Copy link
Member

@jozkee jozkee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure why the CI is failing right now. Can you try to rebase on top of master to be as up-to-date as possible?

Copy link
Member

@jozkee jozkee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jozkee jozkee merged commit 9d60485 into dotnet:master Nov 17, 2020
@jozkee
Copy link
Member

jozkee commented Nov 17, 2020

@marcusturewicz, thanks!

@marcusturewicz marcusturewicz deleted the issue-32350 branch November 17, 2020 22:17
@steveharter
Copy link
Member

Once we manage to enable this test, it will start to fail in .NET framework because of precision discrepancies on floating point values (see #435 (comment)). e.g: JsonSerializer.Serialize(123.45f) will produce 123.45 on .NET 5 but it will produce 123.449997 on .NET Framework.
At that point, I'm not sure if we should ifdef the floating point values to only apply for .NET 5.0 or disable the whole test from running on netfx.

I also ran into this also. It looks like the reader uses a higher-precision "G17" format which causes this issue on .NET Framework AFAIK. Here's my specific test:
https://github.com/dotnet/runtime/blob/master/src/libraries/System.Text.Json/tests/Serialization/CustomConverterTests/CustomConverterTests.Dynamic.Sample.Tests.cs#L62-L66

@ghost ghost locked as resolved and limited conversation to collaborators Dec 19, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
6 participants