Improved testkit

[holograph]

The Accord testkit (for both Specs2 and ScalaTest) is both woefully underpowered and fails the DRY principle. Specifically, it makes it very hard to use native matchers, and impossible to compose rules freely. For example, with the existing framework it's impossible to assert that a result is a failure, and that all violations must match a specific rule. In short, a better testkit is necessary.

Following is prototype code for a new testkit based on Specs2:

trait AbstractResultMatchers {
  import com.wix.accord.{Result, Violation}

  protected type Matcher[ -T ]
  protected def anyMatcher: Matcher[ Any ]

  type ViolationMatcher = Matcher[ Violation ]
  type ResultMatcher = Matcher[ Result ]

  def matchViolation( value:       Matcher[ Any ]              = anyMatcher,
                      constraint:  Matcher[ String ]           = anyMatcher,
                      description: Matcher[ Option[ String ] ] = anyMatcher ): ViolationMatcher
  def matchRule     ( value:       Matcher[ Any ]              = anyMatcher,
                      constraint:  Matcher[ String ]           = anyMatcher,
                      description: Matcher[ Option[ String ] ] = anyMatcher ): ViolationMatcher
  def matchGroup    ( value:       Matcher[ Any ]              = anyMatcher,
                      constraint:  Matcher[ String ]           = anyMatcher,
                      description: Matcher[ Option[ String ] ] = anyMatcher,
                      children:    Matcher[ Set[ Violation ] ] = anyMatcher ): ViolationMatcher

  def fail: ResultMatcher
  def succeed: ResultMatcher
  def failWith( violations: Matcher[ Set[ Violation ] ] ): ResultMatcher
  def failWith( violations: Matcher[ Violation ]* ): ResultMatcher

  /** A convenience implicit to simplify test code. Enables syntax like:
    *
    * ```
    * val rule: ViolationMatcher = "firstName" -> "must not be empty"
    * // ... which is equivalent to
    * val rule = matchViolation( description = "firstName", constraint = "must not be empty" )
    * ```
    */
  implicit def stringTupleToViolationMatcher( v: ( String, String ) ): ViolationMatcher

  def rule( m: ViolationMatcher ): ViolationMatcher
  def group( m: ViolationMatcher ): ViolationMatcher
}

package specs2 {
  import com.wix.accord._
  import org.specs2.matcher.{Matchers, AlwaysMatcher}

  trait AccordResultMatchers extends AbstractResultMatchers {
    self: Matchers =>

    protected type Matcher[ -T ] = org.specs2.matcher.Matcher[ T ]

    protected def anyMatcher = AlwaysMatcher()

    def matchViolation( value:       Matcher[ Any ],
                        constraint:  Matcher[ String ],
                        description: Matcher[ Option[ String ] ] ) =
      value       ^^ { v: Violation => v.value       } and
      constraint  ^^ { v: Violation => v.constraint  } and
      description ^^ { v: Violation => v.description }

    def matchRule( value:       Matcher[ Any ],
                   constraint:  Matcher[ String ],
                   description: Matcher[ Option[ String ] ] ) =
      rule( matchViolation( value, constraint, description ) )

    def matchGroup( value:       Matcher[ Any ],
                    constraint:  Matcher[ String ],
                    description: Matcher[ Option[ String ] ],
                    children:    Matcher[ Set[ Violation ] ] ) =
      group( matchViolation( value, constraint, description ) ) and
      children ^^ { v: Violation => v.asInstanceOf[ GroupViolation ].children }

    implicit def stringTupleToViolationMatcher( v: (String, String) ): ViolationMatcher =
      matchViolation( description = beSome( v._1 ), constraint = be_===( v._2 ) )

    def fail: ResultMatcher = beAnInstanceOf[ Failure ]
    def succeed: ResultMatcher = be_===( Success )

    def failWith( violations: Matcher[ Set[ Violation ] ] ): ResultMatcher =
      fail and violations ^^ { f: Result => f.asInstanceOf[ Failure ].violations }

    def failWith( violations: Matcher[ Violation ]* ): ResultMatcher =
      failWith( contain( allOf( violations:_* ).exactly ) )

    def rule( m: ViolationMatcher ): ViolationMatcher = beAnInstanceOf[ RuleViolation ] and m
    def group( m: ViolationMatcher ): ViolationMatcher = beAnInstanceOf[ GroupViolation ] and m
  }  
}

