You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@pekko.apache.org by "mdedetrich (via GitHub)" <gi...@apache.org> on 2023/04/12 12:28:06 UTC
[GitHub] [incubator-pekko] mdedetrich opened a new pull request, #283: Add ScalaTestWithActorTestKitBase trait to handle multiple extends
mdedetrich opened a new pull request, #283:
URL: https://github.com/apache/incubator-pekko/pull/283
This PR's factors out the common logic of `ScalaTestWithActorTestKit` into a trait `ScalaTestWithActorTestKitBase` while keeping `ScalaTestWithActorTestKit` a class. The context behind this change is that I was looking at flaky tests within https://github.com/apache/incubator-pekko-persistence-r2dbc and I noticed that one of the reasons behind the flakiness is a hardcoded timeout value (see https://github.com/apache/incubator-pekko-persistence-r2dbc/blob/main/.github/workflows/build-test.yml#L75).
While there may be "easier" ways to solve this problem, a much more principled way is to use testcontainers/testcontainers-scala which already has logic (from community effort) that handles these problems so in reality it just works™.
The easiest way to integrate testcontainers-scala with scalatest/testkit is to just define your test suites within a trait and then have a the different testcontainers extend those traits (as documented here https://github.com/testcontainers/testcontainers-scala/blob/master/docs/src/main/tut/usage.md#lifecycle). Unfortunately currently this is not really possible because `ScalaTestWithActorTestKitBase` is an abstract class and there is no equivalent version that is a trait (in stark contrast to `TestKitBase` which is a trait and not an `abstract class`).
With this feature, we can (for example) change https://github.com/apache/incubator-pekko-persistence-r2dbc/blob/ab3709fec338b2641ae00350e3b07512e2da4acf/core/src/test/scala/org/apache/pekko/persistence/r2dbc/state/CurrentPersistenceIdsQuerySpec.scala#L26 to a `trait CurrentPersistenceIdsQuerySpec` and then have an `PostGresCurrentPersistenceIdsQuerySpec` and `YugabyteCurrentPersistenceIdsQuerySpec` extend `CurrentPersistenceIdsQuerySpec`)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] mdedetrich commented on a diff in pull request #283: Add ScalaTestWithActorTestKitBase trait to handle multiple extends
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on code in PR #283:
URL: https://github.com/apache/incubator-pekko/pull/283#discussion_r1164106793
##########
actor-testkit-typed/src/main/scala/org/apache/pekko/actor/testkit/typed/scaladsl/ScalaTestWithActorTestKit.scala:
##########
@@ -74,6 +70,21 @@ abstract class ScalaTestWithActorTestKit(testKit: ActorTestKit)
*/
def this(config: Config, settings: TestKitSettings) =
this(ActorTestKit(ActorTestKitBase.testNameFromCallStack(), config, settings))
+}
+
+/**
+ * A ScalaTest base trait for the [[ActorTestKit]] which [[ScalaTestWithActorTestKit]] extends. If you find yourself in
+ * the situation where you need to extend the same test suite in different ways then you can implement your tests within
+ * a trait that extends [[ScalaTestWithActorTestKitBase]].
+ */
+trait ScalaTestWithActorTestKitBase
+ extends TestSuite
+ with Matchers
+ with BeforeAndAfterAll
+ with ScalaFutures
+ with Eventually {
+
+ def testKit: ActorTestKit
Review Comment:
This is a deliberately a `def` since its the lowest common denominator, i.e. when extending `ScalaTestWithActorTestKitBase` you can specify the `testKit` field to be either a `val`/`lazy val` or `def` (in most cases I would expect it to be a `val` as it is with `ScalaTestWithActorTestKit` but we shouldn't be overly strict in this)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] mdedetrich commented on a diff in pull request #283: Add ScalaTestWithActorTestKitBase trait to handle multiple extends
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on code in PR #283:
URL: https://github.com/apache/incubator-pekko/pull/283#discussion_r1164106793
##########
actor-testkit-typed/src/main/scala/org/apache/pekko/actor/testkit/typed/scaladsl/ScalaTestWithActorTestKit.scala:
##########
@@ -74,6 +70,21 @@ abstract class ScalaTestWithActorTestKit(testKit: ActorTestKit)
*/
def this(config: Config, settings: TestKitSettings) =
this(ActorTestKit(ActorTestKitBase.testNameFromCallStack(), config, settings))
+}
+
+/**
+ * A ScalaTest base trait for the [[ActorTestKit]] which [[ScalaTestWithActorTestKit]] extends. If you find yourself in
+ * the situation where you need to extend the same test suite in different ways then you can implement your tests within
+ * a trait that extends [[ScalaTestWithActorTestKitBase]].
+ */
+trait ScalaTestWithActorTestKitBase
+ extends TestSuite
+ with Matchers
+ with BeforeAndAfterAll
+ with ScalaFutures
+ with Eventually {
+
+ def testKit: ActorTestKit
Review Comment:
This is a deliberately a `def` since its the lowest common denominator, i.e. when extending `ScalaTestWithActorTestKitBase` you can specify the `testKit` field to be either a `val`/`lazy val` or `def` (in most cases I would expect it to be a `val` but we shouldn't be overly strict in this)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] mdedetrich commented on a diff in pull request #283: Add ScalaTestWithActorTestKitBase trait to handle multiple extends
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on code in PR #283:
URL: https://github.com/apache/incubator-pekko/pull/283#discussion_r1164106793
##########
actor-testkit-typed/src/main/scala/org/apache/pekko/actor/testkit/typed/scaladsl/ScalaTestWithActorTestKit.scala:
##########
@@ -74,6 +70,21 @@ abstract class ScalaTestWithActorTestKit(testKit: ActorTestKit)
*/
def this(config: Config, settings: TestKitSettings) =
this(ActorTestKit(ActorTestKitBase.testNameFromCallStack(), config, settings))
+}
+
+/**
+ * A ScalaTest base trait for the [[ActorTestKit]] which [[ScalaTestWithActorTestKit]] extends. If you find yourself in
+ * the situation where you need to extend the same test suite in different ways then you can implement your tests within
+ * a trait that extends [[ScalaTestWithActorTestKitBase]].
+ */
+trait ScalaTestWithActorTestKitBase
+ extends TestSuite
+ with Matchers
+ with BeforeAndAfterAll
+ with ScalaFutures
+ with Eventually {
+
+ def testKit: ActorTestKit
Review Comment:
This is a deliberately a `def` since its the lowest common denominator, i.e. when extending `ScalaTestWithActorTestKitBase` you can specify the `testKit` field to be either a `val`/`lazy val` or `def` (in most cases I would expect it to be a `val`)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] mdedetrich merged pull request #283: Add ScalaTestWithActorTestKitBase trait to handle multiple extends
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich merged PR #283:
URL: https://github.com/apache/incubator-pekko/pull/283
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org