Add law testing guide #1880

LukaJCB · 2017-08-31T17:36:57Z

Should resolve #1585. I'm thinking of creating another short section on why Laws are actually important, but for now this should be a good start I think.

Also I modified the build, so that docs depends on testkit and laws. Not sure, if this was the right way to do it, so feedback is very welcome :)

codecov-io · 2017-08-31T18:12:43Z

Codecov Report

Merging #1880 into master will increase coverage by 0.19%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1880      +/-   ##
==========================================
+ Coverage   94.97%   95.17%   +0.19%     
==========================================
  Files         241      248       +7     
  Lines        4199     4352     +153     
  Branches      106      126      +20     
==========================================
+ Hits         3988     4142     +154     
+ Misses        211      210       -1

Impacted Files	Coverage Δ
kernel/src/main/scala/cats/kernel/Order.scala	`86.48% <0%> (-2.71%)`	⬇️
...rc/main/scala/cats/laws/discipline/Arbitrary.scala	`92.06% <0%> (-0.13%)`	⬇️
...rnel/src/main/scala/cats/kernel/PartialOrder.scala	`88.23% <0%> (ø)`	⬆️
laws/src/main/scala/cats/laws/FoldableLaws.scala	`100% <0%> (ø)`	⬆️
...in/scala/cats/laws/discipline/ReducibleTests.scala	`100% <0%> (ø)`	⬆️
...a/cats/laws/discipline/NonEmptyTraverseTests.scala	`100% <0%> (ø)`	⬆️
core/src/main/scala/cats/instances/order.scala	`100% <0%> (ø)`	⬆️
core/src/main/scala/cats/data/NonEmptyVector.scala	`100% <0%> (ø)`	⬆️
core/src/main/scala/cats/instances/eq.scala	`100% <0%> (ø)`	⬆️
...e/src/main/scala/cats/instances/partialOrder.scala	`100% <0%> (ø)`	⬆️
... and 14 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d85cc86...c8c4328. Read the comment docs.

kailuowang · 2017-09-01T13:31:12Z

docs/src/main/tut/typeclasses/lawtesting.md

@@ -0,0 +1,146 @@
+# Law testing
+
+Laws are an important part of cats.


Laws are explained in the last section in this type class page, maybe make that section a linkable anchor and link from here?

Markdown embeds ids so we don't even need an anchor 🎉. Great suggestion, will do!

kailuowang · 2017-09-01T13:38:28Z

docs/src/main/tut/typeclasses/lawtesting.md

+Laws are an important part of cats.
+They help us enforce constraints that we usually assume to be true and give us guarantees that our code does what we think it does.
+Manually testing each data type to adhere to each of its type class instances can be quite cumbersome.
+Thankfully cats comes with the `cats-testkit`, which makes it a lot easier to write tests for type class instances using laws.


well, strictly speaking, catalysts and discipline are the two libraries to make it easier to "write tests for type class instances using laws.", cats-testkit is just a tiny lib on top of them to make it slightly more convenient to test against cats type classes using scalatests. relevant discussion https://gitter.im/typelevel/sbt-catalysts?at=59a85afdee5c9a4c5f1d45fb

That's true, what do you think of rewording to
"Thankfully cats comes with the cats-testkit, which is based on catalysts and discipline, both of which make it a lot easier to write law tests for type class instances."
? :)

I think, it's probably more accurate to break it into two sentences.
catalysts and discipline are the two libraries to make it easier to "write tests for type class instances using laws. cats-testkit makes it more convenient to test against cats type classes using scalatests. paging @BennyHill for better wording.

kailuowang · 2017-09-01T14:14:03Z

docs/src/main/tut/typeclasses/lawtesting.md

