-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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 ability to create unpersistent versions of persistent behaviors #31464
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking good as an alternative synchronous testkit similar to BehaviorTestKit.
"Unpersistent" sounds a bit funny to me.
Yeah, I bounced between unpersistent and nonpersistent... |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inadvertently did an applyCodeStyle
while in Scala-3 mode...
One bit of functionality that's not implemented yet is the |
|
Okay, hadn't noticed that @johanandren |
051d4f4
to
507f252
Compare
Hi @leviramsey, Thank you for your contribution! We really value the time you've taken to put this together. We see that you have signed the Lightbend Contributors License Agreement before, however, the CLA has changed since you last signed it. |
rebasing on BSL... |
akka-persistence-testkit/src/main/scala/akka/persistence/testkit/ChangePersisted.scala
Outdated
Show resolved
Hide resolved
akka-persistence-testkit/src/main/scala/akka/persistence/testkit/ChangePersisted.scala
Outdated
Show resolved
Hide resolved
akka-persistence-testkit/src/main/scala/akka/persistence/testkit/internal/Unpersistent.scala
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good. I guess I'm slowly getting used to the naming Unpersistent
.
akka-persistence-testkit/src/main/scala/akka/persistence/testkit/ChangePersisted.scala
Outdated
Show resolved
Hide resolved
akka-persistence-testkit/src/main/scala/akka/persistence/testkit/internal/Unpersistent.scala
Outdated
Show resolved
Hide resolved
akka-persistence-testkit/src/main/scala/akka/persistence/testkit/internal/Unpersistent.scala
Outdated
Show resolved
Hide resolved
akka-persistence-testkit/src/main/scala/akka/persistence/testkit/internal/Unpersistent.scala
Outdated
Show resolved
Hide resolved
akka-persistence-testkit/src/main/scala/akka/persistence/testkit/internal/Unpersistent.scala
Outdated
Show resolved
Hide resolved
akka-persistence-testkit/src/main/scala/akka/persistence/testkit/internal/Unpersistent.scala
Outdated
Show resolved
Hide resolved
object UnpersistentBehavior { | ||
|
||
type BehaviorAndChanges[Command, Event, State] = | ||
(Behavior[Command], ConcurrentLinkedQueue[ChangePersisted[State, Event]]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could be more clear to define this as a case class
instead of a tuple
...sistence-testkit/src/main/scala/akka/persistence/testkit/scaladsl/UnpersistentBehavior.scala
Outdated
Show resolved
Hide resolved
akka-persistence-testkit/src/main/scala/akka/persistence/testkit/ChangePersisted.scala
Outdated
Show resolved
Hide resolved
akka-persistence-testkit/src/main/scala/akka/persistence/testkit/ChangePersisted.scala
Outdated
Show resolved
Hide resolved
*/ | ||
def fromEventSourced[Command, Event, State]( | ||
behavior: Behavior[Command], | ||
fromStateAndOffset: Option[(State, Long)] = None): (Behavior[Command], PersistenceProbe[Event], PersistenceProbe[State]) = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The javadsl exposes a wrapper class instead of a Tuple3
, though in the Java API, that exposes a "guaranteed-to-fail" probe for durable state. Could get around that with 4 substantially-identical result types ("Java or Scala" x "Event-sourced or Durable-state"), but that strikes me as a "cure might be worse than the disease" situation
...sistence-testkit/src/main/scala/akka/persistence/testkit/scaladsl/UnpersistentBehavior.scala
Show resolved
Hide resolved
Have not yet adjusted any of the tests |
...rsistence-testkit/src/main/scala/akka/persistence/testkit/javadsl/UnpersistentBehavior.scala
Outdated
Show resolved
Hide resolved
akka-persistence-testkit/src/main/scala/akka/persistence/testkit/internal/Unpersistent.scala
Outdated
Show resolved
Hide resolved
@leviramsey Where are we here? Ready for final review? |
Basically just docs (including adding a testing section for durable state in general, covering some of the other approaches). The code stuff should be OK to review now. (or should we delay docs for another PR?) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another round of review, mostly looking at the API, would be good with a small Java test to see the ergonomics using that API as well (even if I think they'll be pretty 1:1)
...rsistence-testkit/src/main/scala/akka/persistence/testkit/javadsl/UnpersistentBehavior.scala
Outdated
Show resolved
Hide resolved
...rsistence-testkit/src/main/scala/akka/persistence/testkit/javadsl/UnpersistentBehavior.scala
Show resolved
Hide resolved
...-testkit/src/test/scala/akka/persistence/testkit/scaladsl/UnpersistentDurableStateSpec.scala
Outdated
Show resolved
Hide resolved
val recoveryDone = TestInbox[Done]() | ||
val behavior = BehaviorUnderTest("test-1", recoveryDone.ref) | ||
|
||
val (unpersistent, probe) = UnpersistentBehavior.fromDurableState[Command, State](behavior) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know it is just testkit and not production API but returning tuples is not great DX, let's make a result type for Scala as well instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So with a result type patterned on the Java API, you end up with code like
val unpersistent = UnpersistentBehavior.fromDurableState[Command, State](behavior)
val probe = unpersistent.stateProbe
val testkit = unpersistent.behaviorTestKit
or with an extractor, something like the tuple return
val UnpersistentBehavior.DurableState(testkit, probe) =
UnpersistentBehavior.fromDurableState[Command, State](behavior)
OTOH, in the Scala API, maybe it makes the most sense to have the result type take a (BehaviorTestKit[Command], PersistenceProbe[Event], PersistenceProbe[State]) => Unit
(or otherwise for durable state) in addition to the accessors?
So it would be like
UnpersistentBehavior.fromDurableState[Command, State](behavior) { (testkit, probe) =>
// body of test
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That does make the Java API rather different from the Scala API, but I don't get the sense that that sort of resource-like API is at all idiomatic in Java.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A case class so that it is possible to use the unapply if you like, or the object with named fields if you rater like that sounds good to me (better than 3-tuple).
Edit: ignore, comment on previous state of PR, not sure why GitHub didn't show me the latest one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking very good. "Only" reference docs remaining.
And a few nitpicks...
...rsistence-testkit/src/main/scala/akka/persistence/testkit/javadsl/UnpersistentBehavior.scala
Outdated
Show resolved
Hide resolved
...sistence-testkit/src/main/scala/akka/persistence/testkit/scaladsl/UnpersistentBehavior.scala
Show resolved
Hide resolved
...rsistence-testkit/src/main/scala/akka/persistence/testkit/javadsl/UnpersistentBehavior.scala
Outdated
Show resolved
Hide resolved
akka-persistence-testkit/src/main/scala/akka/persistence/testkit/internal/Unpersistent.scala
Outdated
Show resolved
Hide resolved
akka-persistence-testkit/src/main/scala/akka/persistence/testkit/internal/Unpersistent.scala
Outdated
Show resolved
Hide resolved
akka-persistence-testkit/src/main/scala/akka/persistence/testkit/internal/Unpersistent.scala
Outdated
Show resolved
Hide resolved
...rsistence-testkit/src/main/scala/akka/persistence/testkit/javadsl/UnpersistentBehavior.scala
Outdated
Show resolved
Hide resolved
For the doc structure, I'm thinking to make
And also adding a couple of durable state testing pages:
|
Sounds good, but I think it would also be ok to scope the doc effort down and add a section to the existing testing page for typed persistence. Up to you what you have time to complete, we can always add more docs later if needed. |
For now, just adding the section to typed (event-sourced) persistence. Documenting the testing of durable state behaviors in general is a gap in the docs (AFAIK, the main way before this to test them is to combine the If #31673 is merged before this, I will probably adjust the doc snippets to use that (or vice versa, do it in that PR). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some tiny things then this looks good to go
...sistence-testkit/src/main/scala/akka/persistence/testkit/scaladsl/UnpersistentBehavior.scala
Show resolved
Hide resolved
akka-persistence-testkit/src/main/scala/akka/persistence/testkit/internal/Unpersistent.scala
Show resolved
Hide resolved
akka-persistence-testkit/src/main/scala/akka/persistence/testkit/internal/Unpersistent.scala
Show resolved
Hide resolved
akka-persistence-testkit/src/main/scala/akka/persistence/testkit/internal/Unpersistent.scala
Show resolved
Hide resolved
val recoveryDone = TestInbox[Done]() | ||
val behavior = BehaviorUnderTest("test-1", recoveryDone.ref) | ||
|
||
val (unpersistent, probe) = UnpersistentBehavior.fromDurableState[Command, State](behavior) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A case class so that it is possible to use the unapply if you like, or the object with named fields if you rater like that sounds good to me (better than 3-tuple).
Edit: ignore, comment on previous state of PR, not sure why GitHub didn't show me the latest one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM (I don't think my comments are blocking merge on second thought)
Provides an alternative interpreter for
EventSourcedBehavior
andDurableStateBehavior
that does not actually involve any persistence: persistence operations are exposed via a side-channel from the actor.The major benefit of this approach is that the lower-overhead fully-synchronous
BehaviorTestKit
can now be used to test many such behaviors (conditional, of course, on the behavior in question not performing operations which are unsupported in theBehaviorTestKit
: #30050 removesActorContext.ask
from that set of operations). This lower overhead results in somewhat faster tests (see https://gist.github.com/leviramsey/741e64fc9de472e7782aad3ddf40804c) and makes things like property-based testing of persistent behaviors practical.Other minor benefits of this approach might include:
EventSourcedBehaviorTestKit