Skip to content
This repository has been archived by the owner on Feb 24, 2021. It is now read-only.

Fix several tests relying on no-op assertion #46

Merged
merged 44 commits into from
Mar 5, 2020
Merged

Conversation

aballano
Copy link
Member

@aballano aballano commented Feb 25, 2020

BREAKING: Make equalUnderTheLaw throw an exception when the evaluation is false. With this we want to make sure that all tests are verifying something, otherwise non-boolean tests (all tests not using forAll/Few) can naively call equalUnderTheLaw at the end but fail to remember to verify the condition as the name doesn't suggest it. This therefore caused different kind of failures:

  • Some tests failed when trying to assert over a false equalUnderTheLaw, either with a boolean condition or with shouldBe -> Simply fixed them by using a more appropriate assertion
  • Some others failed either due to a problem with how the test was asserting or due to a bug in the implementation.

All changes are explained below:

  • Changed equalUnderTheLaw to rely on shouldBeEq and throw on failure
  • Update Mvar read and take docs to explicitly detail if the value is removed or not
  • Fix EqTest double verification causing it to fail
  • Fix semaphore test wrongly calling acquire
  • Fix weird stack overflow due to toString implementation of IO. This is likely caused by the shouldBeEq impl as it tries to print the values after the assertion and it causes a stack overflow due to the chained stack.
  • Fix FreeC tests failing due to data classes providing toString.
  • Refactor KindConnectionTests to split some tests, remove duplicates and fix some IO comparisons
  • Refactored Resource to use an internal state machine like IO, in order to be able to do tail recursion over its bindings to avoid stack overflows.
  • Simplify stack overflow test for MVar

…ures

Update Mvar read and take docs to explicitly detail if the value is removed or not
Co-Authored-By: Simon Vergauwen <nomisRev@users.noreply.github.com>
Copy link
Member

@nomisRev nomisRev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel we should change the implementation of equalUnderTheLaw instead of changing all the tests, they all suffer from the same issue that resulted in this fix.

