ActorTestKit overhaul, #24598 #25586

patriknw · 2018-09-07T14:11:37Z

composition is the basic building block for ActorTestKit
ActorTestKitWordSpec for integration with ScalaTest
(automatic shutdown)

I haven't updated existing tests outside akka-actor-testkit-typed, will continue on that next week.

Refs #24598

akka-ci · 2018-09-07T14:27:52Z

Test FAILed.

patriknw · 2018-09-10T08:40:40Z

@ktoso ready for review, I have pushed latest

ktoso · 2018-09-10T08:41:41Z

Great, thanks!

akka-ci · 2018-09-10T09:52:49Z

Test PASSed.

patriknw · 2018-09-12T11:30:31Z

rebased and updated latest

akka-ci · 2018-09-12T12:42:20Z

Test PASSed.

ktoso

Looking honestly very nice :)
Like that we keep our small helper one and the keeping kit as val is also nice for people... Thanks for doing this big refactor!

ktoso · 2018-09-10T10:41:29Z

akka-actor-testkit-typed/src/main/scala/akka/actor/testkit/typed/TestException.scala

+
+import scala.util.control.NoStackTrace
+
+class TestException(msg: String) extends RuntimeException(msg) with NoStackTrace


some quick docs for it since it may show up in our examples etc?

ktoso · 2018-09-10T10:46:54Z

akka-actor-testkit-typed/src/main/scala/akka/actor/testkit/typed/javadsl/ActorTestKit.scala

@@ -157,6 +174,8 @@ final class ActorTestKit protected (delegate: akka.actor.testkit.typed.scaladsl.
   */
  def createTestProbe[M](name: String, clazz: Class[M]): TestProbe[M] = TestProbe.create(name, clazz, system)

+  // Note that if more methods are added here they should also be added to TestKitJunitResource


worth making a ConsistencySpec?

not worth it here, I think. The bulk of methods shouldn't be added here, but in the TestProbe

ktoso · 2018-09-10T11:02:42Z

akka-actor-testkit-typed/src/main/scala/akka/actor/testkit/typed/scaladsl/ActorTestKit.scala

+   * Shortcut for creating a new test probe for the testkit actor system
+   * @tparam M the type of messages the probe should accept
+   */
+  def createTestProbe[M](): TestProbe[M] = TestProbe()(system)


on purpose not spawn here I guess... really sure?

I think it's good to reserve spawn for real actors, a TestProbe is not an actor, it has a ref and much else

ktoso · 2018-09-10T16:21:47Z

...or-testkit-typed/src/main/scala/akka/actor/testkit/typed/scaladsl/ActorTestKitWordSpec.scala

+   * `PatienceConfig` from [[akka.actor.testkit.typed.TestKitSettings#DefaultTimeout]]
+   */
+  implicit val patience: PatienceConfig =
+    PatienceConfig(testKit.testKitSettings.DefaultTimeout.duration, Span(100, org.scalatest.time.Millis))


technically maybe also as config...? though I guess maybe "until people ask for it" is fine here; ignore this request :)

yup, and one can override the val if something special is needed

ktoso · 2018-09-12T15:56:34Z

...or-testkit-typed/src/main/scala/akka/actor/testkit/typed/scaladsl/ActorTestKitWordSpec.scala

+ * `testKit.shutdownTestKit()`.
+ */
+abstract class ActorTestKitWordSpec(testKit: ActorTestKit) extends ActorTestKitBase(testKit)
+  with WordSpecLike with Matchers with BeforeAndAfterAll with ScalaFutures with Eventually {


Yeah that's good enough... If people want different things they can copy this one and e.g. change to FlatSpec or whatnot

ktoso · 2018-09-12T15:58:33Z

...-actor-testkit-typed/src/test/scala/akka/actor/testkit/typed/scaladsl/ActorTestKitSpec.scala

+    testKit.system.name should ===("CompositionSpec")
+  }
+}
+


cool and also usable as docs snippet I suppose

ktoso · 2018-09-12T15:59:05Z

.../src/test/scala/docs/akka/actor/testkit/typed/scaladsl/ScalaTestIntegrationExampleSpec.scala

+    }
+  }
+}
+//#scalatest-integration


ok so that's used as the example snippet, good

ktoso · 2018-09-12T16:05:53Z

akka-docs/src/main/paradox/typed/testing.md

+have the async test kit automatically shutdown when the test is complete.
+
+Note that the dependency on ScalaTest is marked as optional from the test kit module, so your project must explicitly include
+a dependency on ScalaTest to use this.


