-
Notifications
You must be signed in to change notification settings - Fork 127
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 test #1450
add test #1450
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.
I think we need two tests and some small changes, as noted below.
|
||
// Create a stream and send some data. | ||
let stream_id = client.stream_create(StreamType::BiDi).unwrap(); | ||
client.stream_send(stream_id, &[6; 100]).unwrap(); |
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.
Ok, so is the datagram too big to fit in a packet that has this stream data as well? I'd send a little more to ensure that is the case.
If it is, then that is good. Then you want to:
- have the client create two packets. the first should be smaller than the datagram size, which you can check
- have the server process the first. it should receive stream data, but not a datagram event
- have the server process the second. it should receive the datagram event then
We probably shouldn't rely on the order of events here. That's not something we'd want to promise in the API unless we really had to (and we don't).
Then add another test that sends low priority stream data first and observe the opposite inversion.
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.
Will do
Note that we're also seeing this new test fail. |
This will starve datagrams, which comes with its own risks. Eventually, we'll need a better strategy.
I'll push a new commit that includes every thing to make the test pass. |
185ddc1
to
08a5001
Compare
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 small suggestion for you regarding the tests, that's all.
a9c1009
to
92724e8
Compare
This is the test for #1447