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

Extension vs composition for typed Scala Testkit #24598

Closed
johanandren opened this Issue Feb 22, 2018 · 10 comments

Comments

3 participants
@johanandren
Member

johanandren commented Feb 22, 2018

The ActorTestKit for typed was made a trait for mixin, mostly because that's how the old testkit is mostly used. But maybe we should rather encourage the same style as the typed Java API where the testkit is always a field inside your test.

Makes somewhat more OO-sense at least to me, that a testkit is something you have and can use, than that a test is a testkit. But on the other hand the mixing is quite convenient.

We could also provide both, but I don't think we should have two ways to do the same unless there are clear situations where either one of them is a bad fit. Having a two ways that works pretty much for all use cases is just wasting the cognitive energy of our users.

@patriknw

This comment has been minimized.

Show comment
Hide comment
@patriknw

patriknw Feb 22, 2018

Member

I had a few more arguments for composition:

  • that style is already in several of the testing tools, such as TestProbe, BehaviorTestkit
  • no risk of name clashes when we add more methods
  • you can more easily create multiple in same test
Member

patriknw commented Feb 22, 2018

I had a few more arguments for composition:

  • that style is already in several of the testing tools, such as TestProbe, BehaviorTestkit
  • no risk of name clashes when we add more methods
  • you can more easily create multiple in same test

@johanandren johanandren added this to Backlog in Akka Typed Feb 23, 2018

@ktoso

This comment has been minimized.

Show comment
Hide comment
@ktoso

ktoso Feb 27, 2018

Member

The field style IMHO leads to increasing the perception "omg so complex!", since people in testing toolkits are used to static importing methods like matchers (scalatest, junit, testng, mockito, any testing tool I know really).

Upside of extending trait:

  • could hooks into lifecycle, and can stop actor system for you -- otherwise everyone has to re-implement that top level class anyway
    • we never shipped those testkit-specs / testkit-scalatest / testkit-junit, but we could do so to ease adoption...
  • methods are available, easily discoverable
    • discoverability is an important one I think

Alternate:

  • provide static import for the methods
    • "feels java"
  • not sure if that's doable since it needs state... we'd have to do some nasty magic to make it happen I guess.
Member

ktoso commented Feb 27, 2018

The field style IMHO leads to increasing the perception "omg so complex!", since people in testing toolkits are used to static importing methods like matchers (scalatest, junit, testng, mockito, any testing tool I know really).

Upside of extending trait:

  • could hooks into lifecycle, and can stop actor system for you -- otherwise everyone has to re-implement that top level class anyway
    • we never shipped those testkit-specs / testkit-scalatest / testkit-junit, but we could do so to ease adoption...
  • methods are available, easily discoverable
    • discoverability is an important one I think

Alternate:

  • provide static import for the methods
    • "feels java"
  • not sure if that's doable since it needs state... we'd have to do some nasty magic to make it happen I guess.
@ktoso

This comment has been minimized.

Show comment
Hide comment
@ktoso

ktoso Feb 27, 2018

Member

I guess the lifecycle thing causes the number of artifacts to increase much. I wonder if people would like it though; or if it would increase perception that it's magical...

Warming up to the field I guess, would have to try a few tests like that.

Member

ktoso commented Feb 27, 2018

I guess the lifecycle thing causes the number of artifacts to increase much. I wonder if people would like it though; or if it would increase perception that it's magical...

Warming up to the field I guess, would have to try a few tests like that.

@ktoso ktoso added the discuss label Feb 27, 2018

@ktoso

This comment has been minimized.

Show comment
Hide comment
@ktoso

ktoso Feb 27, 2018

Member

Having bumped into it a bit now, I feel this is very not nice:

extends ActorTestKit with TypedAkkaSpecWithShutdown 

so it starts a system, but does not manage its lifecycle properly -- this is bad. If we make it like this, it should also terminate the system (know about hooks in testing frameworks), then it'd be very good -- and accessible to new people.

Member

ktoso commented Feb 27, 2018

Having bumped into it a bit now, I feel this is very not nice:

extends ActorTestKit with TypedAkkaSpecWithShutdown 

so it starts a system, but does not manage its lifecycle properly -- this is bad. If we make it like this, it should also terminate the system (know about hooks in testing frameworks), then it'd be very good -- and accessible to new people.

@johanandren

This comment has been minimized.

Show comment
Hide comment
@johanandren

johanandren Feb 27, 2018

Member

The example in the docs is to make an abstract base class for tests that does the shutdown, so that would be the extends AkkaActorTestKitSpec perhaps. And then name the mixin without the testkit something else.

Member

johanandren commented Feb 27, 2018

The example in the docs is to make an abstract base class for tests that does the shutdown, so that would be the extends AkkaActorTestKitSpec perhaps. And then name the mixin without the testkit something else.

