yesodweb / yesod

A RESTful Haskell web framework built on WAI.
http://www.yesodweb.com/
MIT License
2.64k stars 373 forks source link

yesod-test assertEqual should report values that were not equal #1255

Open bitemyapp opened 8 years ago

bitemyapp commented 8 years ago

Currently if you have more than one or two assertEquals in a single test stanza it's infuriating to figure out why a test failed.

Simply reporting the values themselves via Show would be wunderbar. I'm using a helper function to work around this.

Better still would be to use a pretty-printing function that uses Show. I've had great luck with pretty-show.

I'm going to file an issue for a more elaborate version of this that uses diffing + pretty-printing as a wishlist item. There's really no reason for us to not have the kind of polish that Ruby, Python, etc. users are accustomed to. We should not settle for this.

Here's an example of what my test failure currently looks like with assertEqual:

  can report on stats FAILED [1]

Failures:

  ./Yesod/Test.hs:319: 
  1) MyProject.Integration can report on stats of rows for all related data
       active rows

I've stripped the descriptions themselves a little, but the point is, a number from the stats wasn't equal to what we expected (3 /= 0 or somesuch) and we're told little beyond the message attached to the assertEqual itself ("active rows").

bitemyapp commented 8 years ago

For this one I'd rather just change the default behavior of assertEqual so that beginners don't miss it, shoving the version that doesn't Show into a dark pit somewhere. Main reason not to have a Show usually also means you don't have Eq anyway.

bitemyapp commented 8 years ago

Simple example screenshot from 2016-08-08 13-55-30 screenshot from 2016-08-08 13-55-26

bitemyapp commented 8 years ago

Sidebar: another issue (not yesod-test's fault I think) is that the line numbers reported are never right.

bitemyapp commented 8 years ago

Example for record types screenshot from 2016-08-08 13-59-45 screenshot from 2016-08-08 13-59-19

bitemyapp commented 8 years ago

I'll PR this if there are no objections.

snoyberg commented 8 years ago

Sure, let's go for it. I don't mind a major version bump on yesod-test, though if we want to be really methodical, roll-out would be:

  1. Add new function with desired behavior
  2. Add synonym of current behavior under different name
  3. Deprecate current function, refer to two new functions
  4. Release breaking version with current function switching to new behavior

But I'm really not that bothered about simply new-versioning it for something as non-invasive as this.

Potentially evil thought: maybe we should use some evil typeclass machinery to work both with and without a Show instance, via some kind of overlapping instances. I think it would work, and given that it's just used for convenience I wouldn't feel too bad about it. Plus, I'm sure it will really upset someone who will write a rant about how evil Yesod and I are, which is definitely an advantage.

MaxGabriel commented 8 years ago

I think this is fantastic! Love this PR

I don't want to risk bike shedding too much, but what do you think about using different language than expected/got, similar to what Elm is doing? Maybe something like "the first value was/but the second value was" or something? Again, don't want to bike shed, so if you don't love this idea don't worry about it

bitemyapp commented 8 years ago

@MaxGabriel that's the wording I originally wanted. My PR will reflect that. Thank you.

bitemyapp commented 8 years ago

Also pointing over to #1256 - if anyone knows a good visual diff library that works with Generic, I'd be deeply appreciative.

bitemyapp commented 8 years ago

Or even Show munging. I'm up for anything at this point.