Remove the Hash constraint on assert_received_unordered #1836
Conversation
didier-wenzek
temporarily deployed
to
Test Pull Request
GitHub Actions
Inactive
Robot Results
Passed Tests
|
didier-wenzek
temporarily deployed
to
Test Pull Request
GitHub Actions
Inactive
didier-wenzek
temporarily deployed
to
Test Pull Request
GitHub Actions
Inactive
| /// A test helper that extends a message box with various way to check received messages. | ||
| #[async_trait] | ||
| pub trait MessageReceiverExt<M: Message> { | ||
| /// Return a new receiver which returns None if no message is received after the given timeout |
There was a problem hiding this comment.
Wouldn't it be better if the timeout resulted in an Err rather than None? So that we can differentiate between premature channel Sender closures resulting in None response from a genuine timeout? Otherwise we'll always have to investigate both these possibilities whenever there's an assertion error, no?
There was a problem hiding this comment.
The issue is that the returned receiver must implement ReceiveMessages MessageReceiver.
In practice, I don't see that as an issue for testing. Either the sender has been closed prematurely while the test is expecting a value or the sender failed to send a message without the given time window. In both case, the test received nothing aka None.
This would matter if not used for testing purpose though. The ReceiveMessages MessageReceiver trait can be extended with recv method with a timeout.
| /// ``` | ||
| async fn assert_received<Samples>(&mut self, expected: Samples) | ||
| where | ||
| Samples: IntoIterator + Send, |
There was a problem hiding this comment.
Why must the Samples be Send? I agree that things in the MessageBox would be Send anyway. But, just wondering if this bound was necessary.
There was a problem hiding this comment.
It is necessary.
Without it, the compiler complains that "future cannot be sent between threads safely". Indeed, we iterate over the samples and await for the matching messages.
| #[async_trait] | ||
| impl<T, M> MessageReceiverExt<M> for T | ||
| where | ||
| T: ReceiveMessages<M> + Send + Sync + 'static, |
There was a problem hiding this comment.
Side commentary unrelated to this PR. I really wish this ReceiveMessages trait was called MessageReceiver.
|
|
||
| async fn skip(&mut self, count: usize) { | ||
| for _ in 0..count { | ||
| let _ = self.recv().await; |
There was a problem hiding this comment.
| let _ = self.recv().await; | |
| let _ = self.recv().with_timeout().await; |
To avoid calls to skip() waiting forever?
There was a problem hiding this comment.
This is not necessary as done by self ... provided self is a receiver that has been wrapped with_timeout().
There was a problem hiding this comment.
I notice that we don't have the same understanding of the with_timeout().
- What is provided here returns a new receiver that implicitly gives up waiting for a message on all
recv()calls after some delay. - What you suggest here is an explicit timeout on a specific
recvcall.
There was a problem hiding this comment.
provided self is a receiver that has been wrapped with_timeout()
Yeah, that's one way to deal with it, sure.
I notice that we don't have the same understanding of the with_timeout()
Yeah, I imagined users using skip() on a receiver instance directly, without first wrapping it with with_timeout(). Now I understand that the users are always expected to wrap their receiver first with with_timeout() before they use this skip() API, unless they are okay with skip() waiting forever. So, yes this design is flexible for both use-cases.
| F: Fn(&T::Item, &M) -> bool, | ||
| F: Send, |
There was a problem hiding this comment.
| F: Fn(&T::Item, &M) -> bool, | |
| F: Send, | |
| F: Fn(&T::Item, &M) -> bool + Send, |
I'm assuming you didn't do it this way to avoid confusion on the return type of Fn that it is bool + Send
There was a problem hiding this comment.
I'm assuming you didn't do it this way to avoid confusion on the return type of Fn that it is bool + Send
Exactly. The 2 lines version seems clearer to me.
| let mut received = Vec::new(); | ||
|
|
||
| while let Some(msg) = self.recv().await { | ||
| expected.retain(|pat| !matching(pat, &msg)); |
There was a problem hiding this comment.
Not a request for change but more of a personal comment: The HashSet impl was far simpler than this, especially with that !matching. I always find negation based logic difficult to follow. But I agree that the Hash constraint was a bigger limitation. So, this is fine.
There was a problem hiding this comment.
Yes, enforcing a Hash constraint was a pain in practice.
The main concern I have with retain is performance, leading to a received-all check with O(n^2) complexity. But this is not a problem for tests with less than 100 samples.
| /// # use tedge_actors::{Builder, NoMessage, ReceiveMessages, RuntimeError, ServiceConsumer, SimpleMessageBox, SimpleMessageBoxBuilder}; | ||
| /// # use std::time::Duration; | ||
| /// # #[tokio::main] | ||
| /// # async fn main() -> Result<(),RuntimeError> { |
There was a problem hiding this comment.
Actually these commented out lines won't be rendered in html. If you intended this, please ignore my comment.
There was a problem hiding this comment.
This is the intent.
- The tokio header is mandatory to use
asyncfunctions in the test. - The
useclauses seems obvious to me. But maybe, I can be wrong.
| /// |pat:&u32,msg:&MyMessage| msg.count() == *pat, | ||
| /// [3,2], |
There was a problem hiding this comment.
My bad (unrelated to PR itseld), I couldn't understand this. Can you explain what the pattern and why the expected is array..?
There was a problem hiding this comment.
The constraints are flexible. The expected list of samples has only to impl IntoIterator (hence, it can be an array or a vector) and the matching function has to compare each sample (provided via a reference) with a message actually received (also provided via a reference).
Samples: IntoIterator + Send,
F: Fn(&Samples::Item, &M) -> bool,
Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>
- with_timeout: to stop the tests on timeout - skip: to skip a given number of messages More helpers might be added later, as skip_while. Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>
Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>
didier-wenzek
force-pushed
the
actors/fix-test-helpers
branch
from
04d33fa
to
c783c5c
Compare
didier-wenzek
temporarily deployed
to
Test Pull Request
GitHub Actions
Inactive
Proposed changes
awaitwas making things looking fine in any case.Hashconstraint on the samples provided totest_helpers::assert_received_unorderedtest_helpers::assert_received_matchingto provide more flexibility by looking for messages matching patterns.Types of changes
Paste Link to the issue
Checklist
cargo fmtas mentioned in CODING_GUIDELINEScargo clippyas mentioned in CODING_GUIDELINESFurther comments