This testkit is mostly source-compatible with existing test code based on accord-specs2, but still needs some work on the API and available matchers. Comments welcome.

[holograph]

@ittaiz @uriavraham @noam-almog your input would be invaluable here...

[noam-almog]

@holograph First thing, as we previously discussed I'd like you to add a:

def beValid: Matcher[ ResultMatcher ]
def beInvalid: Matcher[ ResultMatcher ]
def beInvalidWith( violations: Matcher[ Set[ Violation ] ] ): ResultMatcher
def beInvalidWith( violations: Matcher[ Violation ]* ): ResultMatcher

As for the rest, I rather inspect matchers on their natural environment, do you have any examples of those matchers being used somewhere ?

side comment: contain( allOf( violations:_* ).exactly is basicall containTheSameElementAs matcher.

[ittaiz]

+1 for @noam-almog's request for usage, I was thinking the same thing. WDYT about the following usage?

"Person Validations" should {
   "fail a person with an empty first name" in new ctx { 
         val personWithEmptyFirstName = Person().with(name = "")
         validate(personWithEmptyFirstName) must beViolation("firstName" -> "must not be empty")
    }
}   

[holograph]

I have a couple of qualms with that idea:

1. validation(...) must beInvalid vs validation(...) must fail is an aesthetic preference. I could provide both, but that'll be a little redundant and mess up the DSL. It might make sense to offer each in its own trait, but that'll hurt discoverability. Since the second option is my personal favorite, I'm leaning towards keeping it as-is; perhaps you can open a separate issue and we'll take votes.
2. What are the semantics of the variadic beInvalidWith (or failWith)? Are all matchers expected to succeed? In order? What happens if there are additional violations? I left it in for backwards compatibility, but I'm less than happy with the design.

As for @noam-almog's question, unless I misunderstand, contain(allOf(...)) is not exhaustive whereas containTheSameElementAs is. In other words, this passes:

List( 1, 2, 3 ) should contain( allOf( 2,3 ) )
List( 1, 2, 3 ) should not( containTheSameElementsAs( List( 2, 3 ) ) )

[noam-almog]

Adding more options that does the same thing is something common with specs, there are always matchers with different method names to make them more readable in several usages for example: be_=== beTypedEqualTo typedEqualTo

I don't think it needs to be separated to different traits.

@holograph , when I started writing stuff I realized that we started on the wrong foot. The code that you sent makes us debate over this code vs another code where the questions that we need to ask are different. The questions should be:

1. What should dev want to check in cases of validation (should they match description, only property, both)
2. Should matchers accept partial violation, they have a subset of violations or full match.

My answers would be:

2. Full match
3. My usages are: I want to be able to just write which properties will fail without description, I want to be able to write properties with descriptions, I want to be able to just check that something is invalid. I don't thing we need to allow them a matcher over description, so user can check description contain something (now you give them full match only which is acceptable) Suggestion: I think we might need to give a matcher over property name so user can give a subset of properties from a whole object so user can say that a cluster of properties will fail without specifying all of them.

This three matchers can be translated to code somehow (with some syntax we can pick) so the code would be:

   validate(SomeClass(p1 = "", p2 = "", p3 = "") must beInvalidWith(violations = "p1", "p3")

   validate(SomeClass(p1 = "", p2 = "", p3 = "") must beInvalid

   validate(SomeClass(p1 = "", p2 = "", p3 = "") must beInvalidWith(violations = "p1" -> "p1 is invalid", "p3" -> "p3 is a null")

   validate(SomeClass(p1 = "", p2Text = "", p3 = "") must beInvalidWith(violationsThat = contain("Text"))

Now we can switch the the beInvalidWith (mine) with failWith (yours) or keep them both...

Btw, I don't like that we write the property names as strings which are not directly connected to the actual member of the class (val or def) but I don't see any way around this.

Btw2, descriptions are flawed because basically nobody can actually know what the description is without running the validating and then copy paste it to the test. This is what I do in the hub (and I know @ittaiz will hate me forever for this) but I would like to find a way to be able to know what the description is before the test to make this TDD compatible.

Btw3, I think I gave you enough reading materials for the whole weekend.... :)

[ittaiz]

Interesting discussion. @holograph Can we use Macros to avoid using property names? Just reference a member and the macro will expand that to the relevant text? After thinking about what @noam-almog said with respect to textual descriptions I think he's right but the question is why not have a DSL for that also- person.firstName -> is(not(null))) ? I don't understand when you'll write to fail all properties whose name equals something ("Text")? That really sounds like something that might allow you to group 2-3 fields max but will cause readability issues. If we remember that we use these validations frequently in API jars and these tests should also enable clients to understand the validations then I think having a set of tests which say "do"s and "don't"s for properties is great. With respect to the partial matching of violations I'm not sure I understand what you're talking about. Are you referring to the case where firstName has multiple validations (can't be null, empty and can't contain the word "john")? If so I think I lean towards specifying each validation in its own test but I think the test-kit can allow both variants (the second is more relevant for test-after)

[noam-almog]

@ittaiz You are not thinking big enough, you can have big objects with multiple groups and as for the readability it can be solved easily.

def isTextField: Matcher[String] = contain("Text")

validate(SomeClass(p1 = "", p2Text = "", p3 = "") must beInvalidWith(violationsThat = isTextField)

As for tests for this, I discussed this with a few ppl back in the day and it was decided that those validators won't need tests. As @holograph put it, 'you wouldn't test the OR mapping for your OR mapper'. I write tests for those validators because I don't trust (forgive me Tomer) accord entirely and I think I should test it's validators and just because I'm on kind of an auto pilot sometimes. My rule for now, is validate those objects as long as there are not too many of them. In user activity domain I wrote tests for 60% of the objects and gave up.

As for partial, I meant that an object failed on p1, and p2, when I match against p1 only it should fail because it didn't fail on p2, failures should match entirely or fail. It's the current assumption in the matchers and I think it should remain the same but still I felt it needed to be specified.

[ittaiz]

I think that we do “test the OR mapper” in the sense that we write Integration Tests which describe what we need out of the tool and what we expect it to do (or in other words to see that we use it correctly).

I know that you do not subscribe to integration tests but that doesn’t mean I don’t want the ability to do such tests.

There is a question of whether we should use the tool’s test kit for these tests as it might be lying too (or we misunderstand the kit as well).

@sagyr WDYT?

BTW, I think I am thinking big I just think a complex object should be composed of other complex objects and so on and that each part should be tested. Doing God tests is hard and unpleasant. 

Ittai Zeidman   Cell: 054-6735021 40 Hanamal street, Tel Aviv, Israel

On Sun, Nov 16, 2014 at 12:24 PM, Noam Almog notifications@github.com wrote:

@ittaiz You are not thinking big enough, you can have big objects with multiple groups and as for the readability it can be solved easily.

def isTextField: Matcher[String] = contain("Text")
validate(SomeClass(p1 = "", p2Text = "", p3 = "") must beInvalidWith(violationsThat = isTextField)

As for tests for this, I discussed this with a few ppl back in the day and it was decided that those validators won't need tests. As @holograph put it, 'you wouldn't test the OR mapping for your OR mapper'. I write tests for those validators because I don't trust (forgive me Tomer) accord entirely and I think I should test it's validators and just because I'm on kind of an auto pilot sometimes. My rule for now, is validate those objects as long as there are not too many of them. In user activity domain I wrote tests for 60% of the objects and gave up.

As for partial, I meant that an object failed on p1, and p2, when I match against p1 only it should fail because it didn't fail on p2, failures should match entirely or fail. It's the current assumption in the matchers and I think it should remain the same but still I felt it needed to be specified.

Reply to this email directly or view it on GitHub: https://github.com/wix/accord/issues/26#issuecomment-63213678

[noam-almog]

As I mentioned earlier, there is no strict guideline to test all object validators on each project, and one should rely on this FW without thoroughly test each validator. What I found that on the TDD scale there are more relaxed ppl and more strict, you for example are strict, other ppl said that it doesn't need tests at all, I'm somewhere in the middle.

Not sure what you are saying that I 'do not subscribe to integration tests' or anything you said after that.

This test kit is intended to test the validators on our unit tests. Do I use it, yes, Do I think everyone should use it, I'm not sure, it depends.

[ittaiz]

I was referring to DAO tests

— Sent from my phone

On Sun, Nov 16, 2014 at 5:01 PM, Noam Almog notifications@github.com wrote:

As I mentioned earlier, there is no strict guideline to test all object validators on each project, and one should rely on this FW without thoroughly test each validator. What I found that on the TDD scale there are more relaxed ppl and more strict, you for example are strict, other ppl said that it doesn't need tests at all, I'm somewhere in the middle. Not sure what you are saying that I 'do not subscribe to integration tests' or anything you said after that.

This test kit is intended to test the validators on our unit tests. Do I use it, yes, Do I think everyone should use it, I'm not sure, it depends.

Reply to this email directly or view it on GitHub: https://github.com/wix/accord/issues/26#issuecomment-63221884

[sagyr]

If the customer wants the product to reject certain inputs with an error I have several options. One would be to validate it manually, yet another might be using some sort of a tool or library. Whatever my choice is for implementing the requirement - it all starts with a failing test. I'm not testing the library I'm just using it to solve a problem and driving it through tests.

[holograph]

I've started more seriously looking at this; I believe the ideal syntax would be something like:

// Test domain --

case class Container(f: String)
case object Container {
    implicit val v = validator[Container] { _.f is notEmpty }
}

case class Test(f1: String, f2: Int, coll: Seq[Container])
case object Test {
    implicit val v = validator[Test] { t =>
        t.f1 is notEmpty
        t.f2 should be > 5
        t.coll.each is valid
    }
}

// Assertions --

val result = validate(Test("", 3, Seq(Container("Good"), Container(""))))

result shouldBe aFailure

// Macro-driven property descriptions per @ittaiz:
result should failOn(_.f1)          
// Negation         
result shouldNot failOn(_.f1)
// Optional constraint matching; values are implicitly expanded to be_===
result should failOn(_.f1 -> "must not be empty")
// Optional value matching
result should failOn(_.f2 -> 3 -> "got 3, must be greater than 5")
// Composing Specs2/ScalaTest matchers
result should failOn(_.f2 -> be_>=(2))
// Matching on group (nested) violations
result should failOn(_.coll because (Container("") -> "is not valid") because (_.f -> "must not be empty"))
// Multiple matches (variadic) with implicit /AND/
result should failOn(_.f1, _.f2, _.coll)
// "Only" modifier for exhaustive checks
result should failOn(_.f1).only

This will require a pretty substantial change to the result model though, since failOn (or whatever we end up calling it) has to know the origin type (Test, in this case) in advance, and the current result model does not provide this information. I'm not sure this is worth it, but the alternative is to explicitly provide the type information on the failOn call, which I feel is a deal-breaker. Right now I feel the better alternative would be to entirely forego typesafe property references.

Thoughts? (pinging @ittaiz @sagyr @noam-almog @hugebdu)

[noam-almog]

:+1: I prefer the beValid notation but other than that, looks gr8.

[holograph]

Unfortunately this (and any variant on the syntax) can never work. The problem is the way the Scala type inference work. Consider the following expression:

result should failOn(_.f1 -> "must not be empty")

The crux of the problem is that failOn has no information with which to infer what the anonymous extractor function (_.f1) takes as its parameter. Even if the result model was parameterized, type inference does not "carry the type" from the left-hand side of the should, so regardless of the strategy, failOn must be parameterized.

A different syntax could be result should failOn[Test](_.f1), but that seems inelegant; a possibly more sensible syntax would be:

  result should failOn { t: Test =>
    t.f1
    t.f2 -> 3 -> "got 3, must be greater than 5"
    t.coll because ( Container( "" ) -> "must not be empty" )
  }

Requiring a brand new DSL for matching violations, to me, feels pretty abusive of Accord's users. What do you think?

[holograph]

Well for the time being, I'm reducing the scope of this feature: basically, switch to using native matchers (for whichever testing library you end up using) in a backwards-compatible manner.