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 tests for async_channel #837

Draft
wants to merge 3 commits into
base: theseus_main
Choose a base branch
from

Conversation

amab8901
Copy link
Contributor

@amab8901 amab8901 commented Feb 12, 2023

addresses 3rd checkbox in #715

Let me know if I did it correctly and if there's anything missing

Copy link
Member

@kevinaboos kevinaboos left a comment

Choose a reason for hiding this comment

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

Thanks! The initial set of tests is decent, but there are a few more scenarios that ought to be tested. The major issue is that the sending and receiving tasks are the same -- for a real test, the sending ought to be done on a different task from the receiving.

The following important scenarios aren't tested (there may be more too):

  • the clone method on Sender/Receiver
  • dropping a Sender or Receiver when multiple ones exist already, rather than just a single one.
  • Trying to send to or receive from a disconnected channel
  • Whether the Receiver::sender() and Sender::receiver() methods properly change the Channel's state in the case of multiple endpoints or a single endpoint.
  • Actually recovering from a channel disconnection using the above methods and then successfully sending and/or receiving through the channel
  • Anything with more than two tasks -- a common use case would be to have several sending tasks and several receiving tasks, so we want to test whether that works properly.

Testing is tough, but in general the goal is to be as rigorous and difficult as possible in order to stress the mechanisms we've implemented rather than test only the easiest scenarios.

@@ -54,7 +54,7 @@ macro_rules! receive_count {
() => (RECEIVE_COUNT.load(Ordering::SeqCst))
}

pub fn main(args: Vec<String>) -> isize {
pub fn main<T:Send>(args: Vec<String>) -> isize {
Copy link
Member

Choose a reason for hiding this comment

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

the main function cannot be parameterized with generic types, since there's no way to invoke it. Have you actually tried to run it to see what happens? T would be nothing, i.e., random junk, meaning it would sporadically fail in the oddest ways.

Copy link
Member

Choose a reason for hiding this comment

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

The test app itself would need to choose a type for T, or a few different types across different test iterations

Copy link
Contributor Author

@amab8901 amab8901 Feb 15, 2023

Choose a reason for hiding this comment

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

How can I use the Sender<T> then, without introducing generic parameter T?

Copy link
Member

Choose a reason for hiding this comment

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

you choose a type when you create the channel, just like with Rust's std channels. So, something like:

let (sender, receiver) = new_channel::<u64>(...);
let (sender, receiver) = new_channel::<String>(...);
let (sender, receiver) = new_channel::<YourCustomTypeThatImplsSend>(...);

@@ -103,11 +103,11 @@ impl From<Error> for core2::io::Error {
///
/// This channel object is not Send/Sync or cloneable itself;
/// it can be shared across tasks using an `Arc`.
struct Channel<T: Send> {
pub struct Channel<T: Send> {
Copy link
Member

Choose a reason for hiding this comment

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

rather than making the Channel type public, or any of its fields or methods, we should simply add methods to Sender and Receiver that invoke the private Channel methods.

I think the only one we really need is channel_status(), so you can add a Sender::channel_status() and Receiver::channel_status() that just call the existing function within Channel.

The reason for this is to achieve proper encapsulation through limited visibility -- we don't necessarily want a caller to be able to access any and all fields/methods within Channel, because then they could potentially violate its semantics by changing things in unforeseen ways.

warn!("WARNING: failed to change ChannelStatus to SenderDisconnected for cloned receiver");
}

// not sure how to test whether receivers were notified
Copy link
Member

Choose a reason for hiding this comment

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

you should print something or assert something in the receiver task, and vice versa for the other example.

drop(sender);

if receiver.channel.channel_status.as_ptr() != &mut async_channel::ChannelStatus::SenderDisconnected {
warn!("WARNING: failed to change ChannelStatus to SenderDisconnected for original receiver");
Copy link
Member

Choose a reason for hiding this comment

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

warning is good, but asserting or throwing an error would be even better. It is a test, after all, so failures should be loud.

@amab8901
Copy link
Contributor Author

Thanks! The initial set of tests is decent, but there are a few more scenarios that ought to be tested. The major issue is that the sending and receiving tasks are the same -- for a real test, the sending ought to be done on a different task from the receiving.

How can I tell whether the sending and receiving tasks are the same?

@kevinaboos
Copy link
Member

How can I tell whether the sending and receiving tasks are the same?

Just like with regular Rust, unless you manually spawned a new task, all of your code in a function or within a series of functions that call each other will all run in the same task. Here I'm suggesting that you create a test scenario that is more representative of real-world channel usage, in that the sender(s) and receiver(s) are typically on different tasks, as it doesn't make much sense for a task to send itself a message via a channel.

@kevinaboos kevinaboos marked this pull request as draft February 24, 2023 00:39
@amab8901 amab8901 mentioned this pull request Apr 1, 2023
6 tasks
@amab8901 amab8901 force-pushed the add_async_channel_test branch from 374e56d to ca426bd Compare April 9, 2023 04:35
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.

2 participants