etorreborre / specs2

Software Specifications for Scala
http://specs2.org
Other
734 stars 215 forks source link

Isolated doesn't create a new spec instance for each scalacheck test #263

Closed caspark closed 10 years ago

caspark commented 10 years ago

(or at least, that's my uneducated summary of the problem)

If you have a test like this:

package com.atlassian.crowd.manager.login.util

import org.specs2.mock.Mockito
import org.specs2.mutable.SpecificationWithJUnit
import org.specs2.ScalaCheck

object TestPieces {
  trait MyInterface {
    def doSomething(): Unit
  }

  class MySystemUnderTest(myInterface: MyInterface) {
    def doTest() = myInterface.doSomething
  }
}

class MyDemoTest extends SpecificationWithJUnit with Mockito with ScalaCheck {
  isolated

  val myMock = mock[TestPieces.MyInterface]
  val underTest = new TestPieces.MySystemUnderTest(myMock)

  "show-problem" in propNoShrink {
    (myStr: String) ⇒
      //org.mockito.Mockito.reset(myMock)
      underTest.doTest
      there was one(myMock).doSomething
  }
}

then show-problem will fail. If you uncomment the line to reset the mocks, it won't fail.

Presumably this is because specs2 doesn't create a new instance of the spec for each scalacheck "test case".

This may not technically be a bug, since the docs say "The isolated argument changes the execution method so that each example is executed in a brand new instance of the Specification", but it sure goes against the spirit of isolated and it'd be great if we didn't have to reset our mocks or create mocks from within the test code.

I also tried to use scopes, to no avail:

  "show-problem-2" in new MyScope {
    propNoShrink {
      (myStr: String) ⇒
        //org.mockito.Mockito.reset(myMock)
        underTest.doTest
        there was one(myMock).doSomething
    }.set(minTestsOk = 10)
  }

  trait MyScope extends Scope {
    val myMock = mock[TestPieces.MyInterface]
    val underTest = new TestPieces.MySystemUnderTest(myMock)
  }

It makes sense to me that that did not work because the scope wraps the propNoShrink, but I couldn't work out how to get it to compile with scope inside the propNoShrink block unless I have the block return ():

  "show problem" in
    propNoShrink {
      (myStr: String) ⇒
        //org.mockito.Mockito.reset(myMock)
        new MyScope {
          underTest.doTest
          there was one(myMock).doSomething
        }
        ()
    }.set(minTestsOk = 10000)

So then I figured I'll do implicit def scopeToProp(u: =>Scope): Prop = () but then the test passes even if asserts in the scope fail, so clearly I'm missing a piece here.

caspark commented 10 years ago

Doh, that last implicit def wasn't working because u wasn't evaluated. But implicit def scopeToProp(u: =>Scope): Prop = { u; () } works as expected, so perhaps there should be something along those lines in org.specs2.matcher.ResultPropertyImplicits?

etorreborre commented 10 years ago

I think that the problem is not so much with isolation here. If you use ScalaCheck, the property is going to be called several times with the same mock object. You would need to reset it in order to get a "fresh" state for that mock otherwise it will remember its past invocations. Not only that but if the ScalaCheck property is evaluated concurrently, executions will step on each other, so reset won't cut it.

One thing you can do is to use a Scope. I'm a bit reluctant to add the implicit you suggest because it adds one more implicit to the implicit search, and this slows down compilation, but also it works only because of side-effects.

An other option is to get ScalaCheck to give you a fresh mock for each property execution:

implicit def arbitraryMock = Arbitrary(Gen.wrap(Gen.const(mock[TestPieces.MyInterface])))

"show-no-problem" in propNoShrink { (myStr: String, myMock: MyInterface) ⇒
   new MySystemUnderTest(myMock).doTest
   there was one(myMock).doSomething
}
caspark commented 10 years ago

Fair enough, that's a good enough idealistic reason to not add the implicit.

We ended up using scopes with this (very similar to the original) implicit:

/**
 * Allow using specs2 scopes inside scalacheck blocks; workaround for https://github.com/etorreborre/specs2/issues/263
 * This is only safe to mix in for mutable specifications.
 */
trait ScalaCheckScopes extends ResultPropertyImplicits {
  import org.scalacheck.Prop
  import org.specs2.execute.Success
  import scala.language.implicitConversions

  implicit def scopeToProp(u: ⇒ Scope): Prop = { u; resultProp(Success()) }
}

I did suggest your Arbitrary approach (thanks for the suggestion), but it seemed like it doesn't work so well (less DRY-ey) when you have a bunch of mocks with interactions that need to be configured, and a system under test which needs each of those mocks.

caspark commented 9 years ago

Specs2 v3.x equivalent:

/**
 * Allow using specs2 scopes inside scalacheck blocks; workaround for https://github.com/etorreborre/specs2/issues/263
 * This is only safe to mix in for mutable specifications.
 */
trait ScalaCheckScopes extends AsResultProp {
  //importing only into this scope because it doesn't really belong to the rest of the spec
  import org.scalacheck.Prop
  import org.specs2.execute.Success
  import scala.language.implicitConversions

  implicit def scopeToProp(u: ⇒ Scope): Prop = { u; asResultToProp(Success()) }
}
gmalouf commented 9 years ago

@caspark there appears to be no difference between what you posted in 2014 and the one from April 2015 - is there intended to be?

caspark commented 9 years ago

Oops. Fixed now; it's basically extend org.specs2.scalacheck.AsResultProp instead of org.specs2.matcher.ResultPropertyImplicits, and call asResultToProp() instead of resultProp().