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

transport tests: add deadline tests #2286

Merged
merged 14 commits into from
Aug 19, 2023
Merged

Conversation

MarcoPolo
Copy link
Collaborator

@MarcoPolo MarcoPolo commented May 11, 2023

Integrations test for deadlines

related to #2194

@marten-seemann
Copy link
Contributor

These are the deadlines I had in mind:

SetDeadline(time.Time) error
SetReadDeadline(time.Time) error
SetWriteDeadline(time.Time) error

The NewStream deadline test seems useful. It might be worth fixing the transports that are failing the test.

The Dial deadline test is not really a transport test, but a swarm test. There's no point testing it with different transports, is there? Unless you want to assert that the underlying transport dial is actually canceled in flight, which is much harder to test.

@p-shahi p-shahi mentioned this pull request May 15, 2023
11 tasks
@MarcoPolo
Copy link
Collaborator Author

mplex does not seem to pass these tests in CI. I'm going to skip them for now

p2p/test/transport/deadline_test.go Outdated Show resolved Hide resolved
p2p/test/transport/deadline_test.go Outdated Show resolved Hide resolved
p2p/test/transport/deadline_test.go Outdated Show resolved Hide resolved
p2p/test/transport/deadline_test.go Outdated Show resolved Hide resolved
p2p/test/transport/deadline_test.go Outdated Show resolved Hide resolved
@MarcoPolo
Copy link
Collaborator Author

friendly ping @marten-seemann. This is the last PR for the transport integration test for #2194

Copy link
Contributor

@marten-seemann marten-seemann left a 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.

p2p/test/transport/deadline_test.go Outdated Show resolved Hide resolved
p2p/test/transport/deadline_test.go Outdated Show resolved Hide resolved
p2p/test/transport/deadline_test.go Outdated Show resolved Hide resolved
p2p/test/transport/deadline_test.go Outdated Show resolved Hide resolved
// Set a deadline
s.SetWriteDeadline(time.Now().Add(10 * time.Millisecond))
start := time.Now()
_, err = s.Write(sendBuf)
Copy link
Contributor

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?

Copy link
Collaborator Author

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.

Copy link
Collaborator Author

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) {
Copy link
Contributor

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.

Copy link
Collaborator Author

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.

@marten-seemann marten-seemann changed the title Transport Integration tests: OpenStream and DialPeer deadlines transport tests: OpenStream and DialPeer deadlines Aug 19, 2023
@marten-seemann marten-seemann changed the title transport tests: OpenStream and DialPeer deadlines transport tests: add deadline tests Aug 19, 2023
@marten-seemann marten-seemann merged commit ddc7238 into master Aug 19, 2023
21 checks passed
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