Cross-platform suite-local fixtures #104

armanbilge · 2021-07-11T17:43:50Z

This PR introduces a ResourceSuiteLocalDeferredFixture[A]. It has identical semantics to JVM-only ResourceSuiteLocalFixture[A] (i.e., allocate once per suite) but instead of providing A provides an IO[A] that is semantically blocked until the resource is allocated. This allows the fixture to be used on both JVM and JS.

RaasAhsan · 2021-07-11T22:15:12Z

common/shared/src/test/scala/munit/CatsEffectFixturesSpec.scala

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


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

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.

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

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

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?

Maybe we should call it UnsafeResourceSuiteLocalFixture to emphasize these hazards.

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?

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

Then I would indeed go for the UnsafeResourceSuiteLocalFixture :)

Does the CatsEffectFunFixtures not give you the functionality you desire?

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.

armanbilge · 2021-09-13T20:40:06Z

@danicheg would you have a moment to review this please? I'd find this incredibly useful.

ce2/shared/src/main/scala/munit/CatsEffectSuite.scala

armanbilge · 2021-09-14T23:39:48Z

@milanvdm @danicheg thank you both for your thoughtful reviews, I've pushed some changes which hopefully address both of your concerns.

Renamed ResourceSuiteLocalDeferredFixture to UnsafeResourceSuiteLocalDeferredFixture. Furthermore, added a detailed comment explaining its semantics and why it is unsafe.
Platformed the implementation for JVM and JS, so that on JVM it uses unsafeRunSync() and only uses unsafeRunAndForget() for JS. TBH I'm not convinced this really buys us anything since the caveats still apply to JS platform, but at least it may make things a bit better on JVM.

Let me know what you think!

danicheg

You did a huge job @armanbilge 👍🏻
Sorry, but I was out from your discussion with @milanvdm at some point.
But as I get the root problem is that ResourceSuiteLocalFixture currently not working on JS correctly. And adding the ResourceSuiteLocalDeferredFixture would mean that ResourceSuiteLocalFixture will still not working on JS.
Maybe it's possible to change ResourceSuiteLocalFixture to have a working version both on JVM and JS? For example, we could put implement details of munit.CatsEffectFixtures.ResourceSuiteLocalFixture#apply for JVM and JS separately.
So we would have one suite to work with resource fixtures both on JVM and JS.
What do you folks think?

armanbilge · 2021-09-15T18:04:37Z

@danicheg Unfortunately, it is not possible to implement ResourceSuiteLocalFixture fixture on JS, even if we implement it separately. The reason is because a Resource[IO, T] allocates a T asynchronously but the fixture setup must run synchronounously. The reason this works on JVM is because you can use unsafeRunSync() to wait for T: https://github.com/typelevel/munit-cats-effect/blob/main/common/jvm/src/main/scala/munit/CatsEffectFixtures.scala#L41

On JS, there is only one thread and therefore no unsafeRunSync() available. So, it is impossible to allocate a T synchronously. So, the trick I used here, is instead of providing a T, to provide an IO[T] that is backed by a Deferred[IO, T]. This is possible, because although you cannot allocate T synchronously, you can allocate Deferred[IO, T] synchronously and then provide an IO[T] that will semantically block until the deferred is set with the allocated T.

Notice that this requires making a source-incompatible change to the signature. Which is obviously not an impossible thing to do, but it would be painful for any downstream currently using ResourceSuiteLocalFixture.

If we really want to fix all this craziness: would require a PR to munit itself to add support for asynchronous fixtures. That would solve all the problems discussed here. See scalameta/munit#186. If you are interested in solving that issue, that would be really amazing!! Especially since so many Typelevel projects are using both CE and munit.

danicheg · 2021-09-15T18:36:50Z

@armanbilge thanks for the detailed answer. What about using cats.effect.IO#unsafeRunAsync instead of unsafeRunSync for JS? I've sketched the implementation of ResourceSuiteLocalFixture for JS: https://gist.github.com/danicheg/be9ed79cd6f5764899e7cc16fe23e48c
That seems semantic is the same as for ResourceSuiteLocalDeferredFixture. Or maybe I missed something.

armanbilge · 2021-09-15T18:42:46Z

Cool, thanks for giving it a try!! Have you tried running it against the test suite? 😉

My concern is that there is no guarantee when unsafeRunAsync will complete. So, a test may start while value is still None.

armanbilge · 2021-09-15T18:45:06Z

Also, you might find the implementation of unsafeRunAndForget interesting :)
https://github.com/typelevel/cats-effect/blob/series/3.x/core/shared/src/main/scala/cats/effect/IO.scala#L709

danicheg · 2021-09-15T18:46:27Z

@armanbilge yep, the tests were passed ._. But I'm not sure if this is legal though.

armanbilge · 2021-09-15T18:49:00Z

