-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
transport tests: add deadline tests #2286
Conversation
These are the deadlines I had in mind: Lines 59 to 61 in 301a197
The The |
mplex does not seem to pass these tests in CI. I'm going to skip them for now |
5404755
to
49006a7
Compare
fc87c97
to
c9ef563
Compare
30e3707
to
09f3c91
Compare
friendly ping @marten-seemann. This is the last PR for the transport integration test for #2194 |
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.
Can you rebase on top of master, there might have been changes that could cause these tests to fail / pass now.
// Set a deadline | ||
s.SetWriteDeadline(time.Now().Add(10 * time.Millisecond)) | ||
start := time.Now() | ||
_, err = s.Write(sendBuf) |
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.
Why would this run into the deadline? We could just send all the data, right?
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 sendBuf is 1 MiB. Unless we can send 1 MiB in 10ms at the start this would hit the deadline. I could increase the sendbuf to 10 MiB instead to make it even less likely.
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.
bumped to 10MiB
}) | ||
|
||
// Like the above, but with SetDeadline | ||
t.Run("SetDeadline", func(t *testing.T) { |
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.
Not sure if we need this 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.
Why not? Is the thought that this is simply calling Set{Read,Write}Deadline? I don't think that is enforced and a transport could forget to implement this.
4dc78d7
to
ed9f4a1
Compare
Integrations test for deadlines
related to #2194