Was thinking if we should include which version at least... but that will be a hassle to maintain so nah

ktoso · 2018-09-12T16:07:19Z

project/Dependencies.scala

@@ -132,6 +132,9 @@ object Dependencies {

      val junit = Compile.junit % "optional;provided;test"

+
+      val scalatest = Def.setting { "org.scalatest" %% "scalatest" % scalaTestVersion.value % "optional;provided;test" } // ApacheV2


dep / provided looks good... 👍 Did we sanity check what ends up in POM so we don't accidentally do it wrong?
AFAIR some of the scopes sometimes work weird...

it becomes:

<dependency> <groupId>junit</groupId> <artifactId>junit</artifactId> <version>4.12</version> <scope>test</scope> <optional>true</optional> </dependency> <dependency> <groupId>org.scalatest</groupId> <artifactId>scalatest_2.12</artifactId> <version>3.0.5</version> <scope>test</scope> <optional>true</optional> </dependency>

which I guess is fine

akka-ci · 2018-09-13T11:27:58Z

Test PASSed.

* composition is the basic building block for ActorTestKit * ActorTestKitWordSpec for integration with ScalaTest (automatic shutdown) * Use ActorTestKitWordSpec in our own tests * doc TestException

patriknw · 2018-09-17T15:06:58Z

Rebased, including @chbatey improvements as separate commit. Don't squash merge.

chbatey

Few doc things that could be done now or in a follow up as this is conflict central!

LGTM

chbatey · 2018-09-17T15:08:24Z

...stkit-typed/src/main/scala/akka/actor/testkit/typed/scaladsl/ActorTestKitScalaTestSpec.scala

+ * Note that ScalaTest is not provided as a transitive dependency of the testkit module but must be added explicitly
+ * to your project to use this.
+ */
+abstract class ActorTestKitScalaTestSpec(testKit: ActorTestKit) extends ActorTestKitBase(testKit)


It is quite a long name, should we drop Spec at the end?

yes

ActorTestKitScalaTest is also stacking a lot of words together and it might be difficult to see that it is about mixing ActorTestKit and ScalaTest. Perhaps ActorTestKitWithScalaTest, or ScalaTestWithActorTestKit.

The latter is perhaps more correct

chbatey · 2018-09-17T15:10:13Z

akka-docs/src/main/paradox/typed/testing.md

@@ -212,16 +215,19 @@ a dependency on JUnit to use this.

 @@@ div { .group-scala } 

-It often makes sense to introduce a common base class for all tests using the async test kit, here is an example how to
-hook it into a ScalaTest test suite.
+If you are using ScalaTest you can extend `akka.actor.testkit.typed.scaladsl.ActorTestKitWordSpec` to


Needs updating now, i missed that in my branch

chbatey · 2018-09-17T15:19:23Z

akka-docs/src/main/paradox/typed/testing.md

-It often makes sense to introduce a common base class for all tests using the async test kit, here is an example how to
-hook it into a ScalaTest test suite.
+If you are using ScalaTest you can extend `akka.actor.testkit.typed.scaladsl.ActorTestKitWordSpec` to
+have the async test kit automatically shutdown when the test is complete.


add in about overriding afterAll?

chbatey · 2018-09-17T15:20:28Z

akka-docs/src/main/paradox/typed/testing.md


 Java
-:  @@snip [AsyncTestingExampleTest.java](/akka-actor-testkit-typed/src/test/java/akka/actor/testkit/typed/javadsl/AsyncTestingExampleTest.java) { #under-test }
+:  @@snip [AsyncTestingExampleTest.java](/akka-actor-testkit-typed/src/test/java/jdocs/akka/actor/testkit/typed/javadsl/AsyncTestingExampleTest.java) { #under-test }

 @scala[Tests extend `ActorTestKit`. This provides access to]@java[Tests create an instance of `ActorTestKit`. This provides access to]


No longer extend this

patriknw · 2018-09-17T15:31:32Z

I can fix those things here

patriknw · 2018-09-17T15:56:21Z

fixed, used ScalaTestWithActorTestKit but can change to something else if we have better suggestions

akka-ci · 2018-09-17T18:31:46Z

Test PASSed.

ktoso · 2018-09-18T02:41:08Z

...stkit-typed/src/main/scala/akka/actor/testkit/typed/scaladsl/ScalaTestWithActorTestKit.scala

-abstract class ActorTestKitWordSpec(testKit: ActorTestKit) extends ActorTestKitBase(testKit)
-  with WordSpecLike with Matchers with BeforeAndAfterAll with ScalaFutures with Eventually {
+abstract class ScalaTestWithActorTestKit(testKit: ActorTestKit) extends ActorTestKitBase(testKit)
+  with TestSuite with Matchers with BeforeAndAfterAll with ScalaFutures with Eventually {


nice change, since some people may want FlatSpec etc 👍

Name I'd consider using the *Support naming we have in some places in akka http; like "ScalaTestActorTestKitSupport"? If you want to keep though then I can live with this one

I don't think "support" adds much information. Happy with current.

another alternative was to put it in a scalatest package and just call it ActorTestKitSupport

my intention of using the separator With word was to see which parts are together. ScalaTest ActorTestKit or is it Scala TestActor TestkKit or ...

naming, I'll just merge this now

ktoso · 2018-09-18T02:41:38Z

Looks good now, nice improvement; Had naming suggestion if you want to give it a shot using that

akka-ci added validating PR is currently being validated by Jenkins needs-attention Indicates a PR validation failure (set by CI infrastructure) and removed validating PR is currently being validated by Jenkins labels Sep 7, 2018

patriknw force-pushed the wip-24598-testkit-patriknw branch from 7dffdd6 to ced0147 Compare September 10, 2018 08:40

akka-ci added validating PR is currently being validated by Jenkins and removed needs-attention Indicates a PR validation failure (set by CI infrastructure) labels Sep 10, 2018

akka-ci added tested PR that was successfully built and tested by Jenkins and removed validating PR is currently being validated by Jenkins labels Sep 10, 2018

patriknw force-pushed the wip-24598-testkit-patriknw branch from ced0147 to fd913a8 Compare September 12, 2018 11:29

akka-ci added validating PR is currently being validated by Jenkins tested PR that was successfully built and tested by Jenkins and removed tested PR that was successfully built and tested by Jenkins validating PR is currently being validated by Jenkins labels Sep 12, 2018

ktoso approved these changes Sep 12, 2018

View reviewed changes

patriknw force-pushed the wip-24598-testkit-patriknw branch from fd913a8 to debe122 Compare September 13, 2018 10:11

akka-ci added validating PR is currently being validated by Jenkins tested PR that was successfully built and tested by Jenkins and removed tested PR that was successfully built and tested by Jenkins validating PR is currently being validated by Jenkins labels Sep 13, 2018

ActorTestKit overhaul, #24598

b16e4d5

* composition is the basic building block for ActorTestKit * ActorTestKitWordSpec for integration with ScalaTest (automatic shutdown) * Use ActorTestKitWordSpec in our own tests * doc TestException

patriknw force-pushed the wip-24598-testkit-patriknw branch from 0a700d5 to 501a893 Compare September 17, 2018 15:05

chbatey approved these changes Sep 17, 2018

View reviewed changes

make scalatest suite type agnostic

1d0603d

patriknw force-pushed the wip-24598-testkit-patriknw branch from 501a893 to 1d0603d Compare September 17, 2018 15:55

akka-ci added validating PR is currently being validated by Jenkins tested PR that was successfully built and tested by Jenkins and removed tested PR that was successfully built and tested by Jenkins validating PR is currently being validated by Jenkins labels Sep 17, 2018

ktoso reviewed Sep 18, 2018

View reviewed changes

patriknw merged commit 83c7894 into master Sep 18, 2018

patriknw deleted the wip-24598-testkit-patriknw branch September 18, 2018 09:23


		import scala.util.control.NoStackTrace

		class TestException(msg: String) extends RuntimeException(msg) with NoStackTrace

		@@ -132,6 +132,9 @@ object Dependencies {

		val junit = Compile.junit % "optional;provided;test"


		val scalatest = Def.setting { "org.scalatest" %% "scalatest" % scalaTestVersion.value % "optional;provided;test" } // ApacheV2

ActorTestKit overhaul, #24598 #25586

ActorTestKit overhaul, #24598 #25586

Conversation

patriknw commented Sep 7, 2018

akka-ci commented Sep 7, 2018

patriknw commented Sep 10, 2018

ktoso commented Sep 10, 2018

akka-ci commented Sep 10, 2018

patriknw commented Sep 12, 2018

akka-ci commented Sep 12, 2018

ktoso left a 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

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

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

akka-ci commented Sep 13, 2018

patriknw commented Sep 17, 2018

chbatey left a 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

Choose a reason for hiding this comment

patriknw commented Sep 17, 2018

patriknw commented Sep 17, 2018

akka-ci commented Sep 17, 2018

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

ktoso commented Sep 18, 2018