@patriknw

This comment has been minimized.

Show comment
Hide comment
@patriknw

patriknw Mar 4, 2018

Member

I agree that it's better that the lifecycle for create/shutdown is symmetrical, and that is actually another argument for why composition is a better. See the java example: https://doc.akka.io/docs/akka/current/typed/testing.html?language=java#asynchronous-testing

Regarding discoverability I think it's easier to discover things with testkit.<ctrl-space> than a global <ctrl-space>.

Regarding test framework integration I think we can do it for scalatest and then use extension, since that is scalatest style anyway. The dependency would be provided. We should not support more frameworks, because we know how difficult it can be to get the dependency quickly for new scala versions.

Member

patriknw commented Mar 4, 2018

I agree that it's better that the lifecycle for create/shutdown is symmetrical, and that is actually another argument for why composition is a better. See the java example: https://doc.akka.io/docs/akka/current/typed/testing.html?language=java#asynchronous-testing

Regarding discoverability I think it's easier to discover things with testkit.<ctrl-space> than a global <ctrl-space>.

Regarding test framework integration I think we can do it for scalatest and then use extension, since that is scalatest style anyway. The dependency would be provided. We should not support more frameworks, because we know how difficult it can be to get the dependency quickly for new scala versions.

@ktoso

This comment has been minimized.

Show comment
Hide comment
@ktoso

ktoso Mar 6, 2018

Member

As long as we do at least a scalatest integration for lifecycle I'm fine with that -- my prime concern is to not have too much boilerplate when explaining it to new people

Member

ktoso commented Mar 6, 2018

As long as we do at least a scalatest integration for lifecycle I'm fine with that -- my prime concern is to not have too much boilerplate when explaining it to new people

@ktoso

This comment has been minimized.

Show comment
Hide comment
@ktoso

ktoso Mar 6, 2018

Member

(and not leaking systems of course)

Member

ktoso commented Mar 6, 2018

(and not leaking systems of course)

@johanandren

This comment has been minimized.

Show comment
Hide comment
@johanandren

johanandren Mar 6, 2018

Member

I'm now convinced we should go for composition only.

Member

johanandren commented Mar 6, 2018

I'm now convinced we should go for composition only.

@patriknw

This comment has been minimized.

Show comment
Hide comment
@patriknw

patriknw Jun 11, 2018

Member

Conclusion: We agree that composition is the way and we will provide a Scala Test specific trait to glue ActorSystem lifecycle in public testkit, which we will also use internally.

Member

patriknw commented Jun 11, 2018

Conclusion: We agree that composition is the way and we will provide a Scala Test specific trait to glue ActorSystem lifecycle in public testkit, which we will also use internally.

@patriknw patriknw added 3 - in progress and removed 1 - triaged labels Sep 7, 2018

@patriknw patriknw self-assigned this Sep 7, 2018

@patriknw patriknw moved this from Backlog to In Progress in Akka Typed Sep 7, 2018

patriknw added a commit that referenced this issue Sep 7, 2018

ActorTestKit overhaul, #24598
* composition is the basic building block for ActorTestKit
* ActorTestKitWordSpec for integration with ScalaTest
  (automatic shutdown)

patriknw added a commit that referenced this issue Sep 10, 2018

ActorTestKit overhaul, #24598
* composition is the basic building block for ActorTestKit
* ActorTestKitWordSpec for integration with ScalaTest
  (automatic shutdown)

@patriknw patriknw moved this from In Progress to Reviewing in Akka Typed Sep 11, 2018

patriknw added a commit that referenced this issue Sep 12, 2018

ActorTestKit overhaul, #24598
* composition is the basic building block for ActorTestKit
* ActorTestKitWordSpec for integration with ScalaTest
  (automatic shutdown)

patriknw added a commit that referenced this issue Sep 13, 2018

ActorTestKit overhaul, #24598
* composition is the basic building block for ActorTestKit
* ActorTestKitWordSpec for integration with ScalaTest
  (automatic shutdown)

patriknw added a commit that referenced this issue Sep 17, 2018

ActorTestKit overhaul, #24598
* composition is the basic building block for ActorTestKit
* ActorTestKitWordSpec for integration with ScalaTest
  (automatic shutdown)

* Use ActorTestKitWordSpec in our own tests

* doc TestException

patriknw added a commit that referenced this issue Sep 18, 2018

@patriknw patriknw added this to the 2.5.17 milestone Sep 18, 2018

@patriknw patriknw added t:testing and removed discuss labels Sep 18, 2018

@patriknw patriknw closed this Sep 18, 2018

@patriknw patriknw moved this from Reviewing to Done in Akka Typed Sep 18, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment