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

Add support for async fixtures #430

Merged
merged 10 commits into from
Oct 16, 2021
Merged

Conversation

olafurpg
Copy link
Member

Previously, it was not possible to create fixtures that loaded
asynchronously. It was possible to work around this limitation on the
JVM by awaiting on futures, but there was no workaround for Scala.js.
This commit adds support to return futures (and anything that converts
to futures) from the beforeAll/beforeEach/afterEach/afterAll methods.

Supersedes #418. This commit is
inspired by that PR but uses a different approach:

  • No new AsyncFixture[T] type. This simplies the logic in
    MUnitRunner and reduces the size of the MUnit public API.
    The downside is that we introduce a breaking change to the existing
    Fixture[T] API, which will require bumping the version to v1.0.0
    for the next release.
  • This approach supports any Future-like values by hooking into the
    default evaluation of test bodies via munitValueTransforms.

Co-authored-by: Daniel Esik [email protected]

Previously, it was not possible to create fixtures that loaded
asynchronously. It was possible to work around this limitation on the
JVM by awaiting on futures, but there was no workaround for Scala.js.
This commit adds support to return futures (and anything that converts
to futures) from the beforeAll/beforeEach/afterEach/afterAll methods.

Supersedes scalameta#418. This commit is
inspired by that PR but uses a different approach:

- No new `AsyncFixture[T]` type. This simplies the logic in
  `MUnitRunner` and reduces the size of the MUnit public API.
  The downside is that we introduce a breaking change to the existing
  `Fixture[T]` API, which will require bumping the version to v1.0.0
  for the next release.
- This approach supports any `Future`-like values by hooking into the
  default evaluation of test bodies via `munitValueTransforms`.

Co-authored-by: Daniel Esik <[email protected]>
@olafurpg
Copy link
Member Author

I'll try to get back to this PR soon so that it doesn't stay open for too long.

A bug in Scala 2.11/2.12 made `TimeoutSuite` fail with the new async
fixtures. This commit changes how we implement `munitTimeout` to race
two futures instead of calling `Await.result`.
@olafurpg olafurpg force-pushed the async-fixtures branch 2 times, most recently from a0d3f58 to 65704c9 Compare October 11, 2021 08:33
This test case was causing CI failures on Linux that I wasn't able to
reproduce on my local MacBook.
Previously, the test suite was using a parasitic execution context.
@olafurpg
Copy link
Member Author

CI is green now but I haven't properly reviewed the changes yet. There are several moving pieces and still some breaking behavioral changes that need to be documented (for example ordering of fixtures).

@olafurpg
Copy link
Member Author

I'd love to get some input on one design decision. We have three options:

  1. Only support Fixture[T] where lifecycle methods return Any
  2. (current PR) Same as option 1 but additionally add UnitFixture[T] and FutureFixture[T] where lifecycle methods return Unit and Future[Unit] respectively.
  3. (my personal preference) Introduce AnyFixture[T] where lifecycle methods return Any, let Fixture[T] methods return Unit and FutureFixture[T] where methods return Future[Unit].

The current PR uses Option 2 but I don't like the name UnitFixture so I'm leaning towards Option 3 because I want to avoid the situation where IDEs autocomplete Any when you implement Fixture[T]. cc/ @gabro

@gabro
Copy link
Member

gabro commented Oct 14, 2021

Just to throw something out there, have you considered Fixture[T, R] where R is the result type?

@olafurpg
Copy link
Member Author

olafurpg commented Oct 14, 2021

@gabro I considered it but didn't want to write override def munitFixtures: List[Fixture[Id, String]] or override def munitFixtures: List[Fixture[_, _]] for common cases. Under the hood, MUnit works with Any and Future[Any] so it doesn't buy us cleaner internal code. For end users, I feel like Fixture[String] and FutureFixture[String] and TaskFixture[String] will be more ergonomic to write and it will result in more readable code.

@olafurpg
Copy link
Member Author

For the record, MUnit has abstracted on the container type Future[_] from the start and it's been possible to write custom Suite classes for other containers like IO. The idea was that munit.FunSuite was special cased for Future[_] but you could implement test suites based on another container. This PR removes that functionality because MUnit internals have always been tied to Future anyways and most users seemed fine with using munit.FunSuite directly (even to integrate with cats-effect IO).