Sounds like the test suite is not rigorous enough! 😬 We should fix that. For example, try using your fixture to allocate this resource: Resource.eval(IO.pure("hello").delayBy(1.second)).

danicheg · 2021-09-15T19:03:04Z

@armanbilge sorry, I've double-checked it and tests didn't run at all. To be honest, they are not running and for the current version of ResourceSuiteLocalFixture for JS 🤷🏻

sbt:munit-cats-effect-3> testOnly munit.CatsEffectFixturesSpec
[info] Passed: Total 0, Failed 0, Errors 0, Passed 0
[info] No tests to run for Test / testOnly

armanbilge · 2021-09-15T19:06:21Z

That's probably because ResourceSuiteLocalFixture is in the JVM-only source directory?

danicheg · 2021-09-15T19:21:24Z

@armanbilge yep, it is. So, version with unsafeRunAsync fails on the same case of afterAll as version on Deffered - assertEquals(released, 1). But adding delay has lead to instantiation problems. So, you are definitely right.

armanbilge · 2021-09-15T19:25:40Z

@danicheg thanks for looking into that. So, it definitely sounds like the current test suite is not sufficient then. I'll push a fix for that.

armanbilge · 2021-09-15T19:51:48Z

@danicheg I added an IO.sleep to the fixture tests so that they properly test for asynchronous fixture allocation. Should be good to go!

Implement ResourceSuiteLocalDeferredFixture

16899c4

RaasAhsan reviewed Jul 11, 2021

View reviewed changes

danicheg reviewed Sep 14, 2021

View reviewed changes

ce2/shared/src/main/scala/munit/CatsEffectSuite.scala Outdated Show resolved Hide resolved

ce2/shared/src/main/scala/munit/CatsEffectSuite.scala Outdated Show resolved Hide resolved

armanbilge added 3 commits September 14, 2021 06:56

Make deferred alias private

38fc284

Make deferred alias private

385d19d

Address reviews

1c5deba

armanbilge requested review from milanvdm and danicheg September 14, 2021 23:39

milanvdm approved these changes Sep 15, 2021

View reviewed changes

Merge branch 'main' into deferred-fixture

d613c32

danicheg reviewed Sep 15, 2021

View reviewed changes

danicheg approved these changes Sep 15, 2021

View reviewed changes

Make fixture tests excercise async path

d9ffc91

milanvdm merged commit 3399cac into typelevel:main Sep 16, 2021

armanbilge mentioned this pull request Dec 24, 2021

Lack of Cross-Platform Support for ResourceSuiteLocalFixture #172

Closed

armanbilge mentioned this pull request Feb 17, 2022

Enable MiMa #191

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cross-platform suite-local fixtures #104

Cross-platform suite-local fixtures #104

armanbilge commented Jul 11, 2021 •

edited

Loading

RaasAhsan Jul 11, 2021

armanbilge Jul 11, 2021

armanbilge Jul 11, 2021

RaasAhsan Jul 11, 2021

armanbilge Jul 11, 2021 •

edited

Loading

armanbilge Sep 14, 2021

milanvdm Sep 14, 2021

armanbilge Sep 14, 2021

milanvdm Sep 14, 2021

armanbilge Sep 14, 2021

armanbilge commented Sep 13, 2021

armanbilge commented Sep 14, 2021

danicheg left a comment

armanbilge commented Sep 15, 2021 •

edited

Loading

danicheg commented Sep 15, 2021

armanbilge commented Sep 15, 2021

armanbilge commented Sep 15, 2021

danicheg commented Sep 15, 2021

armanbilge commented Sep 15, 2021

danicheg commented Sep 15, 2021 •

edited

Loading

armanbilge commented Sep 15, 2021

danicheg commented Sep 15, 2021

armanbilge commented Sep 15, 2021

armanbilge commented Sep 15, 2021

Cross-platform suite-local fixtures #104

Cross-platform suite-local fixtures #104

Conversation

armanbilge commented Jul 11, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

armanbilge Jul 11, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

armanbilge commented Sep 13, 2021

armanbilge commented Sep 14, 2021

danicheg left a comment

Choose a reason for hiding this comment

armanbilge commented Sep 15, 2021 • edited Loading

danicheg commented Sep 15, 2021

armanbilge commented Sep 15, 2021

armanbilge commented Sep 15, 2021

danicheg commented Sep 15, 2021

armanbilge commented Sep 15, 2021

danicheg commented Sep 15, 2021 • edited Loading

armanbilge commented Sep 15, 2021

danicheg commented Sep 15, 2021

armanbilge commented Sep 15, 2021

armanbilge commented Sep 15, 2021

armanbilge commented Jul 11, 2021 •

edited

Loading

armanbilge Jul 11, 2021 •

edited

Loading

armanbilge commented Sep 15, 2021 •

edited

Loading

danicheg commented Sep 15, 2021 •

edited

Loading