+
+```tut:invisible
+import org.scalacheck.{Arbitrary, Gen}
+implicit def arbFoo[A: Arbitrary]: Arbitrary[Tree[A]] =


so this is the one that is actually in use, not the one generated by scalacheck-shapeless?

Yeah, I realize it's not an optimal solution, but it's the best I came up with. If you have a better idea, please do tell :)

i think the spirit of our documentation is that people can just copy sample code to console and it runs. This breaks that assumption. why scalacheck-shapeless didn't work?

I wouldn't know how to include a foreign module for a tut doc 🤔. I guess it would be possible to add it as a dependency, but not sure if that's a good idea for just one part of the documentation?

I am inclined to not pretend to use it in the documentation. Make the arbitrary instance visible and add a note that you can use scalacheck-shapeless to auto derive this instance.
BTW, could we try avoid the asInstance in the arbitrary instance?

I got it to work by adding this single line to docs in the build.sbt:

.settings(libraryDependencies += "com.github.alexarchambault" %% "scalacheck-shapeless_1.13" % "1.1.5" % Compile)

tut then compiles the whole thing without a hitch.

I still prefer making this visible and mentioned the alternative solution from scalacheck-shapeless in a note.

kailuowang · 2017-09-01T14:31:54Z

docs/src/main/tut/typeclasses/lawtesting.md

+
+[Laws](https://typelevel.org/cats/typeclasses.html#laws) are an important part of cats.
+They help us enforce constraints that we usually assume to be true and give us guarantees that our code does what we think it does.
+Manually testing each data type to adhere to each of its type class instances can be quite cumbersome.


hmm, I think the wording here could use some polish. e.g. it's not testing data types, it's testing instances. I'd say for the sake of conciseness and time, how about we just say

[Laws](https://typelevel.org/cats/typeclasses.html#laws) are an important part of cats. Cats uses catalysts and dicipline to help test instances with laws.

then introduce cats-testkit

tpolecat · 2017-09-03T15:49:29Z

Hey I just saw this and it looks great. I had to figure this out a few weeks ago and it was non-obvious so this will be very helpful for people. 👍

ghost · 2017-09-03T16:40:51Z

build.sbt

@@ -181,7 +181,8 @@ lazy val docs = project
  .settings(noPublishSettings)
  .settings(docSettings)
  .settings(commonJvmSettings)
-  .dependsOn(coreJVM, freeJVM)
+  .settings(libraryDependencies += "com.github.alexarchambault" %% "scalacheck-shapeless_1.13" % "1.1.5" % Compile)


I would strongly discourage adding a third party dependency unless it is really essential. cats is quite low down the compile chain so the fewer dependencies it has that could get in the way of upgrades the better. And for thing like testing new scala compilers (eg 2.13.0.M1, native,etc) it is not the dependencies that a third party library brings into cats that is the issue, rather the dependencies needed to build that 3rd party library in the first place.

So I'm not saying it should not have any at all, but that we have to be careful.

As a starting suggestion/discussion.....

The same is true for testkit, that I have suggested we split into two: a testkit that has no dependencies other than pure cats, and another testkit-scalatest that adds the scalatest dependency. The idea here is that we make the the base testkit independent to the testing frameworks. But what about specs2, should we also add a testkit-specs2, testkit-utest and so on?

And what about, say, cats-check. For sure we can't add that as a dependency.

So maybe now is a good time to address this. Maybe we need some kind of uber-cats repo that brings these things together?

You raise a good point, I was hesitant to add the dependency before. I can revert the last commit and make the Arbitrary instance explicit with a mention to scalacheck-shapeless.

I agree we should be very cautious about adding dependencies to the project. In this particular case, it's a dependency added to the docs generation project, so no downstream impact. That's why I didn't object it. However, now I realized that it does impact the docs build in scala 2.13.

"Making the Arbitrary instance explicit with a mention to scalacheck-shapeless" would make more sense to me.

ghost · 2017-09-03T16:50:57Z

Sorry for the tardiness of the comment, my bad

adelbertc · 2017-09-03T17:11:53Z

Semi-unrelated to this commit but is there a reason why our kernel tests are structured differently from core and friends? I remember having an issue ticket about this a while ago but can't seem to find it at the moment. cc @johnynek

Other than that, guide looks good!

ghost · 2017-09-03T17:15:31Z

@adelbertc see #1272

ghost · 2017-09-03T19:40:04Z

docs/src/main/tut/typeclasses/lawtesting.md

+  checkAll("Tree[Int].MonoidLaws", GroupLaws[Tree[Int]].semigroup)
+  checkAll("Tree.FunctorLaws", FunctorTests[Tree].functor[Int, Int, String])
+}
+```


