More coverage #1752
More coverage #1752
Conversation
edmundnoble
force-pushed
the
more-coverage
branch
from
9f03a71
to
eaeb096
Compare
Codecov Report
@@ Coverage Diff @@
## master #1752 +/- ##
==========================================
+ Coverage 94.17% 95.41% +1.23%
==========================================
Files 256 256
Lines 4208 4209 +1
Branches 93 95 +2
==========================================
+ Hits 3963 4016 +53
+ Misses 245 193 -52
Continue to review full report at Codecov.
|
|
Seems there is a bit of overlap with what I did in #1623. |
edmundnoble
force-pushed
the
more-coverage
branch
3 times, most recently
from
e2da87f
to
9aa695d
Compare
| Monoid[WriterT[Id, Int, Int]] | ||
| Semigroup[WriterT[Id, Int, Int]] | ||
| Monoid[Writer[Int, Int]] | ||
| checkAll("Writer[Int, Int]", kernel.laws.GroupLaws[Writer[Int, Int]].monoid) |
There was a problem hiding this comment.
Should we add SerializableTests for the Monoid instance of WriterT?
And for the Semigroup instance below as well?
| @@ -1,7 +1,8 @@ | |||
| package cats | |||
| package tests | |||
|
|
|||
| import cats.data.IdT | |||
| import cats.{Foldable, Functor, Monad, NonEmptyTraverse, Traverse} | |||
There was a problem hiding this comment.
Is there a reason we add these imports explicitly?
There was a problem hiding this comment.
I didn't have the multiple package decls in a previous version of the file, IntelliJ magic. Fixed.
| { | ||
| import cats.implicits._ |
There was a problem hiding this comment.
Should we maybe include a comment specifying that the instances below should be found when all the instances are in scope?
It might not be as obvious what we are doing here, now that we moved to CatsSuite.
There was a problem hiding this comment.
I'm not sure, it seems like all other transformers do not have or need such a comment.
| @@ -10,6 +10,7 @@ class EitherTests extends CatsSuite { | |||
| implicit val iso = CartesianTests.Isomorphisms.invariant[Either[Int, ?]] | |||
|
|
|||
| checkAll("Either[String, Int]", GroupLaws[Either[String, Int]].monoid) | |||
| checkAll("Semigroup[Either[String, Int]]", SerializableTests.serializable(Semigroup[Either[String, Int]])) | |||
There was a problem hiding this comment.
Maybe check Monoid[Either[String, Int]] here?
The instance tested should be the same, but might be clearer given that we are testing GroupLaws.monoid above?
edmundnoble
force-pushed
the
more-coverage
branch
from
28da04d
to
bf30f37
Compare
edmundnoble
force-pushed
the
more-coverage
branch
from
bf30f37
to
6da44b9
Compare
| Invariant[PartialOrder] | ||
| Contravariant[PartialOrder] | ||
| } | ||
|
|
||
| { | ||
| test("companion object syntax") { |
There was a problem hiding this comment.
seems like this could be a scalacheck.
There was a problem hiding this comment.
Excellent point, corrected.
|
Merging with two sign-offs, assuming my latest edit was sufficient for the last comment. |
Calculated hail mary to increase coverage, to somewhat balance out the MTL removal coverage decrease.