disneystreaming / weaver-test

A test framework that runs everything in parallel.
https://disneystreaming.github.io/weaver-test/
Other
442 stars 49 forks source link

cats-testkit/discipline integration #187

Closed kubukoz closed 3 years ago

kubukoz commented 3 years ago

Something that would be very cool to have - shouldn't take a lot of code, and will make it possible to test laws with Discipline & Weaver.

Baccata commented 3 years ago

In all frankness, I'm not too sure about this one. The folks who usually go through the discipline (see what I did there 😉) of checking that their typeclass implementations are lawful are usually handy enough to write the 10-20 locs it takes to get bespoke discipline integration, and I'm wary about adding yet another dependency when our build matrix is already nearing 100 artifacts.

I've been wanting to have a cookbook at some point though, I wouldn't mind having the snippet in there.

kubukoz commented 3 years ago

I guess it could be a separate microlibrary then :) personally if I had more than a couple repositories to maintain I wouldn't want to copy-paste the 10-20 lines anyway.

keynmol commented 3 years ago

I'm interested in what we can do that cannot be offered by existing discipline integrations. Weaver shines when it comes to resources, power assertions, and parallelism - so if we can think of something that it can do for law testing - that'd be awesome.

We've been considering an idea of a contrib module (which we can build separately and not add anymore time to our huge builds), but I think it has to be worth it.

I myself am working on a weaver <-> Natchez integration in a personal project, but if it's deemed viable and doesn't break CI times, I will happily donate it to weaver contrib.

kubukoz commented 3 years ago

I'm interested in what we can do that cannot be offered by existing discipline integrations

My main motivation was to just be able to have a single test framework for everything, without the need to pull in any Scalatest modules onto the classpath etc.

Also, I suppose controlling the level of concurrency in a functional fashion would be nice to have - but this is just a random thing that I haven't even yet checked belongs in the integration (no idea how much of this is controlled by scalacheck)...

We've been considering an idea of a contrib module (which we can build separately and not add anymore time to our huge builds), but I think it has to be worth it.

I think once we have something we can decide whether we want to put it into such a module :) publishing a library is pretty straightforward nowadays (at least for unsigned snapshot releases) so someone can start working on that independently. And then we see if it belongs in a contrib module :)

keynmol commented 3 years ago

Something I didn't realise (but should've expected), is that discipline integrations are living under typelevel, not under original testing frameworks.

Gabriel actually experimented with it and the basic integration, like with other frameworks, is several lines.

So I'd say if there were to be a discipline integration, it should start as a separate project (there's a lot of space to explore :)) and it can then be rolled under typelevel org, just like the rest of them :)

kubukoz commented 3 years ago

Looks reasonable, I even know someone who can create typelevel repos...

gvolpe commented 3 years ago

BTW that initial implementation was broken, it does not report failures properly. This one here works accurately:

package suite

import org.scalacheck.Test
import org.scalacheck.Test.{ Passed, Proved }
import org.typelevel.discipline.Laws
import weaver.SimpleIOSuite

trait DisciplineSuite extends SimpleIOSuite {

  override def maxParallelism = 1

  def checkAll(name: String, ruleSet: Laws#RuleSet): Unit =
    ruleSet.all.properties.toList.foreach {
      case (id, prop) =>
        pureTest(s"$name: $id") {
          expect {
            Test.check(prop)(identity).status match {
              case Passed | Proved(_) => true
              case _                  => false
            }
          }
        }
    }

}

It's been tested in the gvolpe/pfps-shopping-cart repo :)

kubukoz commented 3 years ago

See, it's not that trivial :trollface:

Baccata commented 3 years ago

Alright alright, I'll merge it if either of you PR-it in 😄

kubukoz commented 3 years ago

I'm not saying it needs to live in this repository, just that there's a point in having it as a publishable thing so folks can easily reuse ;)

I can do either when my OSS queue frees up... I estimate it for around June 2026.

Baccata commented 3 years ago

I'm not saying it needs to live in this repository

Considering we're not currently guaranteeing bincompat (we'll start shortly after Scala 3 final), I can't honestly advise for it to live elsewhere at this point in time.

keynmol commented 3 years ago

I'm going to PR @gvolpe's implementation with some tests later today - as a separate discipline module, which will be listed on the integrations doc page.

We can have a chat on the PR - I don't have experience with discipline myself, but from existing integrations I don't see much room for increasing usability, it seems pretty straightforward.

Baccata commented 3 years ago

You can have a look at the scalacheck integration (via mix-in trait) to get some sense of how to make the discipline integration generic (wrt effect types)

gvolpe commented 3 years ago

That'd be awesome @keynmol , thanks! If that's part of an official module, I'd probably make it a bit more configurable, e.g. take a TestParameters as an argument instead of passing identity.

Baccata commented 3 years ago

Courtesy of Anton : https://disneystreaming.github.io/weaver-test/docs/discipline

gvolpe commented 3 years ago

Awesome! Thanks @keynmol , I can remove some code now :)