Note that with catalysts you can do something similar to how algebra/kernel calls the laws, which is something like:

... import catalysts.macros.TypeTagM // this can be added to the general test suite implicit def groupLaws[A: Eq: Arbitrary]: GroupLaws[A] = GroupLaws[A] .... laws[GroupLaws, Tree[Int]].check(_.semigroup)

Do you think this is worth mentioning? I'm not really sure if I can see the benefit 🤔

This reverts commit c0eba6c.

ceedubs

Thank you so much @LukaJCB! This is something that we've needed for a long time. 🎉

ceedubs · 2017-09-12T14:51:08Z

Do we need to add this to https://github.com/typelevel/cats/blob/master/docs/src/main/resources/microsite/data/menu.yml and/or anywhere else?

kailuowang · 2017-09-12T15:34:02Z

We may also want to link it from FAQ.

Add law testing guide

5a6eba3

kailuowang added the documentation label Aug 31, 2017

kailuowang reviewed Sep 1, 2017

View reviewed changes

Link to typeclasses law section

49096d4

kailuowang reviewed Sep 1, 2017

View reviewed changes

Luka Jacobowitz added 2 commits September 1, 2017 16:56

Simplify begining

1036a4c

Add scalacheck-shapeless as local dependency for law testing

c0eba6c

kailuowang approved these changes Sep 3, 2017

View reviewed changes

ghost reviewed Sep 3, 2017

View reviewed changes

Luka Jacobowitz added 2 commits September 4, 2017 15:14

Revert "Add scalacheck-shapeless as local dependency for law testing"

bb78377

This reverts commit c0eba6c.

Make Tree arbitrary instance visible

c8c4328

kailuowang approved these changes Sep 4, 2017

View reviewed changes

ceedubs approved these changes Sep 12, 2017

View reviewed changes

ceedubs merged commit cf544a0 into typelevel:master Sep 12, 2017

kailuowang added this to the 1.0.0-RC1 milestone Oct 13, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add law testing guide #1880

Add law testing guide #1880

LukaJCB commented Aug 31, 2017

codecov-io commented Aug 31, 2017 •

edited

Loading

kailuowang Sep 1, 2017 •

edited

Loading

LukaJCB Sep 1, 2017 •

edited

Loading

kailuowang Sep 1, 2017

LukaJCB Sep 1, 2017

kailuowang Sep 1, 2017

kailuowang Sep 1, 2017

LukaJCB Sep 1, 2017

kailuowang Sep 1, 2017 •

edited

Loading

LukaJCB Sep 1, 2017

kailuowang Sep 1, 2017

LukaJCB Sep 1, 2017

kailuowang Sep 4, 2017

kailuowang Sep 1, 2017

tpolecat commented Sep 3, 2017

ghost Sep 3, 2017

ghost Sep 3, 2017

LukaJCB Sep 4, 2017

kailuowang Sep 4, 2017

ghost commented Sep 3, 2017

adelbertc commented Sep 3, 2017

ghost commented Sep 3, 2017

ghost Sep 3, 2017

LukaJCB Sep 4, 2017

ceedubs left a comment

ceedubs commented Sep 12, 2017

kailuowang commented Sep 12, 2017

		@@ -0,0 +1,146 @@
		# Law testing

		Laws are an important part of cats.

Add law testing guide #1880

Add law testing guide #1880

Conversation

LukaJCB commented Aug 31, 2017

codecov-io commented Aug 31, 2017 • edited Loading

Codecov Report

kailuowang Sep 1, 2017 • edited Loading

Choose a reason for hiding this comment

LukaJCB Sep 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kailuowang Sep 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tpolecat commented Sep 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Sep 3, 2017

adelbertc commented Sep 3, 2017

ghost commented Sep 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ceedubs left a comment

Choose a reason for hiding this comment

ceedubs commented Sep 12, 2017

kailuowang commented Sep 12, 2017

codecov-io commented Aug 31, 2017 •

edited

Loading

kailuowang Sep 1, 2017 •

edited

Loading

LukaJCB Sep 1, 2017 •

edited

Loading

kailuowang Sep 1, 2017 •

edited

Loading