@gabro
Copy link
Member

gabro commented Oct 14, 2021

Ok, I see it would be a tad annoying to deal with the extra type parameter all of the time.

(my personal preference) Introduce AnyFixture[T] where lifecycle methods return Any, let Fixture[T] methods return Unit and FutureFixture[T] where methods return Future[Unit].

About this, how would one go about implementing IOFixture[T] for instance?

@armanbilge
Copy link
Contributor

armanbilge commented Oct 14, 2021

About this, how would one go about implementing IOFixture[T] for instance?

By converting the IO to a Future (scaladoc).

Edit: here's an example: https://github.com/typelevel/munit-cats-effect/blob/main/common/shared/src/main/scala/munit/CatsEffectFunFixtures.scala

@gabro
Copy link
Member

gabro commented Oct 14, 2021

So it would extend FutureFixture and apply a transform. Sounds reasonable.

In this case I'm 👍 for your solution number 3, @olafurpg, seems to be the most ergonomic

@olafurpg
Copy link
Member Author

It's a bit confusing, but IOFixture would need to extend AnyFixture since the lifecycle method signatures should return IO[Unit]. Value transforms would take care of converting those IO values into futures.

@armanbilge
Copy link
Contributor

Well, most likely users won't be expected to work with the lifecycle methods directly since Cats Effect already has Resource exactly for this. So, munit-cats-effect will take a Resource[IO, A] and turn it into a fixture, just like in the example I linked above.

@olafurpg
Copy link
Member Author

olafurpg commented Oct 14, 2021

Edit: here's an example: https://github.com/typelevel/munit-cats-effect/blob/main/common/shared/src/main/scala/munit/CatsEffectFunFixtures.scala

Also confusingly, FunFixture are actually not using Fixture[T]. Fun fixtures are test-local only and require you to re-define the fixture variable name for every test, unlike Fixture[T] where you define the variable once per-suite and reference it via the apply() method.

@armanbilge
Copy link
Contributor

armanbilge commented Oct 14, 2021

Fun fixtures are test-local only and require you to re-define the fixture variable name for every test,

Yes, I'm aware, sorry for the confusion :) I linked that example because it shows the conversion to Future. There's also a fixture implementation that is JVM-only since it relies on a blocking call:

https://github.com/typelevel/munit-cats-effect/blob/main/common/jvm/src/main/scala/munit/CatsEffectFixturesPlatform.scala

@olafurpg
Copy link
Member Author

It would be a good exercise to implement the new Fixture API with Resource. I know some people have achieved it on the JVM with blocking I/O and it's not pretty. The code won't be much prettier with this PR since Fixture[_] is inherently a state-ful API, but it will at least avoid the blocking I/O making it possible to use MUni async fixtures on Scala.js.

@armanbilge
Copy link
Contributor

It would be a good exercise to implement the new Fixture API with Resource.

If you publish a snapshot of this PR, I'd be happy to make a PR against munit-cats-effect demonstrating what this might look like.

@olafurpg
Copy link
Member Author

Sounds good. I'll try to get this PR in shape before the end of this week.

@armanbilge
Copy link
Contributor

Thanks! Really excited about this.

Previously, the `Fixture` lifecycle methods returned `Any`, which meant
that auto-completions would insert `Any` in the result type when
auto-generating implementation of a fixture. Now, the `Fixture[T]` class
has `Unit` as result types for consistency with older versions of MUnit.
Instead, we introduce `AnyFixture[T]` which only needs to be used when
comibing a list of `Fixture[T]` and `FutureFixture[T]` (along with
    3rdparty fixture classes).
@olafurpg
Copy link
Member Author

I went ahead and implemented option 3 from the alternatives here #430 (comment) This came out much cleaner compared to the previous implementation where it was very easy to auto-complete Any for all life-cycle methods.

@olafurpg
Copy link
Member Author

Will go ahead and merge this and cut a milestone release. I have a few more breaking changes that I want to include in the final version of v1.0.0 so please keep that in mind before upgrading everything to the milestone release

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants