elm-community / elm-test

moved to elm-explorations/test
https://github.com/elm-explorations/test
BSD 3-Clause "New" or "Revised" License
339 stars 35 forks source link

Idea: almostEqual expectation && Expect.custom #41

Closed mgold closed 7 years ago

mgold commented 8 years ago

From John Watson on the mailing list comes this idea:

almostEqual : Float -> Float -> Expectation

It would be pretty simple to add if we're doing a minor release.

robertjlooby commented 8 years ago

You'd probably want to be able to supply the delta right? So

almostEqual : Float -> Float -> Float -> Expectation
newlandsvalley commented 8 years ago

Yeah - I think that's a good idea.

eeue56 commented 8 years ago

Without the delta this isn't a good idea :)

mgold commented 8 years ago

I was hoping that we could choose a reasonable delta and free people from worrying about it, but apparently folks disagree.

With a delta, you could use this for within a range, not just floating point precision.

newlandsvalley commented 8 years ago

I can imagine situations where you're generating floats - 10^n > f > 10^-n for large values of n where you'd prefer a default delta (based on the number of significant figures) to an explicit delta.

mgold commented 8 years ago

It's possible for this to be written by a third party using 'Expect.passandExpect.fail`. Let's see how this works in practice and if the community converges on a particular implementation.

newlandsvalley commented 8 years ago

Yes, I've already implemented an arbitrary and very crude version here which I don't really like. Given that you have to check that the target is between two expected values, I wonder if an implementation might benefit if there existed an and combinator for Expectations?

mgold commented 8 years ago

The idea of an all function that takes a list has been brought up before. My concern is that the error message will be the first expectation to fail, and that won't clearly show the intent of the test.

I think Expectations should be thought of like Cmds: they don't compose. Instead you should rely on general-purpose logic. My current best idea for this problem is:

Expect.custom : (a -> b -> Result String ()) -> a -> b -> Expectation

nearlyEqual : { tolerance : Float } -> Expectation
nearlyEqual {tolerance} =
  Expect.custom (\i j ->
    if abs (i - j) < tolerance then
      Ok ()
    else
      Err <| "Expect to be nearly equal, tolerance: " ++ toString tolerance
  )

You can imagine how this would expand to your more sophisticated implementation. On failure (say of 3 and 2) it would produce

3
|
| Expect to be nearly equal, tolerance: 0.001
|
2

so just like the built-in expectations.

newlandsvalley commented 8 years ago

I see what you mean. One of the reasons my implementation is so clunky is that I have no access to the Expectation constructors, nor this magic custom function which has this knowledge and looks very useful. I think if it were eventually made available, I'd like to alter my implementation along the lines you suggest.

Janiczek commented 8 years ago

The nearlyEqual type will probably be

nearlyEqual : { tolerance : number } -> number -> number -> Expectation
nearlyEqual {tolerance} =

I support the Expect.custom idea. Would really free people to create their own, nice-looking, expectations. (read: I've been blocked by this)

What about the difference between {tolerance : number} and number? Is explicit better here? (I think it is - it's obvious which argument is delta and which are the numbers)

Janiczek commented 8 years ago

Another thought is... is replacing Expectation with Result good idea for this? If user returned Expectation, the custom function would probably just "augument" the error message with the vertical bars and whatnot?

mgold commented 8 years ago

The nearlyEqual type will probably be

Oops, yes your type signature is correct and mine is not. Example usage: someComplicated |> expression |> nearlyEqual {tolerance = 0.001} expectedValue

Wrapping tolerance in a record allows us to give it a name, similar to fuzzWith {runs = 1000}. The record pattern extends well to adding more arguments.

just "augument" the error message with the vertical bars

I don't think there's an easy way to get the error message without the vertical bars out the Expectation. (It's possible, but not easy.)

In order to create the vertical bar, we need the expected, the actual, the error in the middle, and a decision-making function. One advantage custom has over some other proposals I've seen is that the function can provide a custom error on each failure, based on the expected and actual.

Here's another example of an expectation built from custom:

expectContains : String -> String -> Expectation
expectContains substring str =
  if String.contains substring str then
    Ok ()
  else
    Err "Should contain the string"

"hello findMe goodbye" |> expectContains "findMe"

But, if you want to use expectContains or nearlyEqual (we need more consistent names for these) in another custom expectation, you're out of luck. So I'm not quite happy with this solution.

That said, I'd like to see what people make of it and get some experience reports. So, here's an implementation of custom based on already-exposed functions. It's somewhat hacky in that it duplicates the vertical bar logic (which isn't exposed and might change one day) but it's enough to play around with.

custom : (a -> b -> Result String ()) -> a -> b -> Expectation
custom f expected actual =
    case f expected actual of
        Ok () ->
            Expect.pass

        Err str ->
            [ toString actual
            , "╷"
            , "│ " ++ str
            , "╵"
            , toString expected
            ]
                |> String.join "\n"
                |> Expect.fail
newlandsvalley commented 8 years ago

Thanks very much for this. I'm now using it - it works very well. (You have to be careful when working with floats also to cater for infinite values).

mgold commented 8 years ago

Do you have a program that could plausibly produce floating point infinity? I've typically ignored it, and Fuzz.float won't produce it.

newlandsvalley commented 8 years ago

Yes - strangely I've been working on a fork of imeckler/Ratio. He allows Rationals to be created that are effectively infinity:

Rational 1 0

and my arithmetic tests compare on one side converting the operands to floats and on the other converting the result to a float. I was worried about this Rational behaviour initially (thinking it to be irrational!) but it seems to work out. Division by infinity gives zero (Rational 0 1).

I guess that this is a real edge case though. I don't need the Fuzzer to produce it.

Janiczek commented 8 years ago

@mgold, I'll try and use this version tonight. Thanks for your effort!

As for floats, wouldn't it (having all the Infinities and NaNs produced by the fuzzer as edge cases) make the code more robust as a result?

(I imagine it would make me think about all the edge cases, the same way as I have to handle all Maybe and Result cases)

mgold commented 8 years ago

As for floats, wouldn't it (having all the Infinities and NaNs produced by the fuzzer as edge cases) make the code more robust as a result?

Yeah, you're not wrong. Something similar can be said for floats of magnitude < 0.000001, which JS will write in scientific notation.

But, I think that writing tests that handle these would quickly bog down into "the thing I expect... or NaN or Infinity", which isn't useful. Once Fuzz.constant lands in 2.1.0, you'll be able to use that and Fuzz.frequency to add some NaNs and Infinities to your float mix. If you have more to say on this please open a new issue.

Janiczek commented 8 years ago

I have tried the custom defined a few posts above and it's great:

    -605028.962967936
    ╷
    │ Expect.nearlyEqual (tolerance 1e-10)
    ╵
    -605028.9629679358
rtfeldman commented 8 years ago

How do people feel about within over tolerance? I like how it reads.

eeue56 commented 8 years ago

:tada: for within, ❤️ for tolerance

mgold commented 8 years ago

I'm pretty sure within and tolerance are mutually exclusive options... unless Noah means Expect.within : { tolerance : Float } -> Float -> Float -> Expectation? Not sure I like that...

BTW there are some draft implementations (not of nearlyEqual though) on the branch great-expectations.

rtfeldman commented 7 years ago

@mgold I read that as a poll - as in, "react with 🎉 to vote for within and react with ❤️ to vote for tolerance". (This is something we do on Slack as a way to quickly survey preferences.)

Given this, and given how fuzzing with floats seems likely to result in flakey tests in the absence of this function, I'm 👍 for adding Fuzz.within : Float -> Float -> Float -> Expectation

mgold commented 7 years ago

I read that as a poll

Oh, of course. And the argument order would be tolerance, expected, actual. So for example:

fuzz float "pythagorean identity" <| \x ->
  sin x * sin x + cos x * cos x |> Expect.within 0.001 1.0

test "floats known to not add exactly" <| \() ->
  0.1 + 0.2 |> Expect.within 0.000000001 0.3

Seems good to me.

rtfeldman commented 7 years ago

Closed by https://github.com/elm-community/elm-test/pull/102