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

[kyo-reactive-streams]: Remove un-necessary sleep time in test cases #939

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

HollandDM
Copy link
Contributor

@HollandDM HollandDM commented Dec 17, 2024

Originally, Async.sleep was used to mimic the real-life delay of a publisher/subscriber, but in the reactive-streams-tck test cases, it provides little to no benefit while significantly slowing down the overall test. Additionally, a quick look at fs2 and zio shows that they also do not use sleep in their test cases.

This PR takes it a step further by removing Async.sleep from all kyo-reactive-stream test cases. Since this feature is primarily about correctly combining effects, I believe that if individual components have been thoroughly tested, we can proceed with just the minimal testing.

Furthermore, because StreamSubscriber has two strategies, I initially duplicated the tests for both. However, I think it suffices to randomly choose one strategy for each test in the suite.

Overall, on my MacBook M3, the old tests took 1 minute and 37 seconds to run, while the new tests complete in just 34 seconds.

Aim to resolve #938

@@ -225,17 +207,13 @@ abstract private class PublisherToSubscriberTest extends Test:
publisher.subscribe(subscriber3)
publisher.subscribe(subscriber4)
}
.andThen(Async.sleep(2.seconds))
.andThen(Async.sleep(1.seconds))
Copy link
Collaborator

Choose a reason for hiding this comment

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

could it be 100ms?

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 is more like a keep alive effect, the whole fiber will be interrupted below in 10.millis. So i dont think duration here is that important

@fwbrasil
Copy link
Collaborator

Another way we can try to speed it up is by running tests in parallel. If you override fork := false in the specific SBT module, the tests will run start running in parallel but without forking JVMs for execution. If I'm not mistaken, SBT will pick up the number of cores as the parallelism. Since these tests seem to use little CPU, we could probably override it to use a higher concurrency but I don't recall the SBT config for that.

@HollandDM
Copy link
Contributor Author

After some playing around, I figured there are 2 solutions:

  1. To set fork := false like you said, this cases it will fork all other test to another JVM and run them sequentially. The downside is that if the test failed, so do the sbt process.
  2. Tweak the concurrentRestrictions and parallelExecution, e.g:
Global / parallelExecution := true
Global / concurrentRestrictions := Def.setting {
      val par = parallelExecution.value
      val max = EvaluateTask.SystemProcessors
      Tags.limitAll(if (par) max else 1) ::
        Tags.limit(Tags.ForkedTestGroup, if (par) max else 1) ::
        Tags.exclusiveGroup(Tags.Clean) ::
        Nil
    }.value

concurrentRestrictions value is the default value taken from sbt codebase, but the Tags.ForkedTestGroup will be free from 1. This will run all the tests in parallel, there for kyo-reactive-streams tests won't block other.
I saw that sbt allow us to group the test cases into group, then decide the fork strategy for each. It bests to group every other tests to one group and kyo-reactive-streams tests to another one and fork them separately, but this will required us to "un-aggregate" the test task and build them manually (as far as i can tell, could be wrong), so it's quite hard to do compare to the 2 solutions above, WDYT?

@fwbrasil
Copy link
Collaborator

fwbrasil commented Dec 18, 2024

@HollandDM I tried the config you mentioned but it still executes sequentially. While debugging, I noticed the TCK tests end up waiting for the timeout in multiple scenarios to wait for potential new events. If I set new TestEnvironment(10L) in the constructor, the tests run in one second. Is that an option?

@HollandDM
Copy link
Contributor Author

@fwbrasil weird, maybe testing environment only has 1 core?
Anw, if we can speed up the test by limiting the waiting time, then it should be good enough, let me update this.

@HollandDM HollandDM force-pushed the fix-reactivestream-tck branch from f4c480f to dad9d90 Compare December 19, 2024 04:02
@HollandDM HollandDM requested a review from fwbrasil December 19, 2024 04:03
@HollandDM HollandDM force-pushed the fix-reactivestream-tck branch from dad9d90 to 5a87bbb Compare December 19, 2024 08:22
fiber1 <- Async.run(subscriber1.stream.run.unit)
fiber2 <- Async.run(subscriber2.stream.run.unit)
fiber3 <- Async.run(subscriber3.stream.run.unit)
fiber4 <- Async.run(subscriber4.stream.run.unit)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I used Fiber.gather but it some time just got stuck, will do some more investigate later

Copy link
Collaborator

@fwbrasil fwbrasil Dec 19, 2024

Choose a reason for hiding this comment

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

What's the goal here? Why not use parallelUnbounded? Fiber.gather is a recent addition, it'd be great if you could try to isolate in case there's an issue

Copy link
Collaborator

@fwbrasil fwbrasil left a comment

Choose a reason for hiding this comment

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

Can we merge this or are you're still planing to work on it? Thanks!

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.

Reactive streams TCK tests are too slow
2 participants