-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Replace instance trait inheritance with imports in tests #3304
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3304 +/- ##
==========================================
+ Coverage 93.31% 93.32% +0.01%
==========================================
Files 378 378
Lines 7689 7689
Branches 206 206
==========================================
+ Hits 7175 7176 +1
+ Misses 514 513 -1
Continue to review full report at Codecov.
|
43e6e45
to
f612a30
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think more normal library usage is to just import cats.implicits._
rather than à la carte syntax, so that goes a bit against the "test it how people use it" principle. Then again, you found bugs. I wonder whether we'd find more with the omnibus import, but I can't think of a convenient way to test both.
@rossabaker I agree that there are trade-offs here. In my view moving away from inheritance for these instances is a major improvement in this respect (tests reflecting usage), and à la carte vs. kitchen sink is a smaller matter. I'd also prefer to use kitchen sink here, if we had decent tests for the à la carte style, but unfortunately we don't. There's also the question about what the recommended style should be after #3043. At least initially in 2.2, |
@rossabaker Switching to kitchen sink wouldn't be all that hard, though. We'd have to reinstate |
@rossabaker Oh, I almost already did that—I've just rebased to make it a little easier to isolate the kitchen-sink-to-à-la-carte change. If you check out 8dd80e5 ("Replace instance trait inheritance with imports in tests") you'll get more or less the kitchen sink version (although not When it's green I'll merge the three rebased commits without squashing in case we want to revisit this in the future (the changes are identical to what was reviewed, just reordered slightly). |
Ugh, sorry, will need one last 👍 after the rebase. @LukaJCB, mind giving it one more? 😄 |
This PR does several things to clean up the Cats tests (the first addresses #3262):
cats
/alleycats
into scope.cats.tests
and others incats.free
).The issue is that the tests are currently set up in a way that saves a few imports, but at the expense of not exercising normal library usage (see #3262 for an example of this causing problems).
After this change the tests do a better job of reflecting what users actually do (e.g. they don't nest stuff inside
package cats
and they don't extend a giant stack of 14AllInstancesBinCompatX
traits).Compile times
Speeding up test compilation was a non-goal for this change, but I was curious and it seems a little faster (consistently 76s vs. 82s for
testsJVM/test:compile
aftertestsJVM/test:clean
on 2.12 on my desktop).The WIP partUpdate: the
catsSyntaxEq
"override" did work just fine, even after switching to imports, but I went ahead and changed to more granular imports forcats.syntax
anyway, since trying it out turned up two existing bugs (fixed separately in #3305 and #3306). I don't see any point in switching to granular imports forcats.instances
since these will probably be gone entirely very soon.I'm opening this so I don't lose track of it, but I still want to make sure the change doesn't cause problems with respect to the===
situation (previously we were overridingcatsSyntaxEq
to make it non-explicit, and now might need to change all of thecats.syntax.all
imports to exclude it instead).There's also one test class (
AlgebraInvariantSuite
) where some function instances needed in the tests weren't resolving correctly when I switched to imports, so I'm still using the instance traits there. I'll try to take a closer look at it, but I think it's something specific tocats.laws.discipline.eq
and I'm not too worried about it.