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

Add overridden duration timeout to StreamTestKit #1468

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mdedetrich
Copy link
Contributor

The context of this change is apache/pekko-http#590 (review). It turns out its extremely cumbersome to manually override the duration so I am adding this public method

@raboof @jrudolph Let me know if this line of thinking is on the right lines.

@@ -35,12 +35,30 @@ object StreamTestKit {
* This assertion is useful to check that all of the stages have
* terminated successfully.
*/
def assertAllStagesStopped[T](block: => T, overrideTimeout: FiniteDuration)(implicit materializer: Materializer): T =
materializer match {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is code duplication here, I can clean it up if the premise of this PR is acceptable.

@mdedetrich mdedetrich force-pushed the add-timeout-duration branch 4 times, most recently from 29df1ad to 14b7ce3 Compare September 9, 2024 10:17
@mdedetrich mdedetrich force-pushed the add-timeout-duration branch 8 times, most recently from afc1774 to dd9fefe Compare September 9, 2024 12:13
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This might look a bit un-orthodox by Pekko standards, but it is the idiomatic way to override a configuration when in ScalaTest (which is what StreamSpec extends).

The defaault behaviour is still the same (i.e. getting the value from pekko.stream.testkit.all-stages-stopped-timeout) but if you decide to override it then you can do it the ScalaTest specific way.

@pjfanning pjfanning added this to the 1.1.2 milestone Sep 9, 2024
@pjfanning
Copy link
Contributor

including in 1.1.2 milestone - if 1.1.1-RC1 vote fails, we can consider adding this to a 1.1.1-RC2

@mdedetrich
Copy link
Contributor Author

Adding some reviewers to this PR to get an idea whether its even on the right track, @jrudolph @raboof Maybe you have some comments?

import java.util.concurrent.TimeUnit

trait StreamConfiguration extends TestKitBase {
case class StreamConfig(allStagesStoppedTimeout: Span = Span({
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The premise here is that by default we get the timeout value from Pekko typesafe config (as is expected when using Pekko) however if a user wants to override this then its done the ScalaTest idiomatic way (which is overriding a def in a trait, similar to how PatienceConfig works).

Copy link
Member

Choose a reason for hiding this comment

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

It is possible to omit units and support more flexibility, but the code is slightly complex and for reference only.

import scala.jdk.DurationConverters._
import org.scalatest.time.Span._
system.settings.config.getDuration("pekko.stream.testkit.all-stages-stopped-timeout").toScala.convertDurationToSpan

I just saw this {} and wanted to make some changes. I looked at the Span code and found that it has convertDurationoSpan. You can do whatever you want.

@pjfanning pjfanning modified the milestones: 1.1.2, 1.1.x Sep 27, 2024
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.

3 participants