Skip to content
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

Cross-platform suite-local fixtures #104

Merged
merged 6 commits into from
Sep 16, 2021
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions ce2/shared/src/main/scala/munit/CatsEffectSuite.scala
Original file line number Diff line number Diff line change
Expand Up @@ -47,4 +47,11 @@ abstract class CatsEffectSuite
{ case e: SyncIO[_] => Future(e.unsafeRunSync())(munitExecutionContext) }
)

private[munit] def unsafeRunAndForget[A](ioa: IO[A]): Unit = ioa.unsafeRunAsyncAndForget()
danicheg marked this conversation as resolved.
Show resolved Hide resolved

}

object CatsEffectSuite {
type Deferred[F[_], A] = cats.effect.concurrent.Deferred[F, A]
armanbilge marked this conversation as resolved.
Show resolved Hide resolved
val Deferred = cats.effect.concurrent.Deferred
}
7 changes: 7 additions & 0 deletions ce3/shared/src/main/scala/munit/CatsEffectSuite.scala
Original file line number Diff line number Diff line change
Expand Up @@ -44,4 +44,11 @@ abstract class CatsEffectSuite
{ case e: SyncIO[_] => Future(e.unsafeRunSync())(munitExecutionContext) }
)

private[munit] def unsafeRunAndForget[A](ioa: IO[A]): Unit = ioa.unsafeRunAndForget()

}

object CatsEffectSuite {
type Deferred[F[_], A] = cats.effect.kernel.Deferred[F, A]
val Deferred = cats.effect.kernel.Deferred
}
Original file line number Diff line number Diff line change
Expand Up @@ -16,4 +16,4 @@

package munit

trait CatsEffectFixtures { self: CatsEffectSuite => }
trait CatsEffectFixturesPlatform { self: CatsEffectSuite => }
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ package munit

import cats.effect.{IO, Resource}

trait CatsEffectFixtures { self: CatsEffectSuite =>
trait CatsEffectFixturesPlatform { self: CatsEffectSuite =>

object ResourceSuiteLocalFixture {

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ package munit

import cats.effect.{IO, Resource}

class CatsEffectFixturesSpec extends CatsEffectSuite with CatsEffectAssertions {
class CatsEffectFixturesPlatformSpec extends CatsEffectSuite with CatsEffectAssertions {

var acquired: Int = 0
var released: Int = 0
Expand Down
44 changes: 44 additions & 0 deletions common/shared/src/main/scala/munit/CatsEffectFixtures.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
/*
* Copyright 2021 Typelevel
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package munit

import cats.effect.{IO, Resource}

trait CatsEffectFixtures extends CatsEffectFixturesPlatform { self: CatsEffectSuite =>

import CatsEffectSuite.Deferred

object ResourceSuiteLocalDeferredFixture {

def apply[T](name: String, resource: Resource[IO, T]): Fixture[IO[T]] =
new Fixture[IO[T]](name) {
val value: Deferred[IO, (T, IO[Unit])] = Deferred.unsafe

def apply(): IO[T] = value.get.map(_._1)

override def beforeAll(): Unit = {
val resourceEffect = resource.allocated.flatMap(value.complete)
unsafeRunAndForget(resourceEffect)
}

override def afterAll(): Unit = {
unsafeRunAndForget(value.get.flatMap(_._2))
}
}
}

}
61 changes: 61 additions & 0 deletions common/shared/src/test/scala/munit/CatsEffectFixturesSpec.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
/*
* Copyright 2021 Typelevel
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package munit

import cats.effect.{IO, Resource}

class CatsEffectFixturesSpec extends CatsEffectSuite with CatsEffectAssertions {

@volatile var acquired: Int = 0
@volatile var released: Int = 0

val fixture = ResourceSuiteLocalDeferredFixture(
"fixture",
Resource.make(
IO {
acquired += 1
()
}
)(_ =>
IO {
released += 1
()
}
)
)

override def munitFixtures = List(fixture)

override def beforeAll(): Unit = {
assertEquals(acquired, 0)
assertEquals(released, 0)
}

override def afterAll(): Unit = {
assertEquals(acquired, 1)
// assertEquals(released, 1) // Release is async, no way to check?

Choose a reason for hiding this comment

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

Yeah this is really my only question here. since there's no way to enforce backpressure between the release of the suite-local fixture and the acquisition of the next, you could run into some weird interactions between different suites. for example, if your fixture is a network server that binds to the same port. suite A might not release its port before the suite B attempts to acquire

I think this is still really useful for javascript to have, so this may just need to be a tradeoff that we warn users about

Copy link
Member Author

Choose a reason for hiding this comment

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

I think this is still really useful for javascript to have, so this may just need to be a tradeoff that we warn users about

I'm mindful of this issue too, but exactly as you said. For plenty of non-interacting resources should be fine.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's particularly evil, but this problem could be solved via some sort of global semaphore right?

Choose a reason for hiding this comment

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

That sounds like it should work, but it would inhibit parallel suite execution (only the suites which use this feature) for users who opt in. which again may be fine and just something we warn users about

the fixtures do have names, so ideally you could split the global semaphore by name, but that relies on the assumption that interacting resources have the same name

Copy link
Member Author

@armanbilge armanbilge Jul 11, 2021

Choose a reason for hiding this comment

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

Right, but on JS we don't have parallel execution anyway. So maybe the trick is to use this implementation for JS (plus some sort of global semaphore), your blocking implementation for JVM, and just provide them under the same interface?

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe we should call it UnsafeResourceSuiteLocalFixture to emphasize these hazards.

Copy link
Member

Choose a reason for hiding this comment

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

Im still a bit confused :)

We have a working ResourceSuiteLocalFixture for the JVM only. As far as I understand, you want to add similar functionality but for JS?

I don't really know anything about ScalaJS and how to create libraries for it, but I don't get why we need to create something that works on both the JVM and JS?
Because, this shared logic on the JVM, doesn't really work as one would expect. And since we already have something that works the JVM, why add something shared?

So in my mind, that has no JS knowledge, can this logic simply be added to the JS part of this library? Or is there some value in having something shared?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good questions!

Or is there some value in having something shared

Yes, absolutely. If it is not shared, then if you want to test your code on both JVM and JS you'd have to write your tests twice, separately for each platform. For a concrete example of this consider the http4s test suite which now needs to support both JVM and JS platforms where in fact I've already been using this construct:
https://github.com/http4s/http4s/blob/main/testing/shared/src/test/scala/org/http4s/Http4sSuite.scala#L68

Copy link
Member

Choose a reason for hiding this comment

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

Then I would indeed go for the UnsafeResourceSuiteLocalFixture :)

Does the CatsEffectFunFixtures not give you the functionality you desire?

Copy link
Member Author

Choose a reason for hiding this comment

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

CatsEffectFunFixtures does work on JVM and JS, but it allocates/tears down the resource once per test. The SuiteLocal fixtures allocate/tear down once per suite.

}

test("first test") {
fixture().assertEquals(())
}

test("second test") {
fixture().assertEquals(())
}

}