Add stackUnsafeMonad to MonadTests

[travisbrown]

As discussed on Gitter. The basic idea is to let people easily test all of the laws for their monad instances except tailRecM's stack safety, if for whatever reason they're not motivated to make their tailRecM stack-safe.

It's not very DRY, but I didn't want to introduce a bunch of abstraction into test code, and all the instances would be a lot of noise.

[kailuowang]

👍 I also deem such a relaxed law helpful.

[non]

👍

[travisbrown]

Just restarted the tests since they'd failed immediately during yesterday's outages.

[codecov-io]

Current coverage is 91.70% (diff: 100%)

No coverage report found for master at 0f70acf.

Powered by Codecov. Last update 0f70acf...ff665f4

[travisbrown]

I guess it makes sense for us to measure test coverage of test code (?), but since we're explicitly not using this for law checking in Cats itself, I'm not sure what to do here.

[kailuowang]

I think we are fine with the coverage. @non wdyt?

[johnynek]

can we call this law as a parent of the normal monad laws (which include stack safety, I don't want them to get out of sync)?

Also, can we exercise them once on a monad that is stack safe?

[travisbrown]

@johnynek I was reluctant to use parent (or bases) here because there's no subtype relationship here and the Discipline docs are pretty precise about the intended use of those methods.

I originally had a shared RuleSet parent for the two, but with the type parameters and implicit arguments it tripled the size of the file instead of just doubling it, and didn't seem worth it.

About exercising them on a stack-safe monad—you mean just to have the code do something when tests run? Sure, I could add that and explain the duplication, but it still seems a little odd.

[johnynek]

Well, it's not that odd to me in that actually exercising this code seems like a good thing to do.

Strictly speaking we should test the laws by running them with instances that pass, and verifying they detect when instances fail. In that sense, I think the laws are actually somewhat untested since we are only checking for positives now (not negatives) if I'm not mistaken.

[travisbrown]

@johnynek I'm more worried about the person looking at the codebase for the first time, seeing checkAll("Eval[Int]", MonadTests[Eval].stackUnsafeMonad[Int, Int, Int]) or whatever, and thinking Eval doesn't have the same stack safety guarantees as other monads they've seen here.

If we want to be principled about exercising the laws (which I agree would be a good thing), I'm not sure that should happen in tests/src/test. Maybe laws/src/test?

[johnynek]

@travisbrown yeah. it should be in laws/src/test

[travisbrown]

@johnynek Okay, sounds good. When I get back to a computer I'll use stackUnsafeMonad there and will open an issue for thinking about checking negatives (but I don't think that should block 0.8.0).

[travisbrown]

Sorry, meant to rename something before pushing. Just a second and I'll amend that last commit…

[travisbrown]

@johnynek Added MonadTestsTests and opened #1423.

[johnynek]

👍

[non]

👍 (but looks like we need to resolve some conflicts)

[travisbrown]

@non Oh, right—I meant to do that after #1415 got merged. I've just rebased, so it should be ready to go on green.

[johnynek]

Couldn't you run the test for a safe monad to test the test?
On Sat, Oct 22, 2016 at 06:36 Travis Brown notifications@github.com wrote:

I guess it makes sense for us to measure test coverage of test code (?),
but since we're explicitly not using this for law checking in Cats itself,
I'm not sure what to do here.

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#1417 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAEJdty9ddKqcUGeiPGTGYA6CNwmiRhZks5q2juIgaJpZM4KdaUJ
.

[travisbrown]

@johnynek Right, that's what I'm doing in MonadTestsTests now.