MangelMaxime/Thoth

Thoth.Json.Net Unit Tests fail on Windows

Closed this issue · 5 comments

When I run the unit tests four tests are failing:

  • All/Thoth.Json.Decode/Object primitives/field output an error when field is missing
  • All/Thoth.Json.Decode/Object primitives/index output an error if array is to small
  • All/Thoth.Json.Decode/Fancy decoding/andThen generate an error if an error occuered
  • All/Thoth.Json.Decode/Object primitives/at output an error if the path failed

After some digging I found the reason:
Newtonsoft.Json uses System.Environment.NewLine as the newline character, which is \r\n under Windows.

The unit tests are comparing the error messages and they aren't equal because of the differing newline characters.

expected:

Expecting an object with a field named `height` but instead got:\n{\n    \"name\": \"maxime\",\n    \"age\": 25\n}

actual:

Expecting an object with a field named `height` but instead got:\n{\r\n    \"name\": \"maxime\",\r\n    \"age\": 25\r\n}

I've tried to fix the error by replacing \n with System.Environment.NewLine but it works only partially because the first line gets replaced also:

expected:

Expecting an object with a field named `height` but instead got:\r\n{\r\n    \"name\": \"maxime\",\r\n    \"age\": 25\r\n}

actual:

Expecting an object with a field named `height` but instead got:\n{\r\n    \"name\": \"maxime\",\r\n    \"age\": 25\r\n}

The fix that works is by setting the newline character on the StringWriter:

module Helpers =

    let anyToString (token: JToken) : string =
        use stream = new StringWriter(NewLine = "\n")
        use jsonWriter = new JsonTextWriter(
                                 stream,
                                 Formatting = Formatting.Indented,
                                 Indentation = 4)

        token.WriteTo(jsonWriter)
        stream.ToString()

Now the unit tests are working:

expected:

Expecting an object with a field named `height` but instead got:\n{\n    \"name\": \"maxime\",\n    \"age\": 25\n}

actual:

Expecting an object with a field named `height` but instead got:\n{\n    \"name\": \"maxime\",\n    \"age\": 25\n}

I'm not sure replacing the standard behavior of the JSON writer is the best solution (even if it is only used for the error messages).
Another solution would be to trim all newline characters from the expected and actual result.

Yes I saw the failing test on windows when trying to add the CI and didn't found the solution to the problem :).

Replacing the standard behavior of the JSON writer is important to have the same result between Fable and .Net code. I don't want to write the same test twice in Thoth.Json

Same if users implement some Unit test for their project if we don't output the same error message then they would need to implement the same test twice.

I forgot to mention the other failing tests in Tests.Json.Encode ;)

  • All/Thoth.Json.Encode/Basic/using pretty space works
  • All/Thoth.Json.Encode/Basic/complexe structure works

Same problem: NewLine character.

At least they can be easily fixed by replacing \n with Environment.NewLine by appending .Replace("\n", System.Environment.NewLine) on the expected values.

If it's important to have the same result for both Fable and .NET and Fable decodes newlines with \n the .NET encoder should also encode newline characters with \n.

Then the fix is by specifying the newline character also on the encoder:

let encode (space: int) (token: JToken) : string =
    let format = if space = 0 then Formatting.None else Formatting.Indented
    use stream = new StringWriter(NewLine = "\n")
    use jsonWriter = new JsonTextWriter(
                            stream,
                            Formatting = format,
                            Indentation = space )

    token.WriteTo(jsonWriter)
    stream.ToString()

I am in favor of specifying the NewLine charactor to the StringWriter.

In term of user experience it's looks much nicer than asking them to use .Replace("\n", System.Environment.NewLine) in their tests.

What do you think @toburger ?

Yes, I think this is the right thing to do.