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

chore: additional test case for terminated PromiseActorRef #32129

Merged
merged 8 commits into from Sep 27, 2023

Conversation

Roiocam
Copy link
Contributor

@Roiocam Roiocam commented Sep 23, 2023

References #29795

The key of ISSUE #29795 is PromiseActorRef. In this PR, I have provided coverage for those problems using additional unit tests:

  1. context.ask: this already implement on publish dead-letter if the context.ask has completed on timeout
  2. AskPattern.ask

case class WorkerMultiply(a: Int, b: Int, replyTo: ActorRef[WorkerResult]) extends WorkerCommand
case class WorkerResult(num: Int) extends WorkerCommand

class ManualTerminatedTestSetup(val workerLatch: CountDownLatch) {
Copy link
Member

Choose a reason for hiding this comment

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

I don't think either test here requires blocking with a latch? Asking a terminated actor and ask without any reply leading to timeout should be possible without this kind of infrastructure.

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 original purpose of this was to simulate real-life scenarios. However, since you mentioned it, the testing can be greatly simplified by using probes to replace some interactions.

Copy link
Contributor Author

@Roiocam Roiocam Sep 25, 2023

Choose a reason for hiding this comment

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

@johanandren PTAL, I have pushed a fixes commit, to ensure the testing remains in minimal implementation. Thnaks

Copy link
Member

@johanandren johanandren left a comment

Choose a reason for hiding this comment

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

That's better, one small suggestion on the actorref lambda parameter naming, then this is good to merge.

Co-authored-by: Johan Andrén <johan@markatta.com>
}

"publish dead letter with recipient when AskPattern timeout" in {
testDeadLetterPublishWhenAskTimeout[WorkerResult](ref => ref.ask(replyTo => WorkerMultiply(3, 9, replyTo)))
Copy link
Member

Choose a reason for hiding this comment

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

same name here

Copy link
Contributor Author

@Roiocam Roiocam Sep 26, 2023

Choose a reason for hiding this comment

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

After committing, I realized that the actor in question is not a terminated actor.
Instead, I sent a request to an actor and expected a response, the actor is still alive but it's possible that there is a backlog of messages, and only has an issue when message tardy replies.
Therefore, perhaps a tardyRef would be more appropriate?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry for that, upon reviewing it myself, I realized that there was only one missing test case in the UnitTest I have simplify this PR.

@Roiocam Roiocam changed the title chore: additional DeadLetterSpec chore: additional case for terminated PromiseActorRef Sep 26, 2023
@Roiocam Roiocam changed the title chore: additional case for terminated PromiseActorRef chore: additional test case for terminated PromiseActorRef Sep 26, 2023
Copy link
Member

@johanandren johanandren left a comment

Choose a reason for hiding this comment

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

Even simpler 👍 LGTM

@johanandren johanandren merged commit 8e06109 into akka:main Sep 27, 2023
5 checks passed
@Roiocam Roiocam deleted the additional-deadletter-sepc branch September 27, 2023 07:56
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.

None yet

2 participants