Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support properties that return Future[Result] #146

Open
mpilquist opened this issue Feb 22, 2020 · 16 comments · May be fixed by #147
Open

Support properties that return Future[Result] #146

mpilquist opened this issue Feb 22, 2020 · 16 comments · May be fixed by #147

Comments

@mpilquist
Copy link
Contributor

This would include a Property.check(...): Future[Report] function. Scalacheck doesn't support this but ScalaTest 3.2's built-in property testing support does. It's really useful when writing effectful tests that run on Scala.js, where you can't await futures inside the properties.

@charleso
Copy link
Collaborator

I was waiting for someone to ask for this. Just to confirm, is the biggest reason related to Scala.js? I'm curious, is Scala Hedgehog working in Scala.js for you at the moment, or you're just interested in the possibility?

The other new Scala property testing library with integrated shrinking is zio-test which is obviously heavily based around a more IO like computation. I'm not sure if it works on Scala.js though.

@mpilquist
Copy link
Contributor Author

Yeah, biggest reason is Scala.js, where we can't sneak a call in to Await.result. Scala.js support should be easy to add, though it would add some build complexity.

I'm looking at moving some of my projects from ScalaTest + ScalaCheck to munit + hedgehog. In particular, fs2 uses the ScalaTest 3.2 generators so that we can write property tests that return IO[Assertion]. We have some bridge code to convert from IO to Future.

@steinybot
Copy link
Contributor

Using Await.result by itself isn't ideal either since you don't get the logs on the result when there is an exception.

I have been using this:

import com.lightbend.console.hedgehog.implicits.FutureImplicits.FutureSyntax
import hedgehog.Result

import scala.concurrent.duration._
import scala.concurrent.{Await, Future, TimeoutException}
import scala.util.{Failure, Success, Try}
import TryImplicits._
import OptionImplicits._

trait FutureImplicits {

  implicit def durationInt(n: Int): DurationConversions = new DurationInt(n)

  implicit def durationLong(n: Long): DurationConversions = new DurationLong(n)

  implicit def durationDouble(n: Double): DurationConversions = new DurationDouble(n)

  implicit def futureSyntax[A](future: Future[A]): FutureSyntax[A] = new FutureSyntax(future)
}

object FutureImplicits {

  implicit class FutureSyntax[A](private val future: Future[A]) extends AnyVal {

    def readyWithin(atMost: Duration): Result =
      Try(Await.ready(future, atMost)) match {
        case Failure(_: TimeoutException) =>
          Result.failure.log(s"Future was not ready within ${atMost.toString}")
        case Failure(exception) =>
          Result.failure
            .log("Error occurred waiting for future to be ready")
            .log(TryImplicits.stackTrace(exception))
        case _: Success[_] => Result.success
      }

    def successfulResultWithin(atMost: Duration, f: A => Result): Result =
      readyWithin(atMost).and(future.value.isSome(_.successfulResult(f)))

    def failedResultWithin(atMost: Duration, f: Throwable => Result): Result =
      readyWithin(atMost).and(future.value.isSome(_.failedResult(f)))
  }
}

and then:

  private def testGetMetrics: Result = {
    val reply = client.getMetrics(GetMetricsRequest(query = "up"))
    reply.successfulResultWithin(LongTimeout, _.success ==== true)
  }

@mpilquist
Copy link
Contributor Author

@steinybot Unfortunately, that technique suffers from the same problem as Await.result when running on Scala.js -- since there's only 1 thread of execution, the call to Await.{result,ready} will fail.

Would the approach to fixing this be changing GenT to be: case class GenT[F[_], A](run: (Size, Seed) => F[Tree[(Seed, Option[A])]]) and changing Gen to type Gen[A] = GenT[Id, A]?

@charleso
Copy link
Collaborator

Would the approach to fixing this be changing GenT to be: case class GenT[F[_], A]

That's actually what it used to be :)

#70

Even though that's what the Haskell version does, and it might solve the problem, I think I would vote/recommend that we take a slightly different approach. In particular I think mixing Future-ish things in with GenT can have unintended side-effects.

hedgehogqa/haskell-hedgehog#122

