Arrow: Stop using applicative/constructor functions to test typeclasses

Created on 26 Nov 2019  路  6Comments  路  Source: arrow-kt/arrow

Currently we have tests like this:
testLaws(cf: (Int) -> Kind<F, Int>) or similar testLaws(AP: Applicative<F>) for our tests. This is awful for several reasons:
cf has no source of randomness, hence the Kind<F, Int> will only ever differ in the content Int. That might not sound too bad for basic things like Id, but easily fails with something like Option, where we can only ever get Some. This is just bad and caused at least one instance where a test should have failed but did not: https://github.com/arrow-kt/arrow/pull/1816#discussion_r350476512
The AP approach has exactly the same problem.

A solution would be to instead take either cf: (Int) -> Gen<Kind<F, Int>> or cf: (Gen<A>) -> Gen<Kind<F, A>> as arguments. This can also be written as a typeclass like this pr did: https://github.com/abendt/arrow/blob/zip/modules/core/arrow-test/src/main/kotlin/arrow/test/generators/LiftGen.kt
(I'd change a few small things, but that is basically what I think is the best solution to the problem)

This should be rather easy to change in almost all places and will significantly improve the coverage of our tests.

bug help wanted noob friendly

Most helpful comment

cf has no source of randomness

Thank you @1Jajen1! This has already caused more false positives in the passed.
I'm sure there is more low hanging fruit in the tests. They definitely deserve more love for all the work they're doing.

All 6 comments

Thing is, None is tested through raiseError for typeclasses that support it. If we'd like to use ApplicativeError for anything above it on the typeclass hierarchy I'm not opposed.

cf has no source of randomness

Thank you @1Jajen1! This has already caused more false positives in the passed.
I'm sure there is more low hanging fruit in the tests. They definitely deserve more love for all the work they're doing.

Thing is, None is tested through raiseError for typeclasses that support it. If we'd like to use ApplicativeError for anything above it on the typeclass hierarchy I'm not opposed.

This can work for Option, Either and anything else that is a sum type of 2 with one error and one success case. But it would be wrong to do so in a generic setting when you want to test all cases.

Btw a quick overview on just how bad this is: https://github.com/arrow-kt/arrow/blob/master/modules/core/arrow-core-data/src/test/kotlin/arrow/core/OptionTest.kt
The only laws that actually test with None are Monoid and the recently added Align, EqK and Unalign. There are 7 more groups of laws that literally never see the None case.
And worst of all some the most common laws Eq, Show, Hash, the entire Monad spectrum etc. use this method of getting Kind<F, Int> to test.

Currently the test result that Option passes the Monad laws is just as good as Id passing because we are literally only ever testing a case that is isomorphic to id. And it's the same for all other monad tests. These tests cannot give any confidence. I am pretty sure most, if not all, data types will pass tests with higher coverage, but that is besides the point :)

I'm up for the plan. It is necessary to parametrize both the generator and either the result or the result checker, because tests like https://bit.ly/34pMCeu rely on checking against a positive case.

Those won't need changes as they are intended uses of just (or same deal with raiseError). This only applies when you need a generic value of Kind<F, A>

I think this can be closed in favor of the follow-up #1858 which addresses the leftovers after #1844 is merged.

Was this page helpful?
0 / 5 - 0 ratings

Related issues

pakoito picture pakoito  路  3Comments

Northburns picture Northburns  路  4Comments

JorgeCastilloPrz picture JorgeCastilloPrz  路  3Comments

vejeta picture vejeta  路  3Comments

jmfayard picture jmfayard  路  4Comments