Add asynchronous fixture #418
Conversation
| import scala.concurrent.Future | ||
| import scala.concurrent.ExecutionContext.Implicits.global | ||
|
|
||
| class TestLocalAsyncFixtureJVMSuite extends FunSuite { |
There was a problem hiding this comment.
This suite was added for JVM and JS only because of the bug in native, see scala-native/scala-native#1918
|
Have no idea why |
There was a problem hiding this comment.
Thank you for this contribution! I have wanted to support async fixtures for a long time so I'm fully on board with adding this new feature. I like that you added a separate API to avoid breaking changes.
The refactoring in MUnitRunner is fairly involved so we may need several iterations to get the code into a state that's ready for merging. I did a quick review through the GitHub web UI and I'd love to do at least one review in my IDE and run tests locally to make sure that everything is working as expected. I've tried to make the test suite as comprehensive as possible but some behavior is currently not covered by tests (like stack safety on sync methods).
Don't hesitate to oppose my comments, I may misunderstand something how Future.sequence works under the hood. My comments are based on my understanding of how things work, but that understanding may be wrong or outdated!
| val result = | ||
| for { | ||
| isBeforeAllRun <- runBeforeAll(notifier) | ||
| if isBeforeAllRun |
There was a problem hiding this comment.
What is the error message when this condition fails? It would be best to fail with an exception that explains the reason why tests didn't run: "Aborting test execution because beforeAll hooks failed to run" (or something like that).
There was a problem hiding this comment.
Shame on me. Nice catch, though! I've rewritten it with preserving the semantic of the original version.
|
@olafurpg thank you for that detailed review. It'd be great to take another iteration on this. |
|
@olafurpg I'm sorry for another ping, but I'd be grateful for another iteration of review on this. |
|
👍 this is an important PR, especially for the JS platform where there is no capability to block and thus async fixtures are supported only by unsafe hacks. |
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 <e.danicheg@yandex.ru>
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 <e.danicheg@yandex.ru>
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 <e.danicheg@yandex.ru>
|
@danicheg I apologize the long delay. I spent a while trying to review and refactor the code in this PR but I wasn't happy with the overall changes. I ended up trying a different approach that I like better #430. This different approach allows us to support async and sync fixtures with the same API while removing several cryptic types from the public API ( |
Fixes #186.
This introduces
AsyncFixture[T]that allows creating suite-local and test-local fixtures with asynchronous computations ofbeforeXXXandafterXXXmethods. Also brings some refactoring toMUnitRunnerinternals to make it type-safer and easier to reason about, though more verbose.I think it's better to have both
Fixture[T]andAsyncFixture[T]. It's good to preserve compatibility when we can. Also for some cases, no need for asynchronous boundaries andFixture[T]fits well as it is. But I'm not so convinced about it and ready for discussion and any suggestions.