What I've been thinking about is being explicit about converting the value inside of PropertyT to Result. For the current case we can just use sometin

 final def takeSmallest[F[_]: Monad](n: ShrinkCount, slimit: ShrinkLimit, t: Tree[Option[(List[Log], Option[F[Result]])]]): F[Status] 

def report[F[_]: Monad](config: PropertyConfig, size0: Option[Size], seed0: Seed, p: PropertyT[F[Result]]): F[Report] = {

// Default version
def check(config: PropertyConfig, p: PropertyT[Result], seed: Seed): Report =
   propertyT.report(config, None, seed, p)

// The one you want:
def checkF[F[_]: Monad](config: PropertyConfig, p: PropertyT[F[Result]], seed: Seed): F[Report] =
    propertyT.report(config, None, seed, p)

Or something like that. That should actually be relatively easy to do, with less impact, and I think avoids all the complications of having a generic F for the Gen/Property monad.

Does that make sense? What do you think?

@steinybot
Copy link
Contributor

steinybot commented Feb 28, 2020

I started playing around with this idea here https://github.com/steinybot/scala-hedgehog/tree/future-result. It is a bit broken at the moment but it shouldn't be too hard to fix.

@mpilquist
Copy link
Contributor Author

@steinybot Nice! I started something today too: https://github.com/hedgehogqa/scala-hedgehog/compare/master...mpilquist:wip/effectful-props?expand=1

@steinybot
Copy link
Contributor

It would be nice to have support in the runner for a result: => PropertyT[F[Result]]. Or would it have to be the concrete type such as result: => PropertyT[Future[Result]]?

What would this look like for Scala.js? Would having a Scala.js runner library make sense?

@mpilquist
Copy link
Contributor Author

In my case, I'd be using cats.effect.IO and integrating it with munit -- basically building off PropertySuite here: https://github.com/scodec/scodec-bits/blob/topic/munit/core/shared/src/test/scala/scodec/bits/PropertySuite.scala

I haven't thought too much about runner support as I only use hedgehog via other testing frameworks.

@steinybot steinybot linked a pull request Mar 1, 2020 that will close this issue
6 tasks
@steinybot
Copy link
Contributor

I made some more progress on this. The main thing left to do is decide what to do regarding stack safety. There are more details on the draft PR.

@charleso
Copy link
Collaborator

charleso commented Mar 2, 2020

Amazing!!! Michael's branch looks almost like what I had in mind, but I hadn't thought about the consequences of needing stack safety now. Sigh.

@charleso
Copy link
Collaborator

charleso commented Mar 2, 2020

It would be nice to have support in the runner for a result: => PropertyT[F[Result]]

Yeah I did wonder what we do with the runner. You could "capture" the Monad for each Test, and you'd also need a separate lawless typeclassy/interface thing to evaluate (F[A] => A) the final result. I don't know how this would then play in to having multiple tests of the same F type though, would you want a single evaluator for each file? What happens if they have different Fs.

I'm still not really sure what to do with my half-baked runner. I won't lie, I vaguely wish "tests" in Scala were just wired up from a single main class (like in Haskell) and the consumers can decide.

I'm open to suggestion? Do people find runner useful?

@steinybot
Copy link
Contributor

I use a modified version of the runner (which I hope to open source soon) because I have added stackable traits to support a before/after style and another for scalamock which needs to wrap the test with behaviour verification. I essentially extended Properties with a Runner trait that defines runTests and runTest and I changed main to call these. I did the same sort of thing with the sbt plugin.

It would be interesting to play around with different options and see how it works out.

@mpilquist
Copy link
Contributor Author

I'm not certain we need stack safety. Non-identity effects will bring their own stack safety (e.g. Future and IO). We just need to make sure the default check function uses a stack safe effect like Function0. Thoughts?

@steinybot
Copy link
Contributor

I didn't really look into it much at the time. I was getting StackOverflowErrors from Id but yeah I think just making sure that the ones that are actually used are safe should be good enough.

Sorry for the slackness on this, I plan to pick this back up soon 🤞.

@steinybot
Copy link
Contributor

I did take another look at this last week but I didn't get very far. I was having a hard time finding what is actually causing the StackOverflow's since they are thrown within all the composed functions and not the place that is doing the composition. Anyone have tips on how to deal with this?

There has been a bit of "turbulence" in my work situation at the moment so I am likely to be putting my spare time into a commercial project before this one sadly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants