Skip to content
This repository has been archived by the owner on Oct 9, 2019. It is now read-only.

Expectation formatting makes it hard to deal with binary data #226

Closed
rhofour opened this issue Dec 25, 2017 · 6 comments
Closed

Expectation formatting makes it hard to deal with binary data #226

rhofour opened this issue Dec 25, 2017 · 6 comments

Comments

@rhofour
Copy link

rhofour commented Dec 25, 2017

Since Elm doesn't have a distinct binary string type, binary data often ends up being represented as a string such as in elm-comidi (an Elm MIDI parser). I'm currently writing tests for elm-comidi which end up spitting out binary to my terminal when they fail.

For example:

Given ({ formatType = 0, trackCount = 1, ticksPerBeat = 1 },[[]])                                                                                                                                                    

    Err "parse error: [\"midi tracks (1)\"], { data = \"MThd\\0\\0\\0\\0\\0\\0\\0MTrk\\0\\0\\0\\0ÿ/\\0\", input = \"ÿ/\\0\", position = 23 }"
    ╷
    │ Expect.equal
    ╵
    Ok ({ formatType = 0, trackCount = 1, ticksPerBeat = 1 },[[]]) 

It's difficult to figure out what's going on here for a couple reasons:

  1. As mentioned in Non-printable characters aren't escaped #217 there's the possibility this includes non-printable characters (like '\x01') which would be impossible to see here.
  2. Since all of the code deals with this as binary I have to manually convert characters like 'ÿ' back to binary to make sense of them.

I imagine the best solution here would be for Elm to have better binary support, but a much easier to implement solution would be #224.

An ugly hack that's almost as good would be to just run my custom formatting code on both the actual and expected values so elm-test is only comparing strings rather than Elm types.

@drathier
Copy link
Collaborator

I would recommend defining a function serialize : Midi -> String which pretty-prints the binary data in a human readable format. Then you can use serialize a |> Expect.equal (serialize b).

It's very hard for us to handle binary data in a general way; you know much more about your binary data than we do. #225 might help a bit.

@rtfeldman
Copy link
Member

I imagine the best solution here would be for Elm to have better binary support

I agree that this is the best solution, and I think we should choose the best solution!

but a much easier to implement solution

Another way of saying this is: Elm doesn't currently support binary data, but in the meantime we could make elm-test worse.

I don't think we should do that. 😅

It seems like the root of the problem is that you're using a string to represent arbitrary binary data. Is this the best format in which to represent it in Elm? For example, what if you base64 encoded it instead? Or used a union type?

@rhofour
Copy link
Author

rhofour commented Dec 26, 2017

@drathier:

I would recommend defining a function serialize : Midi -> String which pretty-prints the binary data in a human readable format. Then you can use serialize a |> Expect.equal (serialize b).

That's what I proposed as a hack that could work. It has the unfortunate side-effect that the serializing has to happen twice for every test even if it passes, but it should work. It also then can introduce false negatives if I don't include enough information in my serializer's output, but I don't think that's a serious concern.

@rtfeldman:

I agree that this is the best solution, and I think we should choose the best solution!

I got the impression this wasn't a priority for Evan and I wasn't sure there was much I could do, but I'd love to hear I'm mistaken there. What I don't want to do is just sit on my hands and wait for someone else to fix this.

It seems like the root of the problem is that you're using a string to represent arbitrary binary data. Is this the best format in which to represent it in Elm?

I believe it is with the current state of the world. It's the type the data has as it comes in from the JS side and the type the parser library works with. A List of Ints is the only option I can see that could really be comparable (and that has some downsides).

what if you base64 encoded it instead?

Then it makes the parser code harder to read since it's dealing with the base64 bytes rather than the real ones. This also still leaves the test output hard to read. It's a step above trying to read unprintable characters right now, but it would still require the base64 values to be translated back to binary to make sense of them.

Or used a union type?

This is for a parsing library so the end goal is to make a union type, but first I need to test the parser that does that. I want to get away from the binary ASAP, but someone has to deal with it.

Or are you suggesting a union type for binary data like: type Byte = B0 | B1 | B2 | ... | B255?

@rtfeldman
Copy link
Member

rtfeldman commented Dec 26, 2017

I would recommend defining a function serialize : Midi -> String which pretty-prints the binary data in a human readable format. Then you can use serialize a |> Expect.equal (serialize b).

That's what I proposed as a hack that could work. It has the unfortunate side-effect that the serializing has to happen twice for every test even if it passes, but it should work. It also then can introduce false negatives

Oh! If those are the only drawbacks to that approach, you can do this:

expectMidiEqual : Midi -> Midi -> Expectation
expectMidiEqual midi1 midi2 =
    if midi1 == midi2 then
        Expect.pass
    else
        Expect.equal (serialize midi1) (serialize midi2)

This way it only does the serialization work if the comparison fails.

If you're worried about false negatives, you could also explicitly guard against them:

expectMidiEqual : Midi -> Midi -> Expectation
expectMidiEqual midi1 midi2 =
    if midi1 == midi2 then
        Expect.pass
    else
        let
            expectation =
                Expect.equal (serialize midi1) (serialize midi2)
        in
            if expectation == Expect.pass then
                -- Oops! False negative. Instead of changing the outcome,
                -- Fall back on `Expect.equal` on the unserialized inputs
                Expect.equal midi1 midi2
            else
                expectation

@rhofour
Copy link
Author

rhofour commented Dec 26, 2017

Huh, that never occurred to me, but it looks like just what I need, thanks!

@rhofour rhofour closed this as completed Dec 26, 2017
@rtfeldman
Copy link
Member

Happy to help! Thanks for being patient through all the discussion. 😄

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

No branches or pull requests

3 participants