@aballano aballano marked this pull request as ready for review February 26, 2020 09:54
@@ -18,13 +18,13 @@ fun throwableEq() = Eq { a: Throwable, b ->
data class Law(val name: String, val test: suspend TestContext.() -> Unit)

fun <A> A.equalUnderTheLaw(b: A, eq: Eq<A>): Boolean =
eq.run { eqv(b) }
shouldBeEq(b, eq).let { true }
Copy link
Member Author

@aballano aballano Feb 26, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems that this change unveiled some tests that weren't doing any verification with forAll or shouldBe and were failing 😓 cc @1Jajen1 @nomisRev

(see below)

@aballano
Copy link
Member Author

aballano commented Feb 26, 2020

Current test status:

  • arrow.fx.ResourceTest > Monad Laws: stack safe FAILED
  • arrow.fx.MVarTest > CancelableMVar - stack overflow test FAILED
  • arrow.fx.MVarTest > CancelableMVar - producer-consumer parallel loop FAILED
  • arrow.fx.MVarTest > CancelableMVar - concurrent take and put FAILED
  • arrow.fx.MVarTest > UncancelableMVar - concurrent take and put FAILED
  • arrow.fx.MVarTest > UncancelableMVar - stack overflow test FAILED
  • arrow.fx.MVarTest > UncancelableMVar - producer-consumer parallel loop FAILED
  • arrow.fx.IOTest > MonadDefer laws: stack safety over repeated left binds FAILED
  • arrow.fx.IOTest > MonadDefer laws: stack safety over repeated attempts FAILED
  • arrow.fx.SemaphoreTest > CancelableSemaphore - tryAcquire with available permits FAILED
  • arrow.fx.SemaphoreTest > UncancelableSemaphore - tryAcquire with available permits FAILED
  • arrow.fx.EqTest > Should fail for pure non-equal values FAILED
  • arrow.fx.KindConnectionTests > uncancelable.pop FAILED

Cleanup test by generating EQ based on EqK
Rework Resource to have different states depending on constructor, similar as IO
Add fold function to do tailrec recursion over Bind chains
Make use function depend on fold
Rework tailRecM function to operate over the different states, fixing the stack overflow issue
@aballano aballano linked an issue Feb 27, 2020 that may be closed by this pull request
@aballano aballano changed the title Fix MVar tests Fix several tests relying on no-op assertion Feb 27, 2020
@aballano aballano added the bug Something isn't working label Feb 27, 2020
@aballano aballano added this to the 0.10.x milestone Feb 27, 2020
Copy link
Contributor

@danimontoya danimontoya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Just need to check the comment about CancelableMVar

# Conflicts:
#	arrow-fx/src/main/kotlin/arrow/fx/internal/CancellableMVar.kt
#	arrow-fx/src/test/kotlin/arrow/fx/KindConnectionTests.kt
Copy link
Member

@nomisRev nomisRev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks awesome! Great work! invoke cannot be removed for 0.10.x, approved to unblock.

arrow-fx/src/main/kotlin/arrow/fx/Resource.kt Show resolved Hide resolved
* A Map which tracks the insertion order of entries, so that entries may be
* traversed in the order they were inserted.
*/
internal class LinkedMap<K, V>(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use LinkedHashMap from kotlin.collections instead?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope AFAIK :D Actually the current MVar has already that kind of list

override fun <A> genK(gen: Gen<A>): Gen<Kind<ResourcePartialOf<ForIO, Throwable>, A>> =
Gen.oneOf(
gen.map { just(it, IO.bracket()) },
IO.genK().genK(gen).map { it.liftF(IO.bracket()) }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here you can see how the Gen is build for Resource in Cats effect.

https://github.com/typelevel/cats-effect/blob/master/laws/shared/src/main/scala/cats/effect/laws/discipline/arbitrary.scala#L118

It build Allocate from Gen<Kind<F, A>> and Gen<Kind<F, Unit>>, it builds FlatMap from the allocate Gen and another for Suspend.

The idea here is that if your Kind<F, A> for example IO<A> is a strong generator that also generates IO at depths etc so it can cover all its cases.

@@ -300,3 +300,7 @@ internal fun <A> asyncContinuation(ctx: CoroutineContext, cc: (Either<Throwable,
*/
internal fun <A> IOForkedStart(fa: IOOf<A>, ctx: CoroutineContext): IO<A> =
IO.Bind(IO.ContinueOn(IO.unit, ctx)) { fa.fix() }

@Suppress("UNCHECKED_CAST")
internal fun <A, B> A.unsafeCast(): B =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm personally not a fan of this, and I think we should be consistent in the Arrow Fx codebase when doing this.

Currently it's hard coded casting everywhere, which I prefer so you can more explicitly see the types throughout the code (which is not easy to read or follow).

cc\ @raulraja @pakoito

onOutput: (A) -> Kind<F, B>,
onRelease: (Kind<F, Unit>) -> Kind<F, Unit>
): Kind<F, B> {
// Interpreter that knows how to evaluate a Resource data structure
// Maintains its own stack for dealing with Bind chains
tailrec fun loop(current: Resource<F, E, Any>, stack: List<(Any) -> Resource<F, E, Any>>): Kind<F, Any?> =
tailrec fun loop(current: Resource<F, E, A>, stack: List<(A) -> Resource<F, E, A>>): Kind<F, B> =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😍

Copy link
Member

@nomisRev nomisRev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome work @aballano!!! 👏 👏 👏

@aballano aballano merged commit e9b07cf into master Mar 5, 2020
@aballano aballano deleted the ab/fix-mvar-tests branch March 5, 2020 16:19
@aballano aballano linked an issue Mar 5, 2020 that may be closed by this pull request
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working
Projects
None yet
3 participants