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

Convert ioRuntime to lazy val #226

Merged
merged 1 commit into from
Nov 19, 2021
Merged

Convert ioRuntime to lazy val #226

merged 1 commit into from
Nov 19, 2021

Conversation

ex0ns
Copy link
Contributor

@ex0ns ex0ns commented Oct 21, 2021

By making ioRuntime a lazy val in the AsyncIOSpec the order in which the trait are mixed in the trait does not matter as much as it is current.

Currently, by mixing AsyncIO trait BEFORE another trait/abstract class with a executionContext the execution fails with a Null Pointer Exception.
The following code shows this issue:

import cats.effect.testing.scalatest.AsyncIOSpec
import org.scalatest.flatspec.AsyncFlatSpecLike
import org.scalatest.matchers.should.Matchers
import cats.effect.IO

class Test extends AsyncFlatSpecLike with AsyncIOSpec with  Matchers {
  "A test" should "fail" in {
    IO { 1 shouldBe 2 }
  }
}
new Test().execute() // Test fails as expected

class TestFailure extends AsyncIOSpec with AsyncFlatSpecLike with  Matchers {
  "A test" should "fail" in {
    IO { 1 shouldBe 2 }
  }
}

new TestFailure().execute() // Fails with an NPE

I think that this change could prevent some mistake made when mixing multiple trait inside ScalaTest, I however don't have enough visibility on the impact caused by this change, and what would happen if multiple ExecutionContext were available when creating the ioRuntime.

Feel free to comment/reject/discuss. The workaround/fix is pretty easy to implement when using the library, but the NPE stacktrace is pretty confusing and not easy to debug when running the tests.

By making ioRuntime a lazy val in the AsyncIOSpec the order in which the
trait are mixed in the trait does not matter as much as it is current.

Currently, by mixing AsyncIO trait BEFORE another trait/abstract class
with a executionContext the execution fails with a Null Pointer
Exception.
Copy link
Member

@djspiewak djspiewak left a comment

Choose a reason for hiding this comment

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

Honestly I think this is a good change and a nice catch!

@djspiewak djspiewak merged commit 077d27e into typelevel:series/1.x Nov 19, 2021
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.

2 participants