Friday, April 17, 2009

Tests and Booleans and Matchers

I was working with the new ScalaTest matchers API recently when I came across something that made me think (I usually don't think). I had something simple like:

Nil.empty must be(true)

This seems harmless enough, but when I made it fail intentionally the error message I got was something reasonably similar to:

Expected true but got false.

This obviously isn't a very good error message, and I was thinking at first the maybe we should provide the user API to provide their own error message. Something like so:

Nil.empty must be(true, "expected Nil to be empty, but it wasn't!")

This also seems harmless enough. However, I talked this over with Bill Venners and in this case there is a way to do it using symbols and reflection that gives better error messages:

Nil must be('empty)

If this fails, it will give a nice error message like:

Nil was not empty

This is ok, but type safety people will scream, and they'd probably be right. And, does this work in all cases and can we always get good error messages? I think no. Let me explain why.

What if we have an object that has a method that takes an argument and returns a boolean like this theoretical surroundedBy method that could exist on String:

def surroundedBy( s: String ) = startsWith(s) && endsWith(s)

We can't use the symbol form here because the method takes a parameter. So what can we do? If we go back to the original form then we are back to our original bad error message.

"htth".surroundedBy( "h" ) must be (true)
"http".surroundedBy( "h" ) must be (false)

Do we have any other options? Yes (with caveats as well). But first lets think about what we'd really like to write here. If we were writing pure English, it would be this:

"htth" must be surrounded by "h"
"http" must not be surrounded by "h"

Matchers


ScalaTest 0.9.5 came out with a new Matchers API that I'll touch on briefly here, and point you to documentation for more. Using matchers, we can get our client code pretty close to this. We'll have to define our custom matcher first:

case class surroundedBy(r:String) extends BeMatcher[String] {
def apply(l: String) =
MatchResult(
l.surroundedBy(r),
l + " was not surrounded by " + r,
l + " was surrounded by " + r
)
}

And our client test code turns out to be pretty close to the English:

"htth" must be(surroundedBy("h"))
"http" must not be surroundedBy("h")

I like this a lot, but, only if its going to be used a lot. Otherwise, if it's a one off test, I could just kick it back way old school and use an assert with an error message:

assert( "htth".surroundedBy("h") === true,
"expected http to be surrounded by h, but it wasnt." )
assert( "http".surroundedBy("h") === false,
"expected http not to be surrounded by h, but it was!!!" )

This is less readable code, but it is less code - about 6 lines less when you include the custom matcher. You get more or less the same error message in this case as well. It is quite ugly though. In this case I'd much rather have an API that just gives me what I originally wanted:

"htth".surroundedBy("h") must be(true,
"expected htth to be surrounded by h, but it wasnt.")

or better:

"htth" must be('surroundedBy, "h")

I could see this one existing easily. It's really nice, but not type safe.

Anyway, maybe there already are better alternatives like the ones I've explained I simply don't know about. Maybe I haven't done my homework and I should be scolded. I really hope not, and I'll happily write up (and more importantly - use) anything I find that helps make this code more readable. Until alternatives surface, I guess I'll somewhat reluctantly stay with these rules:

When testing methods that return booleans you can

  • Use the symbol style ('empty) if the method takes no arguments and you aren't terrible concerned about type safety.

  • Use old school assertions when the method takes arguments but you won't have a lot of repetitive tests for the method.

  • Use a custom matcher otherwise


You'll also find that ScalaTest (and Specs which is unfortunately not covered here) provides many matchers that can use for many common types.

In my next post I'll provide a much deeper overview of how to write your own custom matchers in ScalaTest, starting by explaining what that surroundedBy case class means.

In the meantime, you can look at ScalaTest Matchers and Specs Matchers.

Here's all the custom matcher code:

package org.scalatest.examples

import org.scalatest.matchers.MustMatchers._
import org.scalatest.matchers._


class MatcherTest extends FunSuite with MustMatchers{

implicit def surroundable[T](l:String) = new {
def surroundedBy(r:String) = l.startsWith(r) && l.endsWith(r)
}

case class surroundedBy(r:String) extends BeMatcher[String] {
def apply(l: String) =
MatchResult(
l.surroundedBy(r),
l + " was not surrounded by " + r,
l + " was surrounded by " + r
)
}

test("weve got the place surrounded"){
"htth" must be(surroundedBy("h"))
"http" must not be surroundedBy("t")
}
}

13 comments:

  1. This is an easy problem to solve - just not in Scala right now :-). One answer is syntactic metaprogramming, aka macros. See Template Haskell. The idea is that you say something like

    ensure(Nil.empty must_be(true))

    And at compile time the custom "ensure" macro expands that to

    Nil.empty must_be(true, "Expected Nil.empty to be true but it wasn't so fix it!")

    And then the regular compiler does its thing - detecting misspellings or type errors in the use of Nil.empty as usual.

    ReplyDelete
  2. Hi Josh,

    Macros are indeed something I wish I had in Scala for a few things. However here is something available in specs to add a better description for failure messages:

    Nil.empty aka "empty on an empty list" must be (true)

    (even if I would use the "empty" pre-defined matcher here ;-) )

    This "aka" (also-known-as) feature is documented here:

    http://code.google.com/p/specs/wiki/MatchersGuide#Precise_failures

    Eric.

    ReplyDelete
  3. Eric,

    Bill and I talked for a bit about the aka operator for a bit the other day. I've since read about it, and I even started to write it up this morning but I stopped. The reason being that I simply couldn't think of anything reasonable to alias this to:

    "htth".surroundedBy( "h" )

    What would I alias this to?

    I really like the aka operator, and in many situations it should be great. Unfortunately this was my main example and I was stumped.

    I'm going to give this some more thought before I write anythign up and if you have more ideas that would be great too.

    ReplyDelete
  4. James,

    Recall that ideally, I'd want as close to English as possible, like this:

    "htth" must be surrounded by "h"
    "http" must not be surrounded by "h"

    Does your macro "ensure", has to be put on the front of the code like that? Would there be a way to not have it front? Basically, can macros help make the code look more like the English specification?

    Also, if you could provide well known links so I could catch up, that would be awesome.

    Thanks!

    ReplyDelete
  5. Hi Josh,

    A BeMatcher is indeed the ScalaTest extension point to add the syntax you want, but I would add a space so it reads a bit more like an English sentence. Instead of:

    "htth" must be(surroundedBy("h"))

    I'd write:

    "htth" must be (surroundedBy ("h"))

    However, I think "surroundedBy" is a bit obscure. One of the goals I had in the matchers DSL design was that people unfamiliar with the library would not have to look things up when they encountered the syntax. That it would be obvious from context. So I'd prefer "startAndEndWith", which would be a plain old Matcher not a BeMatcher. Then you could write:

    "htth" must startAndEndWith ("h")

    This echos the startWith and endWith tokens that are already in the API too. And you don't need the extra parens on the positive case. However you need them on the negative case:

    "http" must not (startAndEndWith ("h"))

    If you really want to get rid of this, you don't need macros. This is Scala, right, so you can "pimp" the ScalaTest library with just a bit of finesse. What's missing is a startAndEndWith method on class ResultOfNotWordForString. If you want to appear to add a method to a class, just use an implicit conversion. Then you could write:

    "http" must not startAndEndWith ("h")

    You could also use this pimping technique to get the syntax you wanted originally:

    "htth" must be surroundedBy ("h")

    The reason I don't document this in the ScalaTest docs is that it is a bit advanced, and a bit time consuming. To really make it complete, you'd need to pimp "and", "or", "and not", and "or not" as well. And I'm not sure that's really what people should be spending their time on. I would probably put this in a book about ScalaTest, just so the info is out there and there's an example. But I'd suggest people question whether their time is best spent on that kind of thing.

    Eric, I was telling Josh that I quite like the name of the aka operator, but I just didn't see much of a use case, because in Scala objects tend to have pretty good toString messages. So ScalaTest doesn't have this yet. When we investigated the specific use case Josh was struggling with, it turned out there was a better expression to use that solved his problem already in ScalaTest. So I'm still waiting and looking for use cases.

    Lastly, Josh, if you're just doing the surround with kind of thing a few times, I'd recommend you just do it like this:

    "htth" should (startWith ("h") and endWith ("h"))

    That's readable and supported already, though it does have the "h" repeated. But unless you're doing it a lot, I'd go this route.

    Bill

    ReplyDelete
  6. Hi Bill,

    I'll be the first to admit that this thing is a corner case and isn't a huge deal at all really. But, I do have a few thoughts and questions.

    I think its safe to assume that eventually we'll run across a method that takes an argument and returns a boolean, and, this method will be on a user defined type, not a ScalaTest OOTB supported type. Maybe String was a poor example on my part here, because it is supported.

    I was looking for a general rule for this case:

    trait T{
    def m(a:Any): Boolean
    }

    The rule should expand to encompass any number arguments on m as well, such as:

    trait T{
    def m(a:Any*): Boolean
    }

    Also, in your comment you suggested using
    "htth" should (startWith ("h") and endWith ("h"))

    This is, of course, very nice. But if the type is simply T, are you assuming that we can modify T or that T has more methods on it than just m? What if we can't modify it, and m is the only method on T?

    It's probably a rare case, and reasonably trivial, but I think it would be nice to support it.

    You might say that we should have a concrete example, and you'd be right. I'll keep my eyes open.

    Also, if it pimping the matchers library was an easy thing to do, would you feel the same? Even if its not simple to pimp, its probably wise to pimp it if you have something that is sure to appear over and over again in your tests, I think.

    I've been spending some time understanding your matchers code, and I'll try to pimp it and see if I have any thoughts on how to make that easier.

    Thanks for the feedback.

    ReplyDelete
  7. Hi Josh,

    I'll try pimping it to see what it looks like. I suspect the resulting code will be quite small and relatively easy to understand. The tricky part will be knowing where to do the pimping. That's what I'm not sure will be worth people's time.

    I one time had a consulting job where I was being paid by the hour to track down a bug in some C code. To reproduce the bug took several iterations of adding in some debug code, then doing a build, then running it. But unfortunately to run it, I had to get completed dressed in white garb and go into a clean room and run some process that took a half hour. So it ended up taking 8 hours to track down the bug, which I fixed by adding 1 pair of parentheses. I got paid something like $600 for that pair of parens, but I used to say I charged $1 for each paren, and $598 for knowing where to put them.

    So the problem with pimping the matchers API instead of using the "recommended" custom matchers approach is having to spend the time studying the API so you know where to do it. But I think it is worth having an example anyway, so long as I explain that you should really think if this is the best use of your time. Because after all, you're probably supposed to be getting some production code done, and you want to avoid getting so distracted by making the test code beautiful that you miss your deadline for the production code.

    ReplyDelete
  8. "I charge $1 for each paren, and $598 for knowing where to put them!" This is going into my newest post as an update. Hysterical.

    As for getting distracted by making test code beautiful, I'm guilty. Professional developers should spend time on making their tests, well, professional. However, they probably shouldn't obsess about it.

    But its good to have a few overly obsessed people like us to make writing tests as natural as possible for everyone else. My definition of natural in this case is simply an English specification. I think my job as the sidekick here is to use everything, question everything, push it to the limits, and make improvements. I think we can get to "natural" for all cases, not just most.

    ReplyDelete
  9. This comment has been removed by the author.

    ReplyDelete
  10. Hi Josh and Bill,

    One thing about the "surroundedBy" example.

    I think we must remember what we're specifying in that example. Whatever the name is, we're testing the return value of a method called "surroundedBy".

    So my feeling is that the specification could look like this:

    "The surroundedBy method" should {
    "return true when a string begins and ends with the same letter" in {
    "htth".surroundedBy("h") must be (true)
    }
    "return false when a string doesn't begin and end with the same letter" in {
    "http".surroundedBy("h") must be (false)
    }
    }

    Then, if the example fails, the report will show something like:

    return true when a string begins and ends with the same letter
    x expected true, got false

    In that case, even if the failure message is not very descriptive, the example description is here to help us understand what was wrong. The only thing not showing up is the precise method call being used in the example. It would be difficult to display the example in the error message without breaking the DRY principle in Scala (we could use and interpreter but then we loose type-safety and automated refactoring).

    Trying to create a custom "surroundedBy" matcher is a possibility but IMHO this hides the purpose of the specification and adds a supplementary risk of introducing a bug in the specification code, which will possibly hide a bug in the production code.

    About the "aka" operator, I've used it a few times to give more meaning to the failure messages. For example:

    "allow a label decorator to be used to surround the label" in {
    val p = Prop("Result", 1.123).decorateLabelWith((s: String) => {s}).toXhtml(0) \("td")
    p aka "a prop with a bold decorated label" must ==/(Result)
    }

    In that case, the "aka" description gives a bit more details about the precise data used for the example.

    Here's another interesting example from specs specification code:

    s.systems.flatMap(_.examples).flatMap(_.failures).size aka "the number of failures" must_== 0

    In the example above, "the number of failures" serves 3 purposes:

    - it gives better context for the failure message. Instead of having "1 is not equal to 0", you get "the number of failures: 1 is not equal to 0".

    - it documents the complex expression before the "aka"

    - it gives a big "refactoring" warning actually! If the expression is so complex that it needs to be described, then it should certainly refactored to a better API (which actually exists now in specs: "s.failures.size",...)

    Eric.
    [I removed the previous comment to try putting code in "code" tags but this doesn't work,...]

    ReplyDelete
  11. Eric,

    I agree, mostly, with a couple of hesitations. I see the value in writing out the spec in detail as you did, but its also very long winded. There are positives and negatives with this.

    First, it's very similar to documentation in the code, which can quickly become out of date. That scares me a bit. It's possible that a refactoring changes the SUT, yet the tests still pass and the comments have little meaning, or worse - incorrect meanings. The more actual code we can specify in near English, the safer we will be.

    Second (and this is more of a personal taste), for unit tests, I think it feels like cutting your steak with a chainsaw. I think most unit tests should be smaller. To me the longer winded style works wonders when specifying business functionality for your stories. But at the class level, where things change faster, I'd rather keep thinks much smaller. And, when I'm practicing TDD I sometimes don't have the right words for my specs. I have rough ideas and sometimes I know exactly, but I often find myself going back later and populating the spec messages. Like I said, maybe this is just a personal style thing.

    What can we do to combat the first issue?

    ReplyDelete
  12. > What can we do to combat the first issue?

    Well, if it's a real refactoring, the meaning shouldn't change, should it?

    Anyway, you're right, too much text along with the examples may decay pretty fast. But I'm afraid that we can't stay as DRY as we wish in Scala. Certainly something like 'Template Scala', if there was such a thing, would give us better capabilities.

    ReplyDelete
  13. >Well, if it's a real refactoring, the meaning shouldn't change, should it?

    I'm assuming you do understand, but for some people I think this statement can be misleading. I think there's a misconception with refactoring for some people where they think behavior shouldn't change. Behavioral changes can happen at the micro level, while still preserving behavior at the macro level. This is commonplace.

    Unit tests can become out of date very quickly with a very high level refactoring/code restructuring. Methods and classes can disappear entirely, yet to someone in the business, nothing changed.

    This is the level that I've been at through all of this discussion - I'm testing something very, very small.

    >Anyway, you're right, too much text along with the examples may decay pretty fast. But I'm afraid that we can't stay as DRY as we wish in Scala. Certainly something like 'Template Scala', if there was such a thing, would give us better capabilities.

    I think we can have API in Scala to deal with all cases, very big and very small. That's Scala's goal as a language in general. We just need to continue to work to get there. I agree with Bill, we don't need something like Template Scala. We just need to take our time and discuss all these cases, just like we are doing now.

    In fact, I think we are almost there. This whole discussion started with one tiny thing I felt uncomfortable with.

    ReplyDelete